chore(web): conversion of lm-worker browser-test for @web/test-runner use 🏃 #11404
Conversation
Uses @web/test-runner to run it, making it our first integrated test with the new testing library
… reporter for in-progress summary
Doesn't actually affect the test passing, but it does silence a warning.
User Test ResultsTest specification and instructions User tests are not required Test Artifacts
|
github-actions
bot
added
common/
common/resources/
| const dir = dirname(fileURLToPath(import.meta.url)); | ||
| const KEYMAN_ROOT = resolve(dir, '../../../../../../'); |
There was a problem hiding this comment.
Doing this, along with setting rootDir to it explicitly, allows us to work around modernweb-dev/web#2720 and modernweb-dev/web#2721. Those are based in use of a relative path, but this pattern allows us to use an absolute one instead.
resources/shellHelperFunctions.sh
Outdated
|
|
||
| # Configure Web browser-engine testing environments. As is, this should only | ||
| # make changes when we update the dependency, even on our CI build agents. | ||
| playwright install |
There was a problem hiding this comment.
Use of the new setup uses playwright, which operates by using browser engines directly. It works by downloading specific versions of the relevant browser engines... and this command performs that download.
Note that it essentially performs a npm-global-style install, storing them outside of the repository's file structure. Essentially, once it's fired successfully once (locally or on the build agent), the engines are essentially cached until we change playwright version.
playwright supposedly also detects when previously-downloaded engines are no longer referenced, so it should remove old versions after an upgrade. (I am a touch worried about it "thrashing" them when an upgrade is pending review and/or branch-hopping, though.)
There was a problem hiding this comment.
This should not be done in the build script.
There was a problem hiding this comment.
So, you're saying even the base browser-engine download part shouldn't be done here? Just making sure that's your intent with this comment, given the other comment re: the install-deps concern.
My understanding is that the base install command downloads only the engines it'll use for testing. Obviously, it's not an npm install or npm ci, but it doesn't appear to touch system files. (That would definitely raise concerns.)
There was a problem hiding this comment.
After reviewing what playwright install does, I am more comfortable with leaving this line in. I do think it belongs outside verify_npm_setup because it's only for web-based, not node-based projects, so a dev on Keyman Developer won't want it, for example.
|
For clarity during the review:
The migrated tests show up properly in the test report and passed in the expected manner. |
There was a problem hiding this comment.
While we don't do anything to specifically leverage it here, do note that the test-spec file is in TypeScript, using the .ts extension. The test engine handles it fine with the test-runner esbuild plugin in place.
| assert.isString(LMLayerWorkerCode); | ||
| }); | ||
| }); | ||
|
|
||
| describe('Usage within a Web Worker', function () { | ||
| it('should install itself in the worker context', function (done) { | ||
| let uri = WorkerBuilder.asBlobURI(LMLayerWorkerCode); | ||
| let blob = new Blob([LMLayerWorkerCode], { type: 'text/javascript' }); | ||
| let uri = URL.createObjectURL(blob); |
There was a problem hiding this comment.
This is a subtle change caused due to relocating the test; we shouldn't depend on the parent module's classes for a child module's test.
| new LauncherWrapper(playwrightLauncher({ product: 'chromium' })), | ||
| new LauncherWrapper(playwrightLauncher({ product: 'firefox' })), | ||
| new LauncherWrapper(playwrightLauncher({ product: 'webkit', concurrency: 1 })), | ||
| named(new LauncherWrapper(playwrightLauncher({ product: 'webkit', concurrency: 1, createBrowserContext({browser}) { |
There was a problem hiding this comment.
I've been running into issues with testing against the WebKit engine when concurrency is set greater than 1. I don't think I actually had that problem for this test set, in particular, but I set the workaround in place just the same.
…eb/lm-worker-test-migration
resources/shellHelperFunctions.sh
Outdated
| # make changes when we update the dependency, even on our CI build agents. | ||
| playwright install | ||
| # Needed for tests to work on our Linux BAs | ||
| playwright install-deps |
There was a problem hiding this comment.
Note: this command should not last until production. It is useful, for now, for ensuring things will work once it's a consistent part of build-agent setup, and it lets development proceed while this is a pending change.
There was a problem hiding this comment.
Note @mcdurdin's comment here: #11300 (comment)
There was a problem hiding this comment.
Just following up with conversation on Slack. It's important that this call not be in the script at all, for test, development, or production. If it hits Github, it impacts our CI. We don't want a PR reviewer to have binaries on their machine updated without warning either.
resources/shellHelperFunctions.sh
Outdated
|
|
||
| # Configure Web browser-engine testing environments. As is, this should only | ||
| # make changes when we update the dependency, even on our CI build agents. | ||
| playwright install |
There was a problem hiding this comment.
So, you're saying even the base browser-engine download part shouldn't be done here? Just making sure that's your intent with this comment, given the other comment re: the install-deps concern.
My understanding is that the base install command downloads only the engines it'll use for testing. Obviously, it's not an npm install or npm ci, but it doesn't appear to touch system files. (That would definitely raise concerns.)
I reviewed https://playwright.dev/docs/browsers#managing-browser-binaries and I think it will be okay. But there's a definite push towards testing against pre-release versions of browsers -- can we test support for downlevel browsers as well, or is it only ever latest versions? We want to test against older browsers to make sure we continue to have support for our minimum versions. |
That is the limitation we face by updating our test engine and utilizing modern browser-launchers like Playwright. It is possible to connect to BrowserStack instead of Playwright if we really wish to do so. |
From a discussion in team Slack: [me] A possible hybrid thought: we could run either a once-daily or once-a-sprint BrowserStack run against the legacy mode while keeping the standard test-config focused on modern-browser use. [mcdurdin] Re use of BrowserStack with the new engine: This way, we can get the benefit of stability from locally testing on PR builds while still getting legacy checks via BrowserStack periodically. |
| import { Worker as WorkerBuilder } from "../../../build/lib/web/index.mjs"; | ||
| import { LMLayerWorkerCode } from "/base/common/web/lm-worker/build/lib/worker-main.wrapped.min.js"; | ||
| import * as helpers from "../helpers.mjs"; | ||
| import { assert } from 'chai'; |
There was a problem hiding this comment.
yay for esbuild fixing node_modules direct refs
There was a problem hiding this comment.
We actually get this even without the esbuild plugin - it's automatically provided by vanilla @web/test-runner, be it for .js or .ts files.
Either way, yay for fixing node_modules direct refs!
resources/shellHelperFunctions.sh
Outdated
|
|
||
| # Configure Web browser-engine testing environments. As is, this should only | ||
| # make changes when we update the dependency, even on our CI build agents. | ||
| playwright install |
There was a problem hiding this comment.
After reviewing what playwright install does, I am more comfortable with leaving this line in. I do think it belongs outside verify_npm_setup because it's only for web-based, not node-based projects, so a dev on Keyman Developer won't want it, for example.
…eb/lm-worker-test-migration
|
Changes in this pull request will be available for download in Keyman version 18.0.41-alpha |
Now, after all that setup... we can (finally) start migrating tests. That said, since this will be the start of the test-migration process, I feel that it's wise to start by migrating a single test, rather than do a whole batch right out of the gate.
As it stands, I've always been a bit bothered by the lm-worker loading-test being in common/predictive-text rather than common/web/lm-worker. The test is pretty reasonable to isolate, so I decided to start with it.
Note that patterns established here will generally be used for further test migration - it's best to "have at me" here so we can get things done right the first time for further work.
Also, lest it go unnoticed, note that all other browser-based automated testing for Web still uses the old Karma engine at this point. I've set things up so that we can migrate each module to the new test-engine individually, rather than trying to do all of the tests at once. Things should be far easier to review this way.
Finally, I opted against enabling code-coverage checks here. Reasons:
c8caches from headless tests.v8-to-istanbul- that can makec8's output compatible with what is accessible via @web/test-runner.c8-output coverage data to a matching format (v8-to-istanbul)istanbul-merge)@keymanapp-test-bot skip