search

keymanapp / keyman

Keyman cross platform input methods system running on Android, iOS, Linux, macOS, Windows and mobile and desktop web
https://keyman.com/
Other
372 stars 102 forks source link

chore(web): updates chai, @types/chai dependencies 🏃 #11318

Closed jahorton closed 4 days ago

jahorton commented 2 weeks ago

One major task I identified while working on #11300 (toward resolving #10497) is that we'll need to update our JS testing-assertion library. Chai 4 exists as a CommonJS module, but the modern tools we're looking to migrate toward require ES Module libraries. The Chai team fixed this with version 5, so we may as well update.

This PR's changeset has been directly extracted from an intermediate state of #11300.

This has a few "fun" knock-on effects for our Karma tests; the "framework" loading approach we previously used is not viable with it, as it is not ES-module aware. With a little legwork, we can patch everything up and maintain the tests as they are, just using the new Chai version, before we begin migrating the affected tests to a new testing framework.

@keymanapp-test-bot skip

keymanapp-test-bot[bot] commented 2 weeks ago

User Test Results

Test specification and instructions

User tests are not required

Test Artifacts

jahorton commented 1 week ago

Whoa, noticed something really weird about the last Web test run...

https://build.palaso.org/buildConfiguration/Keymanweb_TestPullRequests/460863

09:09:49   07 05 2024 22:09:49.807:DEBUG [launcher.browserstack]: Shutting down BrowserStackLocal
09:09:49   07 05 2024 22:09:49.826:DEBUG [launcher]: FINISHED -> FINISHED
09:09:49   07 05 2024 22:09:49.827:DEBUG [launcher]: FINISHED -> FINISHED
09:09:49   07 05 2024 22:09:49.963:DEBUG [launcher.browserstack]: chrome 100.0 (Windows 10) (worker 122904664) successfully killed.
09:09:49   07 05 2024 22:09:49.963:DEBUG [launcher]: null -> FINISHED
09:09:49   07 05 2024 22:09:49.963:DEBUG [launcher]: FINISHED -> FINISHED
09:09:53   07 05 2024 22:09:53.602:DEBUG [launcher.browserstack]: Stopped BrowserStackLocal
09:09:53   [common/web/gesture-recognizer] ## test:browser failed
09:09:53   [web] ## test failed
09:09:54   [web] build.sh parameters: <coverage>
09:09:55   [web] ## coverage starting...
09:09:55   [web] Creating coverage report...
09:10:02   [web] ## coverage completed successfully
09:10:02   Process exited with code 0

Somehow, the script kept going after the test failure, with the coverage success masking it!? This isn't from changes in this PR, though.

Edit: this script was unadjusted by the recent #11329 and has its set -x disabled. It fully utilizes builder-script functionality, though. That could be related.

mcdurdin commented 6 days ago

Somehow, the script kept going after the test failure, with the coverage success masking it!?

That's because the build configuration step masks it:

if exist .build-builder (
  node ../resources/gosh/gosh.js ./ci.sh test
) else (
  node ../resources/gosh/gosh.js unit_tests/test.sh -CI
)

find "coverage_action" build.sh > NUL
if not errorlevel 1 (
  node ../resources/gosh/gosh.js ./build.sh coverage
)

Batch files do not exit automatically on failure. You need something like this after each node call:

  if errorlevel 1 exit %errorlevel%
jahorton commented 4 days ago

I'm not a fan of the deep references into node_modules but I guess there isn't a really nice way around that at present?

However, what if we had a stub module called /common/web/test/chai.js which just exported from node_modules:

export { assert, expect } from '../../../../../../../.../../././..,,.,/./.../node_modules/chai/chai.js';

Fortunately, once we convert tests to the new format, all those deep-references disappear. We need them for karma (at least, with our existing configurations), but not with @web/test-runner. Actually, I've seen evidence that there may be a plugin out there we could use to fix them even with karma, but I'm not sure it's worth the time investment since karma's on the way out for us. I could chase that if you'd prefer, though.

What you've suggested here is likely a "decent enough" workaround, and would probably be enough if you do want me to go ahead and make the transitional form cleaner.

See https://github.com/keymanapp/keyman/pull/11404#discussion_r1598135715 as a reference for how it gets fixed upon migration.

mcdurdin commented 4 days ago

Fortunately, once we convert tests to the new format, all those deep-references disappear.

This is good enough for me. No point in doing extra maintenance on an issue that's about to be resolved anyway.

keyman-server commented 3 days ago

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