[marceloprerender]

What

Replaces Mocha 6 with Vitest 4.1.11 and adds a pull-request test workflow. Until now the tests only ran inside the publish job on push to master.

Same tests, same results

Mocha on master: 39 passing. Vitest on this branch: 39 passed, with the same assertions (93 in index-spec.js). The run used a clean npm ci.

Changes

• done callbacks are now awaited promises, and the assertions run after the await. Before, a failing assertion inside a callback surfaced as an uncaught exception; now it fails its own test.
• vitest.config.js unsets PRERENDER_SERVICE_URL, so the suite is hermetic (it also passes with the variable set to a bogus URL). It runs test files serially because both Express support apps bind port 3000 when loaded.
• The specs import from vitest, because Vitest 4 can't be required. Everything else stays require. assert, sinon, nock and supertest are kept.
• New .github/workflows/test.yml runs npm test on PRs against master, on Node 20, with the same install steps as the publish job.
• .travis.yml: Node 10 → 20, since Vitest needs Node 20 or newer. The runtime engines field is untouched; Vitest is a devDependency only.

🤖 Generated with Claude Code

[marceloprerender]

Test plan

What changed

• Only the test runner changed: Mocha 6 → Vitest 4.1.11. index.js, the published middleware, is untouched.
• A new test.yml workflow runs the suite on PRs. Before, it only ran in the publish job after merge.

Preconditions — Node ≥ 20.

1. [local] The suite runs the same 39 tests

• Do: npm ci && (cd test/support/express3 && npm install) && (cd test/support/express4 && npm install) && npm test
• Expect: Test Files 3 passed, Tests 39 passed.
• Negative: PRERENDER_SERVICE_URL=http://bogus.example npm test still gives 39 passed, because vitest.config.js deletes the variable.

2. [local] Converted done tests still fail when they should

• Do: in test/index-spec.js, change one expected body (e.g. '<html>Google Bot</html>') and run npm test.
• Expect: that test fails as a normal assertion failure, not an uncaught exception or a timeout.

3. [CI] The new PR workflow runs

• Expect: a Test / test check on this PR, green.

Deployed-build check — merging pushes to master, which runs publish.yml. It runs npm test before publishing, and only publishes when package.json's version is above npm's, which this PR doesn't change. So merging publishes nothing.

Acceptance criteria with no block — no ticket.

Automated coverage

Behavior	Covered by	Command	Gap
Middleware behavior	test/index-spec.js, test/express3-spec.js, test/express4-spec.js (unchanged assertions, 93 in index-spec.js)	npm test	none new

Risks / not covered

• .travis.yml was bumped to Node 20, but Travis is probably dead for this repo; it may be worth deleting instead.
• Test files run serially, because both support apps bind port 3000 when loaded.

_{No ticket · commit 0d20ec1 · regenerate with /test-plan 281}

[marcin-prerender]

Review summary

Reviewed in three independent lanes: correctness (code-review skill, high effort), conventions (project reviewers), and comments.

Verdict: request changes. One item blocks: correctness #1, a one-line .npmignore fix. The conventions lane returned PASS with no violations. The comments lane found no issues. The correctness lane ran a clean npm ci and got 39/39 tests passing.

Must fix — correctness

1. vitest.config.js will ship in the published npm tarball (vitest.config.js:1)
   • package.json has no files field, and .npmignore excludes only test and .travis.yml.
   • On the next publish, every consumer of prerender-node will download vitest.config.js. That file calls require('vitest/config'), but Vitest is a devDependency, so consumers won't have it installed.
   • Fix: add vitest.config.js to .npmignore.

Out of scope: older code this PR only touches, and dropped suggestions

Older code (the same pattern exists on master, so this PR did not introduce it):

• Shared state is reset inline instead of in afterEach (test/index-spec.js:180 and about 14 more places).
  • The resets are delete prerender.whitelist/blacklist, prerender.set('afterRender'|'beforeRender', null), the restore of getPrerenderedPageResponse, and delete process.env.PRERENDER_SERVICE_URL.
  • If an assertion throws first, the reset never runs and the change leaks into later tests. #addUserAgents also never resets crawlerUserAgents.
  • This was flagged as a warning. Moving the resets into afterEach would fit the PR's goal of making each failure fail only its own test, but it is optional here.
• In test/express{3,4}-spec.js, mock.done() and server.close() run before assert.ifError(err). This is the same order as on master.

Dropped suggestions (all from refactor-advisor):

• Test helpers to remove duplication (botRequest, renderAndWaitForEnd, assertRespondedWith, it.each for #buildApiUrl): these are refactors beyond a test-runner migration.
• Combining express3/express4 specs with describe.each, and removing the dead first .filteringPath: the duplication and dead code are older than this PR.
• Renaming tests and removing the CF-Visitor / X-Forwarded-Proto comments: those names and comments are unchanged from master.
• Awaiting res.end in the sync beforeRender tests, and using sandbox.stub for getPrerenderedPageResponse: these tests passed before and still pass. Nice to have.
• The unused request require, and converting require/var to ESM: the PR explains why it mixes import and require, and the unused require is older.
• Merging the two "prerender service unavailable" tests: they test different paths (afterRender vs next).
• Adding an ## Observability section to AGENTS.md: unrelated to this PR.

Reviewer coverage note: test-reviewer and test-redundancy-reviewer did not run. They only match *.spec.js files, and this repo names its specs *-spec.js.

[marceloprerender]

Thanks @marcin-prerender. Fixed in a475ce5: vitest.config.js is now in .npmignore.

While checking with npm pack --dry-run, I saw .github/ was in the tarball too. That predates this PR, but the new test.yml would have joined it, so I excluded it as well. The tarball is now CHANGELOG.md, README.md, SECURITY.md, index.js and package.json, the same as master minus .github/.

I'm leaving the out-of-scope items for later. Moving the inline resets into afterEach is a good follow-up, but it changes test structure on master's lines, so it doesn't belong in a runner swap.