8000 test(developer): add test to kmc-analyze for Warn_PreviousMapFileCouldNotBeLoaded by cvosoft · Pull Request #14038 · keymanapp/keyman · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

test(developer): add test to kmc-analyze for Warn_PreviousMapFileCouldNotBeLoaded #14038

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
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

test(developer): add test to kmc-analyze for Warn_PreviousMapFileCouldNotBeLoaded #14038

wants to merge 1 commit into from
+10 −0

Conversation

cvosoft
Copy link
@cvosoft cvosoft commented May 23, 2025

Summary

This PR adds a unit test for the Warn_PreviousMapFileCouldNotBeLoaded message in AnalyzerMessages.

Context

This is related to the "good first issue" about low test coverage in developer/src/kmc-analyze.

I couldn't run the tests locally on Linux due to Node.js + ESM + ts-node compatibility issues, but I followed the project's existing patterns closely. Feedback welcome!

@github-project-automation github-project-automation bot moved this to Todo in Keyman May 23, 2025
@keymanapp-test-bot keymanapp-test-bot bot added the user-test-missing User tests have not yet been defined for the PR label May 23, 2025
@keymanapp-test-bot
Copy link

User Test Results

Test specification and instructions

ERROR: user tests have not yet been defined

@keymanapp-test-bot keymanapp-test-bot bot added this to the A19S3 milestone May 23, 2025
@keyman-server
Copy link
Collaborator

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.

mcdurdin
mcdurdin requested changes May 23, 2025
View reviewed changes
Copy link
Member
@mcdurdin mcdurdin left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for the PR!

Unfortunately this is not quite what we are looking for in testing the messages. What we want to test is that the analyzer itself generates the message when given a relevant input (the message infrastructure itself is tested in the verifyCompilerMessagesObject call). So you could look at some of the other message tests in other kmc modules to see the pattern, for example see kmc-copy:

keyman/developer/src/kmc-copy/test/copier-messages.tests.ts

Lines 33 to 38 in ba8e5dc

it('should generate ERROR_InvalidKeyboardId if the output keyboard id is invalid', async function() {
const copier = new KeymanProjectCopier();
assert.isTrue(await copier.init(callbacks, {dryRun: true, outPath: '0invalid'}))
assert.isNull(await copier.run('cloud:jawa'));
assert.isTrue(callbacks.hasMessage(CopierMessages.ERROR_InvalidKeyboardId));
});

@mcdurdin
Copy link
Member

I couldn't run the tests locally on Linux due to Node.js + ESM + ts-node compatibility issues, but I followed the project's existing patterns closely. Feedback welcome!

We don't yet have a docker image for building Developer, which would simplify this for sure. @ermshiperete what do you think? Shouldn't be too hard to do because the build scripts already run on Linux, so probably could start from the Web docker image?

@mcdurdin mcdurdin changed the title test(kmc-analyze): add test for Warn_PreviousMapFileCouldNotBeLoaded test(developer): add test to kmc-analyze for Warn_PreviousMapFileCouldNotBeLoaded May 24, 2025
@mcdurdin mcdurdin added developer/ test Any acceptance test issue labels May 24, 2025
@mcdurdin mcdurdin mentioned this pull request May 24, 2025
Closed
@cvosoft
Copy link
Author
cvosoft commented May 25, 2025

Thanks for the detailed feedback! That makes sense.

I'll try to fix my local compatibility issues with Node.js and my Ubuntu (24) sytem first ...

@cvosoft
Copy link
Author
cvosoft commented May 25, 2025

Hi! I tried to follow your suggestion and base the test on CopierMessages, but noticed that it also imports @keymanapp/developer-test-helpers, which seems to be internal and not part of the public repository or the installable packages.

Many greetings.

@keyman-server keyman-server modified the milestones: A19S3, A19S4 May 26, 2025
@mcdurdin
Copy link
Member

Hi! I tried to follow your suggestion and base the test on CopierMessages, but noticed that it also imports @keymanapp/developer-test-helpers, which seems to be internal and not part of the public repository or the installable packages.

Yes, it is an internal package, part of the keyman npm workspace --> developer/src/common/web/test-helpers/package.json. It should resolve automatically if added to the kmc-analyze package.json.

@keyman-server keyman-server modified the milestones: A19S4, A19S5 Jun 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcdurdin mcdurdin mcdurdin requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
developer/ test Any acceptance test issue user-test-missing User tests have not yet been defined for the PR
Projects
Keyman
Status: Todo
Milestone
A19S5
Development

Successfully merging this pull request may close these issues.
3 participants
@cvosoft @keyman-server @mcdurdin
0