-
-
Notifications
You must be signed in to change notification settings - Fork 103
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.
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
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
|
const dir = dirname(fileURLToPath(import.meta.url)); | ||
const KEYMAN_ROOT = resolve(dir, '../../../../../../'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be done in the build script.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note @mcdurdin's comment here: #11300 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move the playwright install out of verify_npm_setup and somewhere where it will only be called for browser-based projects?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yay for esbuild fixing node_modules direct refs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
c8
caches 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