keymanapp
test(developer): add test to kmc-analyze for Warn_PreviousMapFileCouldNotBeLoaded
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!
![keymanapp-test-bot[bot] avatar](https://avatars.githubusercontent.com/in/133554?v=4 "Published by a pub.dev verified publisher"){kind=small}
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.
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?
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 ...
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.
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.
Hi @cvosoft, just following up on this stalled PR. Do you think you'll be able to do any more on it? Note: I am now living in the same TZ as you so will be more accessible for discussion if that helps.
Hi @cvosoft, just following up on this stalled PR. Do you think you'll be able to do any more on it? Note: I am now living in the same TZ as you so will be more accessible for discussion if that helps.
Hi @mcdurdin, thanks for following up and your patience!
I've been quite busy with another project (Angular/DRF) over the last few weeks, but I could definitely see myself picking this up again — I just can't make any promises on timing. I'll let you know when I find a window to work on it.
Cheers!
Updated the PR:
- Rebased on latest master
- Added a working unit test for Warn_PreviousMapFileCouldNotBeLoaded, verified in Docker
- Increased coverage threshold to 75%
- Tested successfully with resources/docker-images/run.sh :developer -- developer/src/kmc-analyze/build.sh test.
Thanks, this looks good. One small change requested:
Rather than poking into the internal method by casting to
any, can you use theunitTestEndPointspattern as shown here:https://github.com/keymanapp/keyman/blob/838aea52ed8c62825030395af6e7ae47f9357cfb/developer/src/kmc-copy/src/KeymanProjectCopier.ts#L716-L720
That helps with maintenance and code analysis in the future 😁
Thanks for the feedback! I’ve implemented the unitTestEndPoints pattern and cleaned up the test as suggested.
Looks good to me! (small suggestion for checking return value of
loadPreviousMapbut no biggie)I think we should duplicate this pattern for other warnings generated in
loadPreviousMapas follow up PR(s).
Updated the test as suggested — using unitTestEndPoints and assert.isNull(), plus some small formatting clean-up.