test(developer): add coverage tests for warning messages in kmc-analyze/AnalyzeOskCharacterUse#15021
Conversation
User Test ResultsTest specification and instructions User tests are not required |
|
This pull request is from an external repo and will not automatically be built. The build must still be passed before it can be merged. Ask one of the team members to make a manual build of this PR. |
|
Test-bot: skip |
mcdurdin
left a comment
There was a problem hiding this comment.
Thank you for your PR! It's looking good. I have some changes requested but they are basically cleanup.
developer/src/kmc-analyze/test/osk-character-use-warnings.tests.ts
Outdated
Show resolved
Hide resolved
developer/src/kmc-analyze/test/osk-character-use-warnings.tests.ts
Outdated
Show resolved
Hide resolved
developer/src/kmc-analyze/test/osk-character-use-warnings.tests.ts
Outdated
Show resolved
Hide resolved
developer/src/kmc-analyze/test/osk-character-use-warnings.tests.ts
Outdated
Show resolved
Hide resolved
developer/src/kmc-analyze/test/osk-character-use-warnings.tests.ts
Outdated
Show resolved
Hide resolved
developer/src/kmc-analyze/test/osk-character-use-warnings.tests.ts
Outdated
Show resolved
Hide resolved
|
I’ve addressed all the review comments (file rename, fixture move, setup pattern). Thanks again for the detailed review and explanations — very helpful for learning the Keyman conventions! |
mcdurdin
left a comment
There was a problem hiding this comment.
Looks great! Just two small assertions to add to the tests, for completeness. Approving this with those two changes.
Greatly appreciate your contribution 😁
developer/src/kmc-analyze/test/osk-character-use-messages.tests.ts
Outdated
Show resolved
Hide resolved
developer/src/kmc-analyze/test/osk-character-use-messages.tests.ts
Outdated
Show resolved
Hide resolved
Thanks! I've added the two assertions as suggested — everything should be covered now. 😊 |
| import { AnalyzeOskCharacterUse } from './osk-character-use/index.js'; | ||
| import { TestCompilerCallbacks } from '@keymanapp/developer-test-helpers'; | ||
| import { AnalyzerMessages } from './analyzer-messages.js'; |
There was a problem hiding this comment.
Builds failed; looks like import paths need adjustment (VSCode sometimes doesn't get these quite right automatically):
| import { AnalyzeOskCharacterUse } from './osk-character-use/index.js'; | |
| import { TestCompilerCallbacks } from '@keymanapp/developer-test-helpers'; | |
| import { AnalyzerMessages } from './analyzer-messages.js'; | |
| import { TestCompilerCallbacks } from '@keymanapp/developer-test-helpers'; | |
| import { AnalyzeOskCharacterUse } from '../src/osk-character-use/index.js'; | |
| import { AnalyzerMessages } from '../src/analyzer-messages.js'; |
(reordered per our style guide suggestion, not important though)
There was a problem hiding this comment.
Thanks! The test now runs successfully locally after fixing the import paths.
(I initially thought the issue was due to an outdated Docker image.)
|
Changes in this pull request will be available for download in Keyman version 19.0.146-alpha |
Summary
Adds two new unit tests covering the warning cases
Warn_PreviousMapDidNotIncludeCountsandWarn_PreviousMapDidIncludeCountsinAnalyzeOskCharacterUse.These tests increase branch coverage for
kmc-analyze.