Skip to content

build(COMPASS-28): move to jest 30 - #2720

Draft
timdawborn wants to merge 1 commit into
compass-28-typescript-5-9from
compass-28-jest-30
Draft

build(COMPASS-28): move to jest 30#2720
timdawborn wants to merge 1 commit into
compass-28-typescript-5-9from
compass-28-jest-30

Conversation

@timdawborn

@timdawborn timdawborn commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Ticket

COMPASS-28 — Bump spot's NodeJS minimum from 18 to 22

PR 3 of 9. Staying in draft until the whole chain is verified.

Stacked on #2719. Review that first — until it merges, the diff here includes its commit. This PR's own commit is 5b64915.

What

Package From To
jest 29.7.0 ^30.4.2
@types/jest ^29.5.0 ^30
jest-junit ^16.0.0 ^17
@types/supertest ^6.0.0 ^7

ts-jest stays at 29.4.12 — its peer range is already jest: ^29.0.0 || ^30.0.0, so it does not need to move.

Supersedes Dependabot #2623, #2603 and #2688.

No test needed changing

All 54 suites and 553 tests pass unmodified. The only content this touches is the header line of the three snapshot files, which jest 30 rewrites from the goo.gl shortlink to the documentation URL:

-// Jest Snapshot v1, https://goo.gl/fbAQLP
+// Jest Snapshot v1, https://jestjs.io/docs/snapshot-testing

One line per file, three files, and no change to any snapshot body — all 44 snapshots still match. Worth stating explicitly because a snapshot file appearing in a dependency-bump diff is normally the thing to be suspicious of.

The install failure worth reading

jest-resolve 30 depends on unrs-resolver, which ships a postinstall. strictDepBuilds: true did its job and failed the install rather than quietly skipping the script:

ERR_PNPM_IGNORED_BUILDS  Ignored build scripts: unrs-resolver@1.12.2

That script is napi-postinstall — a fallback for when the native binding did not arrive through optionalDependencies. All 22 bindings are pinned in the lockfile, including linux-x64-gnu (CI) and both linux-arm64-gnu / linux-arm64-musl (the emulated arm64 image build), so on every platform we build on the binding installs directly and the script has nothing left to do.

So it is denied rather than allowed — the same treatment core-js already gets in pnpm-workspace.yaml, and the option that does not execute a third-party install script. Verified that jest resolves and runs with the build denied. The image never sees any of this: jest is a devDependency and the runtime stage installs --prod.

If CI disagrees on some platform, the fix is unrs-resolver: true, but that is a supply-chain decision and should not be made pre-emptively.

test-reports/ added to .gitignore

It is jest's own output directory. A local sharded run — the form CI uses — leaves it in the repository root as untracked files, one XML per shard. It belongs in the PR that owns jest.

How this was verified

Locally on Node 22.23.2, all exit 0:

Check Result
pnpm install 0 — clean, no ignored-build error
pnpm build 0
pnpm test 0 — 54 suites, 553 tests, 44 snapshots
pnpm lint:check 0
pnpm build-docs 0
docker build + check-image-parity 0 — all checks passed
validate / ts-lint / checksum / --help 0

Also ran the exact CI invocation, since the reporter and sharding are what a jest-junit major could break:

JEST_JUNIT_OUTPUT_NAME=... pnpm test --ci --reporters=default --reporters=jest-junit --shard=1/4

Shard 1/4 ran 14 suites / 91 tests and produced valid JUnit XML (tests=91 failures=0 errors=0), with JEST_JUNIT_UNIQUE_OUTPUT_NAME still appending a UUID — so test-summary/action's ./test-reports/**/*.xml glob still matches.

@timdawborn

Copy link
Copy Markdown
Contributor Author

Automated review — /pr-review-toolkit:review-pr

Agents run: code-reviewer, comment-analyzer. Scoped to this PR's own commit (5b64915).

🔴 Critical (0)

None.

🟡 Important (1)

.gitignore:8 — the ignore covers CI's configured path, not the one a local run actually produces.

test-reports/ exists only because .github/actions/build-and-test/action.yml:58 sets JEST_JUNIT_OUTPUT_DIR. jest-junit's own defaults are outputDirectory: process.cwd(), outputName: 'junit.xml', and jest.ci.config.js declares the reporter with no env var. Verified empirically:

$ pnpm ci:test -- <one spec>
$ ls junit.xml
-rw-r--r--  869  junit.xml      # at the repo root, and NOT gitignored

So pnpm ci:test still drops an untracked file. Add /junit.xml alongside /test-reports.

This also makes this PR's own commit message wrong: "test-reports/ is jest's own output directory" — it is the CI action's chosen directory. That wording will mislead the next person deciding where reports land.

🔵 Suggestions (2)

  • pnpm-workspace.yaml:19-20 — "the binding for every platform we build on does" is the soft half of an otherwise sound claim. The checkable fact is all 22 @unrs/resolver-binding-* are pinned in the lockfile; state that, and name where the denial stops being sound — on a platform with no pinned binding the failure moves to require time and the denied fallback cannot repair it. Also "and is not run" just restates false on the next line.
  • Worth one line in the PR body, not a change: eslint is a runtime dependency here, and unrs-resolver commonly arrives via eslint-import-resolver-typescript. If that ever lands, this entry stops being dev-only and the "never reaches the image" reasoning expires.

✅ Verified sound

  • Denying the build is the right call, and independently confirmed rather than taken on trust: unrs-resolver@1.12.2's postinstall is postinstall.jsnapi-postinstall.checkAndPreparePackage, whose resolve-then-break path is a no-op when the binding is present. All 22 bindings are in the lockfile covering darwin arm64/x64, linux x64/arm64 gnu+musl and win32-x64.
  • The dev-only claim holds — the lockfile's only dependent of unrs-resolver is jest-resolve@30.4.1, so the --prod runtime stage genuinely never sees it.
  • The snapshot header change is not suspicious. @jest/snapshot-utils@30.4.1 sets SNAPSHOT_GUIDE_LINK to the docs URL with SNAPSHOT_VERSION still '1', and validateSnapshotHeader compares only the version. 44/44 snapshots green.

Recommended action

  1. Add /junit.xml to .gitignore and correct the commit-message wording about whose directory test-reports/ is.
  2. Optionally tighten the allowBuilds comment to the checkable fact.

`jest` to ^30.4.2, `@types/jest` to ^30, `jest-junit` to ^17 and
`@types/supertest` to ^7. `ts-jest` stays at 29.4.12, whose peer range is
already `jest: ^29.0.0 || ^30.0.0`.

No test needed changing. The only content this touches is the header line
of the three snapshot files, which jest 30 rewrites from the `goo.gl`
shortlink to the documentation URL; all 44 snapshots and every assertion
are unchanged.

`jest-resolve` 30 depends on `unrs-resolver`, which has a postinstall, and
`strictDepBuilds` correctly failed the install rather than skipping it.
That postinstall only fetches a native binding when the platform one is
missing, and all 22 bindings are pinned in the lockfile, so it resolves and
exits — denied rather than allowed, the same treatment `core-js` gets. On a
platform with no pinned binding the failure moves to require time; denying
the script does not repair that case, which is why the comment says so. The
image never sees any of it: jest is a devDependency and the runtime stage
installs `--prod`.

`.gitignore` picks up both places a JUnit report lands. `jest-junit`
defaults to `junit.xml` in the working directory, which is what a local
`pnpm ci:test` produces; `test-reports/` exists only because the CI action
sets `JEST_JUNIT_OUTPUT_DIR`. Neither was ignored.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@timdawborn

Copy link
Copy Markdown
Contributor Author

Automated review, round 2 — /pr-review-toolkit:review-pr

Agent: code-reviewer. Scoped to the revised commit e72f05e.

🔴 Critical (0) · 🟡 Important (1)

AGENTS.md:17 — a doc line this commit makes more wrong, not less.

pnpm ci:test - Run tests in CI mode with coverage reporting

Two false claims. Nothing invokes it — CI runs pnpm test --ci --reporters=default --reporters=jest-junit --shard=… directly. And neither jest config sets collectCoverage, so there is no coverage reporting anywhere.

This commit bumps jest-junit and adds /junit.xml specifically to serve that dead path, which makes the stale line more prominent rather than less. It is already drifting: jest.ci.config.js never received the testMatch restriction jest.config.js carries.

Either delete ci:test + jest.ci.config.js (and the eslint.config.js:22 ignore entry and this doc line), or correct the line. I'd lean delete — I added /junit.xml to catch an artifact only a dead script produces.

🔵 Suggestions (1)

.gitignore:8/junit.xml misses junit-<uuid>.xml, which jest-junit produces when JEST_JUNIT_UNIQUE_OUTPUT_NAME is set without a directory. CI sets both, so it is harmless today; /junit*.xml closes it.

✅ Verified sound

  • Both round-1 fixes work. Ran pnpm ci:test on one spec under jest 30.4.2: junit.xml appeared at the repo root and git status --porcelain came back empty.
  • The anchor is right. jest-junit@17 defaults outputDirectory: process.cwd() and overrides it only from a jest-junit key in the nearest package.json (absent — confirmed undefined) or from JEST_JUNIT_OUTPUT_DIR. pnpm run executes with cwd at the package root, so root is the only place it can land; the env path is CI-only and covered by /test-reports.
  • The rewritten allowBuilds comment is accurate on every claim. jest-resolve@30.4.1unrs-resolver@1.12.2 confirms "reached through jest-resolve". unrs-resolver declares exactly 22 optionalDependencies and 22 unique @unrs/resolver-binding-* appear in the lockfile. And napi-postinstall@0.3.4/lib/index.js:176-187 confirms the mechanism: for each target present it require.resolves and breaks, fetching only on resolve failure — so the "denying does not self-heal an unsupported platform" caveat holds.
  • Snapshot movement is complete and exactly as described — only 3 .snap files exist, all 3 are in the diff, header line only, 44 exports[ total.

Recommended action

Decide on ci:test: delete the script, its config, its eslint ignore and the doc line — or correct AGENTS.md:17. Everything else here is done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant