chore: migrate the test harness to Node 22 and cut CI over - #2375
Draft
aryanku-dev wants to merge 8 commits into
Draft
chore: migrate the test harness to Node 22 and cut CI over#2375aryanku-dev wants to merge 8 commits into
aryanku-dev wants to merge 8 commits into
Conversation
Groundwork for running the suite on Node 20. Neither change touches product code; both keep passing on Node 14. - client/proxy: `Invalid URL` TypeError no longer carries the offending input on Node >=18, so the exact-string assertion only held on 14. Match the stable prefix instead. - sdk-utils/request: Node >=20 detects the module syntax in this file and parses it as ESM, where `__dirname` is undefined. Derive the directory from `import.meta.url` under a distinct name so it is also safe if the file is ever loaded as CJS. Verified locally: @percy/client is 266/266 on both 14 and 20 (modulo one pre-existing failure caused by a system-level proxy on the test machine, which fails identically on both).
Replaces the getFormat/getSource/transformSource trio -- hooks Node removed in 16.12, which had been silently uncalled ever since -- with a single synchronous `load`, registered via `module.registerHooks` (Node >=22.15) from a new scripts/loader-register.js loaded with `--import`. Why registerHooks and not register(): the suite's mocking shares `global.__MOCK_IMPORTS__` between hooks and specs. `--loader`/`register()` run hooks on a separate module thread from Node 20.6 on, so that global is not shared. registerHooks runs them in-process in the same realm, which is what the Node 14 setup effectively had. Verified on Node 22.22.2 (was failing on Node 20): - @percy/sdk-utils 9 failed -> 169/169. Needed no spec changes: restoring the Babel step transpiles this (untyped) package back to CommonJS, so `proxyModule.proxyAgentFor = ...` is writable again instead of hitting a frozen ESM namespace. The `__dirname` workaround is reverted here for the same reason. - @percy/core Install 21 failed -> 21/21, and non-vacuously: the specs assert on the mocked spy (install.test.js:147), so the loader mock is demonstrably applied. - build 18/18, @percy/env, @percy/logger, @percy/client unchanged. NOT DONE -- @percy/config regresses 82/82 -> 13 failed, all `PercyConfig .load()`, failing as "Config file not found": the memfs volume is not visible to the config loader under the new hooks. Confirmed a real regression, not environmental (82/82 on Node 14 with the old loader in this same tree). Three hypotheses tested and ruled out: transforming memfs-backed sources, gating interception to import-only via context.conditions, and reading source through the unspied fs binding. Also open: the karma/browser half of sdk-utils errors with "Cannot read properties of undefined (reading 'stderr')", and the full core suite has not been re-measured on 22.
mockfs()'s bypass predicates are all written against strings
(p.includes('node_modules'), p.match(INTERNAL_FILE_REG)), but fs accepts a
path as a string, a Buffer or a file: URL. On a URL those matchers evaluate
to undefined, so the bypass silently fails and the read is served from the
in-memory volume instead of the real filesystem.
This surfaces from Node 22: module.registerHooks intercepts require() as well
as import, and Node reads CommonJS sources through the public fs using a URL.
The first lazy require() of a real dependency inside a mockfs block --
cosmiconfig requiring js-yaml to parse a config file -- threw ENOENT, which
search() swallowed, so all 13 PercyConfig .load() specs reported
"Config file not found".
Normalize the first argument before matching. Verified 82/82 on Node 22 with
the ported loader and 82/82 on Node 14 with the original loader.
…ocesses Two spots still forked Node with `--loader=../../scripts/loader.js`, which the migration replaced everywhere else. `--loader` runs hooks on a dedicated module thread from Node 20.6 onward, where `nextLoad` returns a promise the ported `load` hook does not await, and the mock registry cannot be shared. Point both at scripts/loader-register.js via --import, matching scripts/test.js.
Silent-mock audit (plan Phase 2). Of the four sites that register loader mocks, install.test.js, snapshot.test.js and cli/test/helpers.js already assert against the mocked double, so a mock that fails to apply fails the spec. The cross-spawn site did not: spawning the nonexistent `foobar` rejects with the real cross-spawn too, so the spec passed either way. Make it a spy and assert it was called. Also stop substituting a plain Map when global.__MOCK_IMPORTS__ is missing. The loader does not consult that Map, so the fallback turned "the loader is not registered" into a green run with every mock silently disabled. Fail loudly instead. Separately, the ci-log assertion indexed [0] of the captured stderr chunks. Each 'data' event is its own entry, so the position was never guaranteed; Node 22 emits an [UNDICI-EHPA] experimental warning that now takes that slot. Assert over the joined messages. @percy/cli-exec: 73/73 on Node 22.
Registering a `load` hook makes Node 22 translate CommonJS imported from ESM through the ESM pipeline instead of the classic CJS loader. The `require` that pipeline builds carries only `main` and `resolve` -- `cache` and `extensions` are missing. Reproduced with a bare pass-through `load` hook (so it is not caused by anything this loader does) and confirmed fixed in Node 24. The hook cannot decline to supply CommonJS source: Node rejects that with ERR_INVALID_RETURN_PROPERTY_VALUE. So patch the two properties back from inside the module, gated on the source actually referencing them. In practice that is `import-fresh`, which cosmiconfig requires to read .percy.js config files; it dereferences `require.cache[filePath]` and threw "Cannot read properties of undefined". @percy/config swallowed the TypeError in search(), so a .percy.js config silently loaded as empty. @percy/cli-snapshot: 28/28 on Node 22 (was 1 failed, passing on Node 14).
The proxy double ends the client socket right after writing each response, but
sent no Connection: close header. Node enables keepAlive on http.globalAgent by
default from Node 19, so the client pooled that socket and reused it for the
next request, which then failed instantly with ECONNRESET ("socket hang up").
Surfaced as "probeUrl via proxy succeeds when correct credentials are supplied
in proxy URL" failing on Node 22 only in full-suite order — it reused the socket
the preceding 407 spec had left closed, and passed in isolation.
@percy/cli-doctor: 505/508 on Node 22, matching the Node 14 full-suite baseline
exactly. The 3 remaining are pre-existing and environmental (a system proxy on
the host and leaked PERCY_* env vars), failing identically on Node 14.
Cut over rather than adding 22 alongside 14: scripts/loader-register.js throws
below Node 22.15 by design, so a [14, 22] matrix would be red by construction.
- All node-version pins and the test matrix go 14 -> 22. release.yml and
version-bump.yml stay on 24, unchanged by this migration.
- Rotate the caches. The keys embed the version, so the node-14/ fragments are
now node-22/, and .github/.cache-key is bumped (its content hash is the key)
so no Node 14 node_modules can be restored onto a Node 22 runner.
- Replace the archived vercel/pkg with @yao-pkg/pkg@6.22.0. pkg 5.8.1 was the
final release of an archived project and has no Node 22 base binary, so the
standalone executable could not be built on 22 at all. The fork's pkg-fetch
v3.6 ships prebuilt Node 22 binaries for linux, macos and win.
- Pin pkg --targets explicitly. Unpinned, pkg infers targets from the host, so
the arm64 macOS runner would silently start emitting an arm64 percy-osx.
Verified locally: node22-{linux,macos,win}-x64 all build, produce the same
run-linux/run-macos/run-win.exe names the script renames, and the macOS binary
reports v22.23.2 darwin x64.
grep -rn "node-14\|node-version: 14" .github/ is now empty.
|
|
||
| while (dir.startsWith(ROOT)) { | ||
| if (typeCache.has(dir)) return typeCache.get(dir); | ||
| let pkg = path.join(dir, 'package.json'); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Migrates the test harness from Node 14 to Node 22 and cuts CI over.
@percy/cliwas not on Node 14 by preference —scripts/loader.jsexported threemodule hooks (
getFormat,getSource,transformSource) that Node removed in16.12. On any Node ≥16.12 they were never called, silently disabling both the Babel
transform and the module-mocking layer the suite depends on. That is why all 21
node-version:pins said14.The port replaces them with a single synchronous
loadregistered throughmodule.registerHooks(Node ≥22.15), which runs in-process and in the same realm —the behaviour
--experimental-loaderhad on Node 14, and the reason this targets 22rather than 20 (Node 20 has only the off-thread
module.register).This is a test-harness migration. No product behaviour changes. The only
non-test source files touched are
packages/core/post-install.js(a dev-only forkthat still passed the removed
--loaderflag) andscripts/executable.sh.Why this is a clean cut, not a matrix
scripts/loader-register.jsthrows below Node 22.15 by design, so anode: [14, 22]matrix would be red by construction. The 21 pins move together.
Root causes fixed
Four defects, each reduced to a root cause and verified against a Node 14 baseline
rather than guessed at:
@percy/config— 13 specs, "Config file not found".mockfs()'s bypasspredicates are written against strings (
p.includes('node_modules')), butfsalso accepts a
URL.registerHooksinterceptsrequire()(which--experimental-loadernever did) and Node reads CommonJS sources with afile:URL, so the bypass silently returned falsy and the read was served from the
in-memory volume. cosmiconfig's lazy
require('js-yaml')threw ENOENT,search()swallowed it, and every config file read as empty.
require.cacheisundefinedon Node 22 for CommonJS reached through ESMinterop, whenever any
loadhook is registered. Reproduced with a barepass-through hook, so it is not caused by this loader — and it is fixed in Node 24.
The hook cannot decline to supply CommonJS source (Node rejects that with
ERR_INVALID_RETURN_PROPERTY_VALUE), so the two properties are patched back frominside the module, gated on the source actually referencing them. In practice that
is
import-fresh, which cosmiconfig uses to read.percy.jsconfigs.http.globalAgentenables keepAlive from Node 19.cli-doctor's proxy doubleended each client socket without sending
Connection: close, so the client pooledthe dead socket and the next request failed with
ECONNRESET. Surfaced only infull-suite order; passed in isolation.
Two stale
--loadercall sites (core/post-install.js, acli-execspec)moved to
--import. Under--loaderthe hooks run off-thread, wherenextLoadreturns a promise the ported
loaddoes not await.Silent-mock audit (the migration's real risk)
This layer fails by not mocking while still reporting green, so a green suite is not
evidence of a correct migration. All four sites that register loader mocks were audited
for an assertion against the mocked double:
core/test/unit/install.test.js:147core/test/snapshot.test.js(runDiagnostics)cli/test/helpers.js→commands.test.js:212cli-exec/test/exec.test.js(cross-spawn)The
cross-spawnspec spawned a nonexistentfoobar, which rejects with the realcross-spawntoo, so it passed whether or not the mock applied. It is now a spy withexpect(crossSpawn).toHaveBeenCalled().Also removed
global.__MOCK_IMPORTS__ = global.__MOCK_IMPORTS__ || new Map(). Theloader does not consult that Map, so the fallback converted "the loader is not
registered" into a green run with every mock silently disabled. It now throws.
Executable build
executable.shrannpm install -g pkg, whose latest release (5.8.1) is the finalrelease of the archived vercel/pkg and ships no Node 22 base binary — the
standalone
percybinary could not be built on 22 at all. Replaced with@yao-pkg/pkg@6.22.0; itspkg-fetchv3.6 provides prebuilt Node 22 binaries forlinux, macos and win on x64 and arm64.
--targetsis now pinned explicitly. Unpinned, pkg infers targets from the host, sothe arm64 macOS runner would have silently started emitting an arm64
percy-osx—a change to what customers download. The published matrix stays x64; whether to add
arm64 assets is a separate release decision.
Testing
Every failure was attributed with the repo's own
CLI_TEST_FAILURES_FILE+CLI_TEST_ONLY_FAILEDmechanism: record failures on Node 22, replay those exactspecs on Node 14 with the original loader. Failures present on both are pre-existing.
env/logger/webdriver-utilsconfigcli-snapshotcli-execcli/cli-app/cli-build/cli-command/cli-configcli-upload/monitoringsdk-utilsdomclientcli-doctorcoreLint and typecheck both pass on Node 22.
Not verified locally — needs this PR's CI
corespec:DiskSpillStore … destroy removes the entire dir and clears index.Fails only in full-suite order on Node 22 and passes in isolation, so the
replay-the-failures protocol cannot attribute it — a subset run is exactly the
condition under which it passes. A full Node 14 baseline is running; the plan never
completed one (§5.1 records it as "timed out >25 min"), so no prior baseline exists
for this spec.
destroy()swallows its own errors, and the symptom (existsSync(dir)still true) is consistent with
#readybeing false, i.e. leakedfsspies from anearlier spec rather than anything version-specific.
registerHooksreceivesfile:///C:/…URLs;specifier.startsWith(ROOT)mixes URLand path forms and is the likely failure point.
envproved it on 22 pre-PR; per-packagetest:coveragestill to run. Instrumentation happens inside the loader's Babelstep, so anything the
onlyfilter or the?__mock__short-circuit skips losescoverage.
type: modulestripping, Apple codesign/notarize, andreseditonwindows-2022need secrets and runners. Locally verified only that@yao-pkg/pkgbuilds all three targets with the same output filenames and that themacOS binary reports
v22.23.2 darwin x64.Two local failures are environmental, not regressions: a system proxy on the host
(
127.0.0.1:*) and a missing local Firefox (karma-firefox-launchercrashes withCannot read properties of undefined (reading 'stderr')— CI runners have Firefox).Post-Deploy Monitoring & Validation
No runtime/production impact from the harness change itself — it touches
scripts/, tests, and CI only. The executable change does ship an artifact, so:test.yml(all 18 packages),windows.yml,lint.yml,typecheck.yml,executable.yml,executable-check.ymlon this PR.::warning title=Flaky testsannotations — a package that goes green only viaa spec-level retry is not actually green (PER-9011).
percy-linux.zip,percy-osx.zip,percy.exesize and arch.grep -rn "node-14\|node-version: 14" .github/→ must return nothing../scripts/verify-executable.sh ./percygreen on all three platforms.file percy-osx→ must still reportMach-O 64-bit executable x86_64, notarm64. An arch flip is the highest-consequence silent failure here.
percy --versionfrom each built binary.nyc report --check-coverage→ 100/100/100/100 per package.once (keys rotated, by design) then hit.
retry1–retry4→ treat as a real failure; a mock thatsilently stopped applying looks exactly like flake.
executable.shchange and keepthe executable pipeline on its own older Node (plan §7.1 option c) while the rest
of CI stays on 22.
that publishes executables. Owner: @AryanKumar.
Follow-ups (deliberately not in this PR)
engineson 17 packages>=14→>=22, andbabel.config.cjstargets: {node:'14'}(lines 30, 39) which shape published
dist. Both raise the customer floor and mustfollow a support-policy decision, not lead it.
@percy/domdeclares noenginesat all.packages/cli-doctorwrites a straynulfile during tests (a Windows device name);pre-existing, unrelated.
🤖 Generated with Claude Opus 5 (1M context, extended thinking) via Claude Code