Repository navigation
test: run the browser pages in Chromium, Firefox and WebKit with Playwright - #73
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 20 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configuration
⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
133b1f5 to
1676099
Compare
![project516-review-bot[bot]](https://avatars.githubusercontent.com/in/4903871?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR adds Playwright-based cross-browser testing (Chromium, Firefox, WebKit) for the existing Mocha browser pages, with isolated (COOP/COEP) and plain modes. The new tests-browsers CI job runs as a non-blocking matrix. The serve.mjs script is refactored to export a reusable function with an --no-isolation flag. Test HTML files now use location.origin for core URLs so any port works. The implementation correctly wraps mocha.run() via an init script to collect results, validates crossOriginIsolated matches the mode, and handles timeouts/hangs with a within helper. All three issues noted by the prior review (hung page output loss, zero-test false pass, non-Error rejection crash) are fixed in this diff.
Not approved: the check on these comments could not run, so they are unverified.
review-bot, model nvidia/nemotron-3-ultra-550b-a55b:free, verdict comment
There was a problem hiding this comment.
This PR adds a new Playwright-based browser test job (tests-browsers) that runs Mocha test pages in Chromium, Firefox, and WebKit with both isolated (COOP/COEP) and plain modes. The scripts/serve.mjs is refactored to export a reusable serve() function with an --no-isolation flag. Test HTML files now use location.origin for core URLs. The job is non-blocking (continue-on-error: true). Several defects exist in the new test runner: non-Error rejections crash the reporter, skipped tests are misreported as 'no tests ran', default arguments allow silent runs, and server/browser cleanup lacks awaits/timeouts.
Not approved: the check on these comments could not run, so they are unverified.
review-bot, model nvidia/nemotron-3-ultra-550b-a55b:free, verdict comment
| const partial = await within(page.evaluate(() => window.__mochaResult).catch(() => null)); | ||
| result = { | ||
| passes: partial?.passes ?? 0, | ||
| failures: [...(partial?.failures ?? []), { title: "(page)", message: err.message }], |
There was a problem hiding this comment.
If err is not an Error (e.g., a string rejection), err.message is undefined and the failure annotation becomes useless. Use String(err?.message ?? err) as done on line 36.
| timeout: PAGE_TIMEOUT_MS, | ||
| }); | ||
| result = await page.evaluate(() => window.__mochaResult); | ||
| if (result.passes === 0 && !result.failures.length) { |
There was a problem hiding this comment.
A page with only skipped tests (Mocha "pending" events) has zero passes and zero failures but did run tests. Add a runner.on("pending", ...) listener in collectResults to track pending count, or check runner.stats.pending on end.
| import { chromium, firefox, webkit } from "playwright"; | ||
| import { serve } from "./serve.mjs"; | ||
|
|
||
| const [browserName = "chromium", mode = "isolated"] = process.argv.slice(2); |
There was a problem hiding this comment.
Running node scripts/test-browser.mjs without arguments silently defaults to chromium/isolated. Require both args and exit with usage when process.argv.length < 4.
| failed ||= failures.length > 0; | ||
| } | ||
| await browser.close(); | ||
| server.close(); |
There was a problem hiding this comment.
server.close() returns a Promise; not awaiting it means the process exits before the server finishes closing. Use await new Promise(resolve => server.close(resolve)) for clean shutdown.
| }; | ||
|
|
||
| const server = serve(0, { isolated }); | ||
| await new Promise((resolve) => server.on("listening", resolve)); |
There was a problem hiding this comment.
No timeout on server startup. If the server fails to listen (e.g., permission error), this hangs indefinitely. Add a timeout via within() or Promise.race.
| const server = serve(0, { isolated }); | ||
| await new Promise((resolve) => server.on("listening", resolve)); | ||
| const origin = `http://localhost:${server.address().port}`; | ||
| const browser = await launcher.launch(); |
There was a problem hiding this comment.
No timeout on browser launch. If Playwright fails to start the browser, this hangs indefinitely. Wrap with a timeout.
Goal
Run the existing Mocha browser pages in Chromium, Firefox and WebKit, and test the st core on a page with no SharedArrayBuffer. Failures print test titles and messages.
What changed
scripts/test-browser.mjs <chromium|firefox|webkit> <isolated|plain>: starts the static server in-process, opens each page with Playwright, wrapsmocha.run()through an init script to collect pass counts and failures (window.__mochaResult), and prints failing titles and errors, plus the page console on failure. It assertscrossOriginIsolatedmatches the mode so a plain run cannot silently be isolated.scripts/serve.mjs: exportsserve(port, { isolated }); the CLI gains--no-isolation. Default behavior is unchanged.tests/ffmpeg-{st,mt,esm-fallback}.test.html: core URLs uselocation.origininstead ofhttp://localhost:3000, so any port works. The Mocha test files are untouched.tests-browsersjob, matrix of 3 browsers x {isolated, plain}.plainruns the three st pages;isolatedalso runs mt. The job hascontinue-on-error: trueand is not a required check.testsjob andmocha-headless-chromeare untouched for now.Results
The first CI run passed all six combinations. WebKit isolated ran ffmpeg-core-st (24 tests), ffmpeg-st (16), ffmpeg-esm-fallback (1) and ffmpeg-mt (16) with no failures, so the mocha hook fires and the pages run in every engine. Rebased on #80, with this job's actions pinned to the same SHAs as the rest of CI.
Not verified locally
Nothing browser-related was run locally (no browsers on the Pi). Lint passes. Expected trouble to look for in the first runs:
crossOriginIsolatedon WebKit Linux may be false, which the runner reports as a page failure.window.mochasetter hook fires in all three engines (a missing hook shows up as a timeout on(page)).Switch the required
testsjob over, and dropmocha-headless-chrome,puppeteer-coreandscripts/dump-test-page.mjs, once this job is green.Review
Review-bot did not review this PR, so a Sonnet subagent reviewed head 1676099. It found that a hung page lost all failure output, because reading the partial result and closing the page had no timeout. It also found that a page that ran zero tests counted as passing, and that a non-Error rejection would crash the report. All three are fixed in 0d8951c, along with a
package.jsonspacing slip. It also noted that all pages run at once in one browser. That matches the existingtestsjob, so it stays.