Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

test(common/web): unit tests for kvk-file-writer #12734

Merged
merged 27 commits into from
Dec 10, 2024
Merged

test(common/web): unit tests for kvk-file-writer #12734

merged 27 commits into from
Dec 10, 2024

Conversation

markcsinclair
Copy link
Contributor

Unit tests for kvk/kvk-file-write.ts

See #9052

@keymanapp-test-bot skip

@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented Nov 28, 2024 •
edited
Loading

User Test Results

Test specification and instructions

User tests are not required

@keymanapp-test-bot keymanapp-test-bot bot added this to the A18S16 milestone Nov 28, 2024
@github-actions github-actions bot added common/ common/web/ test Any acceptance test issue web/ labels Nov 28, 2024
@markcsinclair markcsinclair marked this pull request as draft November 28, 2024 07:47
@markcsinclair markcsinclair self-assigned this Nov 28, 2024
@github-actions github-actions bot added web/ and removed web/ labels Nov 28, 2024
@github-actions github-actions bot added web/ and removed web/ labels Nov 28, 2024
@markcsinclair
chore(common/web): Merge branch 'master' into test/common/web/types/9…
42191bc 
…052-unit-tests-kvk-file-writer
@markcsinclair
chore(common/web): also check keys as part of initial build test case
4f21cde
@markcsinclair
chore(common/web): uncommented initial test case on write() now initV…
7a462cf 
…isualKeyboard() is no longer incomplete
@markcsinclair
chore(common/web): add more flexibility in setting up keys via initVi…
7e024d2 
…sualKeyboardKey()
@markcsinclair
chore(common/web): add checkVisualKeyboardKey to compare source and b…
027db4b 
…inary keys
@markcsinclair
chore(common/web): add checkBuilderKvkFile() to compare source and bi…
bbc8fa7 
…nary keyboards
@markcsinclair
chore(common/web): simplify checks on fonts in checkBuilderKvkFile
da4162b
@markcsinclair
chore(common/web): added greater flexibility via optional parameters …
5479415 
…to initVisualKeyboard()
@markcsinclair
chore(common/web): add two test cases for null parameters to setString()
db31ed0
@ermshiperete
Copy link
Contributor

ermshiperete commented Nov 29, 2024 •
edited
Loading

Oh no, another race! 😄 If #12709 gets merged first you'll have to rename the new test files to match the new *.tests.ts naming pattern (and tests location).

@markcsinclair
chore(common/web): Merge branch 'test/common/web/types/9052-unit-test…
103009f 
…s-file-types' into test/common/web/types/9052-unit-tests-kvk-file-writer
@github-actions github-actions bot added web/ and removed web/ labels Dec 3, 2024
@github-actions github-actions bot added the web/ label Dec 5, 2024
@github-actions github-actions bot added web/ and removed web/ labels Dec 5, 2024
@markcsinclair
Copy link
Contributor Author

markcsinclair commented Dec 5, 2024 •
edited
Loading

There are problems with null handling of associatedKeyboard, key ansiFont.name and unicodeFont.name that will require a fix, plus incorrect initialisation of BUILDER_KVK_STRING, which should be { len: 1, str: '' } not { len: 0, str: '' }. Test cases have been written to cover the three null strings, but are commented out as they fail.

@darcywong00 darcywong00 modified the milestones: A18S16, A18S17 Dec 7, 2024
@github-actions github-actions bot added web/ and removed web/ labels Dec 9, 2024
@github-actions github-actions bot added web/ and removed web/ labels Dec 9, 2024
@markcsinclair markcsinclair marked this pull request as ready for review December 9, 2024 10:42
@markcsinclair markcsinclair mentioned this pull request Dec 9, 2024
Open
8 tasks
mcdurdin
mcdurdin approved these changes Dec 10, 2024
View reviewed changes
Copy link
Member

@mcdurdin mcdurdin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

common/web/types/tests/kvk/kvk-file-writer.tests.ts Outdated
const binary: BUILDER_KVK_FILE = writer['build'](vk);
checkBuilderKvkFile(binary, vk);
});
// it('can handle a null associatedKeyboard', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to have these uncommented and skip with it.skip? Or will we have compile issues?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests were ready for the fix, so straightforward to shift to it.skip() ... I like it, hadn't seen it before ... reports 3 pending, which is better than commenting out.

common/web/types/tests/kvk/kvk-file-writer.tests.ts Outdated Show resolved Hide resolved
@github-actions github-actions bot added web/ and removed web/ labels Dec 10, 2024
@github-actions github-actions bot added web/ and removed web/ labels Dec 10, 2024
@github-actions github-actions bot added web/ and removed web/ labels Dec 10, 2024
@markcsinclair markcsinclair merged commit 94025c8 into master Dec 10, 2024
3 checks passed
@markcsinclair markcsinclair deleted the test/common/web/types/9052-unit-tests-kvk-file-writer branch December 10, 2024 10:47
@keyman-server
Copy link
Collaborator

Changes in this pull request will be available for download in Keyman version 18.0.157-alpha

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcdurdin mcdurdin mcdurdin approved these changes

@jahorton jahorton Awaiting requested review from jahorton jahorton is a code owner

Assignees

@markcsinclair markcsinclair

Labels
common/web/ common/ test Any acceptance test issue web/
Projects
Keyman
Archived in project
Milestone
A18S17
Development

Successfully merging this pull request may close these issues.
5 participants
@markcsinclair @ermshiperete @keyman-server @mcdurdin @darcywong00