Make the puppeteer test suite run headless with no local server - #3
Conversation
validate.test.cjs previously required a manually started server on localhost:8000 and a headed browser (headless: false), so npm test always failed 26 tests out of the box and the suite could not run in CI. - jest-puppeteer.config.cjs: headless by default; HEADFUL=1 restores a visible browser for debugging; pass --no-sandbox in CI containers. - validate.test.cjs: navigate to an intercepted http://ableplayer.test/ origin fulfilled from memory instead of localhost:8000. A real http(s) origin is still required because isProtocolSafe() resolves relative URLs against window.location.origin (opaque on about:blank), but no server process is needed. - ci.yml: new test-browser job runs the puppeteer project (after npm run build, which produces build/test/validate.umd.js). npm test now passes 81/81 locally with zero setup. Build artifacts intentionally omitted per contributing.md. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe CI workflow now builds the project and runs the Puppeteer Jest project. Puppeteer defaults to headless mode, supports ChangesPuppeteer CI testing
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant npm
participant Jest
participant Puppeteer
participant Chromium
GitHubActions->>npm: Install dependencies
GitHubActions->>npm: Build project
GitHubActions->>Jest: Run Puppeteer project
Jest->>Puppeteer: Launch configured browser
Puppeteer->>Chromium: Start headless browser
Jest->>Chromium: Navigate to ableplayer.test
Chromium-->>Jest: Receive intercepted empty HTML response
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 47-51: Update the workflow steps using actions/checkout and
actions/setup-node to reference approved full commit SHAs instead of version
tags, and add persist-credentials: false to the checkout step’s with
configuration. Preserve the existing Node.js version and npm cache settings.
In `@jest-puppeteer.config.cjs`:
- Line 6: Update the headless option in the Puppeteer configuration to disable
headless mode only when process.env.HEADFUL equals "1"; treat unset, "0", and
"false" values as headless execution.
In `@scripts/__tests__/validate.test.cjs`:
- Around line 17-22: Update the request interception condition in the test setup
to match only the exact navigation URL used by page.goto, rather than every URL
prefixed with http://ableplayer.test/. Preserve the existing synthetic HTML
response for the root navigation while allowing same-origin scripts,
stylesheets, images, and fetches to proceed or use resource-specific fixtures.
- Around line 15-27: Extend the browser fixture around page.goto to capture the
navigation response and assert its status is 200, then add a test asserting
window.location.origin equals http://ableplayer.test. Keep the existing request
interception behavior unchanged and place the regression assertions in
scripts/__tests__ coverage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7d6d00ef-d0da-4b21-8b2f-6e6395d35b41
📒 Files selected for processing (3)
.github/workflows/ci.ymljest-puppeteer.config.cjsscripts/__tests__/validate.test.cjs
| - uses: actions/checkout@v4 | ||
| - uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: 22 | ||
| cache: npm |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Pin the actions and disable checkout credential persistence.
Lines [47-51] use unpinned action references. actions/checkout also retains credentials in the local repository configuration. Pin both actions to approved full commit SHAs and set persist-credentials: false.
Proposed hardening
- - uses: actions/checkout@v4
+ - uses: actions/checkout@<approved-full-commit-sha>
+ with:
+ persist-credentials: false
- - uses: actions/setup-node@v4
+ - uses: actions/setup-node@<approved-full-commit-sha>🧰 Tools
🪛 zizmor (1.28.0)
[warning] 47-47: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 47-47: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 48-48: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml around lines 47 - 51, Update the workflow steps
using actions/checkout and actions/setup-node to reference approved full commit
SHAs instead of version tags, and add persist-credentials: false to the checkout
step’s with configuration. Preserve the existing Node.js version and npm cache
settings.
Source: Linters/SAST tools
| module.exports = { | ||
| launch: { | ||
| headless: false, // Set to true to run tests in headless mode | ||
| headless: process.env.HEADFUL ? false : true, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Treat only HEADFUL=1 as headed mode.
Line [6] checks whether HEADFUL is present. Therefore, HEADFUL=0 and HEADFUL=false also disable headless mode. Compare the value with "1" to preserve unattended execution.
Proposed fix
- headless: process.env.HEADFUL ? false : true,
+ headless: process.env.HEADFUL !== "1",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| headless: process.env.HEADFUL ? false : true, | |
| headless: process.env.HEADFUL !== "1", |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@jest-puppeteer.config.cjs` at line 6, Update the headless option in the
Puppeteer configuration to disable headless mode only when process.env.HEADFUL
equals "1"; treat unset, "0", and "false" values as headless execution.
| await page.setRequestInterception(true); | ||
| page.on("request", (request) => { | ||
| if (request.url().startsWith("http://ableplayer.test/")) { | ||
| request.respond({ | ||
| status: 200, | ||
| contentType: "text/html", | ||
| body: "<!doctype html><html><head></head><body></body></html>", | ||
| }); | ||
| } else { | ||
| request.continue(); | ||
| } | ||
| }); | ||
| await page.goto("http://ableplayer.test/"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Add regression coverage for the intercepted origin.
Lines [15-27] introduce a new browser fixture but do not assert its contract. Capture the navigation response and assert status 200. Add a test that checks window.location.origin is exactly http://ableplayer.test.
As per path instructions, scripts/__tests__/** test expansion is the maintainer's stated top priority; add coverage for this new browser setup.
Proposed test coverage
- await page.goto("http://ableplayer.test/");
+ const response = await page.goto("http://ableplayer.test/");
+ expect(response?.status()).toBe(200);
+test("uses the intercepted HTTP origin", async () => {
+ expect(await page.evaluate(() => window.location.origin)).toBe(
+ "http://ableplayer.test",
+ );
+});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/__tests__/validate.test.cjs` around lines 15 - 27, Extend the browser
fixture around page.goto to capture the navigation response and assert its
status is 200, then add a test asserting window.location.origin equals
http://ableplayer.test. Keep the existing request interception behavior
unchanged and place the regression assertions in scripts/__tests__ coverage.
Source: Path instructions
| if (request.url().startsWith("http://ableplayer.test/")) { | ||
| request.respond({ | ||
| status: 200, | ||
| contentType: "text/html", | ||
| body: "<!doctype html><html><head></head><body></body></html>", | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Restrict the synthetic response to the navigation URL.
Lines [17-22] match every path below http://ableplayer.test/, not only the navigation at /. A same-origin script, stylesheet, image, or fetch then receives an HTML document with status 200. Match the exact URL used by page.goto, or provide resource-specific fixtures.
Proposed fix
- if (request.url().startsWith("http://ableplayer.test/")) {
+ if (request.url() === "http://ableplayer.test/") {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (request.url().startsWith("http://ableplayer.test/")) { | |
| request.respond({ | |
| status: 200, | |
| contentType: "text/html", | |
| body: "<!doctype html><html><head></head><body></body></html>", | |
| }); | |
| if (request.url() === "http://ableplayer.test/") { | |
| request.respond({ | |
| status: 200, | |
| contentType: "text/html", | |
| body: "<!doctype html><html><head></head><body></body></html>", | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/__tests__/validate.test.cjs` around lines 17 - 22, Update the request
interception condition in the test setup to match only the exact navigation URL
used by page.goto, rather than every URL prefixed with http://ableplayer.test/.
Preserve the existing synthetic HTML response for the root navigation while
allowing same-origin scripts, stylesheets, images, and fetches to proceed or use
resource-specific fixtures.
…trict status checks require an up-to-date branch) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
What
npm testcurrently fails 26 of 81 tests out of the box:validate.test.cjsnavigates tohttp://localhost:8000(no server in the repo starts one) andjest-puppeteer.config.cjssetsheadless: false, so the suite needs a manually started server AND a display.Changes
HEADFUL=1restores the visible browser for debugging;--no-sandboxflags added underCIfor container runners.localhost:8000dependency with puppeteer request interception: the page navigates tohttp://ableplayer.test/and the request is fulfilled from memory with an empty document. A real http(s) origin is still required —isProtocolSafe()resolves relative URLs againstwindow.location.origin, which is opaque onabout:blank(the 4 URL tests fail there) — but no server process is needed.test-browserjob runs the puppeteer project afternpm run build(the suite loadsbuild/test/validate.umd.js).Verification
npm test: 81/81 pass locally with zero setup (was 55/81)Note
Internal fork PR to validate before offering upstream — this one directly serves upstream contributing.md's "expanding the test suite is a high priority" by making the existing suite actually runnable.
🤖 Generated with Claude Code
Summary by CodeRabbit