Add browser CI workflow - #136
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
browser/Makefile bundles node_modules/assert/assert.js, which so far arrived only transitively through browserify. Pin the same major explicitly so the shim survives a future toolchain change. Co-authored-by: Claude <noreply@anthropic.com>
The runner exited silently on success; one line from the stats the page already publishes makes the run legible in CI logs and locally, and still prints before a failing verdict throws. Co-authored-by: Claude <noreply@anthropic.com>
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a428cf544
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "allowScripts": { | ||
| "msgpackr-extract": false | ||
| }, |
There was a problem hiding this comment.
On regular installs such as the Node CI workflow's npm install, npm does not enforce this allowScripts metadata, so removing .npmrc causes the optional msgpackr-extract lifecycle script to run again. I confirmed this behavior with npm 11.4.2—the version bundled with the workflow's Node 24 toolchain—where a dependency's install script still executed despite an allowScripts: {dep: false} entry; npm's documented mechanism for suppressing lifecycle scripts is ignore-scripts. Use an enforcement mechanism that npm actually recognizes so the native installer remains blocked as intended.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🔵 Needs a closer look
It reworks build tooling (fragile perl-based rollup rewiring, transitive buffer reliance) and dependency-resolution behavior (omit=optional → npm-v12-only allowScripts) whose effects span multiple environments, warranting a human's final review even though no outright bug was found.
Pull request overview
This PR adds a browser CI pipeline for msgpack-lite, running the existing Mocha suite against the shipped dist/msgpack.min.js in headless Chromium via Playwright. It introduces a self-contained browser/ harness (Makefile + Playwright runner + HTML page) that rollup-bundles the test files, externalizes assert and ../index, and rewires them to page globals (with prebuilt Buffer/assert IIFE shims). It also refactors the test files to drop the old window.msgpack || require(...) browser-detection dance in favor of plain require("../index") and literal titles, and adjusts dependency handling so the browser build works on a clean install.
Changes:
- New
Browser CIworkflow plus abrowser/harness (Makefile,tests.cli.mjs,tests.html) that runs the suite againstdist/msgpack.min.js. - Mechanical refactor of 21 test files: literal
TITLEstrings replace__filename, andrequire("../index")replaces theisBrowser/window.msgpackbranch. - Dependency/config updates:
.npmrc(omit=optional) replaced byallowScriptsinpackage.json, explicit"type": "commonjs", and rollup/plugins/assert added as devDependencies.
File summaries
| File | Description |
|---|---|
.github/workflows/browser.yml |
New Browser CI workflow (push to main + manual), consistent with existing @v7 actions. |
browser/Makefile |
Builds the test/shim bundles via rollup and installs/runs Playwright. |
browser/tests.cli.mjs |
Playwright runner that waits on window.mochaStats and fails on errors/no-tests. |
browser/tests.html |
Loads mocha, dist bundle, shims, and bundled tests in the correct order. |
package.json |
Adds allowScripts, rollup/plugin/assert devDeps, explicit type: commonjs. |
.npmrc |
Removes omit=optional so platform binaries return for the browser build. |
.gitignore |
Ignores generated browser/tests/, keeps browser/tests.cli.mjs tracked. |
test/10–27, 61, 62, 64, 30, 50 *.js |
Literal titles + plain require("../index"); browser-detection removed. |
I verified that all isBrowser/window.msgpack/__filename usages were removed consistently (no stragglers), each literal TITLE matches its filename, the dist global name (msgpack) matches the perl rewiring, 64.decode-limit.js's exclusion from the browser glob is justified (it uses the Node-only process.memoryUsage()), and the allowScripts package.json field is a genuine npm v12 feature. I did not find concrete defects to comment on.
Review details
- Files reviewed: 27/28 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
tests.html exercises the shipped minified bundle, which nothing in the browser flow rebuilt: a source change without a dist refresh would be tested against the old code. Delegate to the root Makefile's file target, which stays the owner of whether a rebuild is due. Co-authored-by: Claude <noreply@anthropic.com>
tests.html needs the shipped bundle whether the suite runs headless or is opened by hand in a browser, so the freshness check belongs to the target that prepares the page, not to the CLI run. Co-authored-by: Claude <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 270bfe355f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| PLAYWRIGHT_VERSION := 1.62.1 | ||
|
|
||
| TEST_SRC := ../test/[12]*.js ../test/6[12]*.js |
There was a problem hiding this comment.
Bundling the 1* and 2* suites into a plain browser IIFE includes test/21.ext.js and test/22.typedarray.js, which evaluate expressions such as global[name]. Chromium does not define the Node global identifier, and none of the loaded shims creates it, so bundled.js raises a ReferenceError while registering tests and the new browser job fails on every run. Replace those lookups with globalThis/window or provide an explicit browser global shim.
Useful? React with 👍 / 👎.
| "allowScripts": { | ||
| "msgpackr-extract": false | ||
| }, |
There was a problem hiding this comment.
When developers or the existing Node.js CI run plain npm install, this allowScripts object is ignored by npm, so deleting .npmrc's omit=optional causes msgpackr-extract to be installed and its lifecycle script to run again. This was reproducible with npm 11.4.2, and older npm versions in the Node 18/20/22 matrix also do not enforce this field; retain an npm-supported restriction such as omitting optional dependencies or using --ignore-scripts at every install site.
Useful? React with 👍 / 👎.
| on: | ||
| push: | ||
| branches: [main] | ||
| workflow_dispatch: |
There was a problem hiding this comment.
This workflow only runs after a commit reaches main (or when manually dispatched), so browser-only regressions introduced by a pull request cannot be detected before merge; the existing Node workflow does not execute make -C browser test. Add a pull_request trigger so this new suite can serve as a pre-merge CI check.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
🟡 Changes recommended
The new Browser CI workflow is missing an npm install step needed for the Makefile-driven dist/test bundling, and the Playwright runner currently hides captured page errors when the run times out.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
browser/tests.cli.mjs:25
pageErrorsare only surfaced afterwaitForFunctionsucceeds. If the page throws during load andwindow.mochaStatsis never set, the run will time out and the collectedpageerrors won’t be included, which makes CI failures much harder to debug.
await page.waitForFunction(() => window.mochaStats !== undefined, null, {timeout: 60_000})
const {tests, passes, pending, failures, duration} = await page.evaluate(() => window.mochaStats)
console.log(`${passes} passing, ${failures} failing, ${pending} pending (${tests} tests, ${duration}ms)`)
if (pageErrors.length) {
- Files reviewed: 27/28 changed files
- Comments generated: 1
- Review effort level: Lite
Summary
Browser CIworkflow for pushes tomainand manual runs, byte-identical to the sibling packages' workflow, with the sharedtests.cli.mjsrunner (also byte-identical): completion is reported throughwindow.mochaStats, an unfinished page times out, and an empty run fails with "Mocha ran no tests"browser/harness: the test bundle externalizesassertand../indexand is rewired to the page globals, so the browser suite exercises the shippeddist/msgpack.min.jsrather than a re-bundled copy of the sourcesBufferandassertcome as prebuilt IIFE shims bundled from the npm packages into page-scopeconstbindings — nowindowpollution, and no Node globals assumed__filename-derived titles and thewindow.msgpack || require(...)dance for literal titles and a plainrequire("../index"), which the bundle rewiring maps to the dist global in browsers.npmrc(omit=optional) is replaced by"allowScripts": { "msgpackr-extract": false }: the omit existed to keep msgpackr's optional native accelerator out, but it also dropped rollup's platform-specific binaries and broke the browser build on a fresh install. The allowScripts entry keeps the accelerator's install script blocked and the install warning-free, while optional dependencies come back"type": "commonjs"becomes explicit, so the.mjsrunner and the.jssources are both unambiguousVerification
make -C browser install && make -C browser test— 177 tests / 177 passes in headless Chromium, againstdist/msgpack.min.jsnpm test— 191 passing (Node; the stream/compat suites stay Node-only)🤖 Generated with Claude Code