From 9c60ed696b5ef0deb51d7fe478de581e7f8fa61e Mon Sep 17 00:00:00 2001 From: Aryan Kumar Date: Fri, 7 Aug 2026 19:11:32 +0530 Subject: [PATCH 1/8] test(client,sdk-utils): decouple two specs from the Node version 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). --- packages/client/test/unit/proxy.test.js | 5 ++++- packages/sdk-utils/test/request.test.js | 12 ++++++++++-- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/packages/client/test/unit/proxy.test.js b/packages/client/test/unit/proxy.test.js index 7bc93a695..e5f96fbb5 100644 --- a/packages/client/test/unit/proxy.test.js +++ b/packages/client/test/unit/proxy.test.js @@ -86,7 +86,10 @@ describe('proxy', () => { const url = 'http://example.com'; const options = {}; process.env.PERCY_PAC_FILE_URL = 'invalid-url'; - expect(() => proxyAgentFor(url, options)).toThrowError('Failed to initialize PAC proxy: Invalid URL: invalid-url'); + // Matched as a prefix: Node <18 appends the offending input to the + // `Invalid URL` TypeError ("Invalid URL: invalid-url"), Node >=18 does + // not. Asserting the full string pins this test to one Node version. + expect(() => proxyAgentFor(url, options)).toThrowError(/^Failed to initialize PAC proxy: Invalid URL/); }); }); }); diff --git a/packages/sdk-utils/test/request.test.js b/packages/sdk-utils/test/request.test.js index 4bdae7496..dfe95e2b4 100644 --- a/packages/sdk-utils/test/request.test.js +++ b/packages/sdk-utils/test/request.test.js @@ -3,11 +3,19 @@ import https from 'https'; import http from 'http'; import fs from 'fs'; import path from 'path'; +import { fileURLToPath } from 'url'; + +// Resolve fixtures relative to this file. `__dirname` is unavailable here: +// Node >=20 detects the module syntax below and (re)parses this file as ESM, +// where `__dirname` is not defined. Deriving it from `import.meta.url` works +// on every supported Node version, and avoids redeclaring `__dirname` in the +// event the file is ever loaded as CJS. +const testDir = path.dirname(fileURLToPath(import.meta.url)); // NOTE: Although sdk-utils test run in browser as well, we do not run sdk-utils/request test in browsers as we require creation of https server for this test const ssl = { - cert: fs.readFileSync(path.join(__dirname, 'assets', 'certs', 'test.crt')), - key: fs.readFileSync(path.join(__dirname, 'assets', 'certs', 'test.key')) + cert: fs.readFileSync(path.join(testDir, 'assets', 'certs', 'test.crt')), + key: fs.readFileSync(path.join(testDir, 'assets', 'certs', 'test.key')) }; // Returns the port number of a URL object. Defaults to port 443 for https From f5714715b27a0152f06d9ab475d2e8940d7d6ac0 Mon Sep 17 00:00:00 2001 From: Aryan Kumar Date: Fri, 7 Aug 2026 21:30:25 +0530 Subject: [PATCH 2/8] WIP test(loader): port module hooks to registerHooks for Node 22 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. --- packages/sdk-utils/test/request.test.js | 12 +- scripts/loader-register.js | 25 ++++ scripts/loader.js | 160 +++++++++++++++++++----- scripts/test.js | 7 +- 4 files changed, 162 insertions(+), 42 deletions(-) create mode 100644 scripts/loader-register.js diff --git a/packages/sdk-utils/test/request.test.js b/packages/sdk-utils/test/request.test.js index dfe95e2b4..4bdae7496 100644 --- a/packages/sdk-utils/test/request.test.js +++ b/packages/sdk-utils/test/request.test.js @@ -3,19 +3,11 @@ import https from 'https'; import http from 'http'; import fs from 'fs'; import path from 'path'; -import { fileURLToPath } from 'url'; - -// Resolve fixtures relative to this file. `__dirname` is unavailable here: -// Node >=20 detects the module syntax below and (re)parses this file as ESM, -// where `__dirname` is not defined. Deriving it from `import.meta.url` works -// on every supported Node version, and avoids redeclaring `__dirname` in the -// event the file is ever loaded as CJS. -const testDir = path.dirname(fileURLToPath(import.meta.url)); // NOTE: Although sdk-utils test run in browser as well, we do not run sdk-utils/request test in browsers as we require creation of https server for this test const ssl = { - cert: fs.readFileSync(path.join(testDir, 'assets', 'certs', 'test.crt')), - key: fs.readFileSync(path.join(testDir, 'assets', 'certs', 'test.key')) + cert: fs.readFileSync(path.join(__dirname, 'assets', 'certs', 'test.crt')), + key: fs.readFileSync(path.join(__dirname, 'assets', 'certs', 'test.key')) }; // Returns the port number of a URL object. Defaults to port 443 for https diff --git a/scripts/loader-register.js b/scripts/loader-register.js new file mode 100644 index 000000000..b7369b383 --- /dev/null +++ b/scripts/loader-register.js @@ -0,0 +1,25 @@ +// Registers the test loader's module customization hooks. +// +// Loaded with `node --import` (see scripts/test.js) rather than +// `--loader scripts/loader.js`, because `--loader` runs its hooks on a +// dedicated module thread from Node 20.6 onward. The suite's mocking works by +// sharing `global.__MOCK_IMPORTS__` between the hooks and the specs, which a +// separate thread cannot do -- tests would see `undefined` and every +// `__MOCK_IMPORTS__.set()` would throw. +// +// `module.registerHooks` (Node >=22.15) runs the hooks synchronously, in-process +// and in the same realm, which is the behaviour the old `--experimental-loader` +// had on Node 14. That is why this migration targets 22 and not 20: Node 20 only +// has the off-thread `module.register`, so it would need the mock registry moved +// onto a MessagePort with an ack before every dynamic import. +import { registerHooks } from 'module'; +import { resolve, load } from './loader.js'; + +if (typeof registerHooks !== 'function') { + throw new Error( + 'The test loader requires module.registerHooks (Node >=22.15). ' + + `Running Node ${process.version}.` + ); +} + +registerHooks({ resolve, load }); diff --git a/scripts/loader.js b/scripts/loader.js index 136d17bb6..e1a0fe6d0 100644 --- a/scripts/loader.js +++ b/scripts/loader.js @@ -36,21 +36,36 @@ export const LOADER_ALIAS = { }; // resolve specifier file url -export async function resolve(specifier, context, defaultResolve) { +export function resolve(specifier, context, nextResolve) { + // `module.registerHooks` intercepts require() as well as import, which the + // old --experimental-loader did not. Mock interception has to stay + // import-only: applying it to require() redirects internal CommonJS requires + // into the memfs volume and breaks fs mocking (@percy/config). + let isRequire = context.conditions?.includes('require'); + // check for import or filesystem mocks - if (MOCK_IMPORTS.has(specifier)) { - return { url: `mock://${specifier}?__mock__=${MOCK_IMPORTS.__uid__}&module` }; - } else if (context.parentURL && '$vol' in fs) { + if (!isRequire && MOCK_IMPORTS.has(specifier)) { + return { + url: `mock://${specifier}?__mock__=${MOCK_IMPORTS.__uid__}&module`, + shortCircuit: true + }; + } else if (!isRequire && context.parentURL && '$vol' in fs) { let filename = specifier.startsWith('file:') ? url.fileURLToPath(specifier) : specifier; let filepath = path.resolve(path.dirname(url.fileURLToPath(context.parentURL)), filename); if (fs.$vol.existsSync(filepath)) { let fmt = CJS_REG.test(fs.$vol.readFileSync(filepath)) ? 'commonjs' : 'module'; - return { url: `${url.pathToFileURL(filepath)}?__mock__=${MOCK_IMPORTS.__uid__}&${fmt}` }; + + return { + url: `${url.pathToFileURL(filepath)}?__mock__=${MOCK_IMPORTS.__uid__}&${fmt}`, + shortCircuit: true + }; } } // rewrite dist to src in development + let original = specifier; + if (specifier.startsWith('#')) { let pkgRoot = url.fileURLToPath(context.parentURL.replace(/(packages\/[^/]+\/).+$/, '$1')); let pkgJSON = JSON.parse(fs.readFileSync(path.resolve(pkgRoot, 'package.json'))); @@ -60,18 +75,16 @@ export async function resolve(specifier, context, defaultResolve) { specifier = specifier.replace(LOADER_ALIAS.find, LOADER_ALIAS.replace); } - // transform absolute filepaths into absolute file urls - if (specifier.startsWith(ROOT)) specifier = url.pathToFileURL(specifier).href; + // Transform absolute filepaths into absolute file urls, but only for + // specifiers we actually rewrote. `module.registerHooks` also intercepts + // `require()`, whose resolver rejects a file: URL -- converting every + // in-repo path unconditionally broke requires like babel.config.cjs. + if (specifier !== original && specifier.startsWith(ROOT)) { + specifier = url.pathToFileURL(specifier).href; + } // use default resolve when not mocked - return defaultResolve(specifier, context, defaultResolve); -} - -// get module format for loader mocks -export async function getFormat(srcURL, context, defaultGetFormat) { - return srcURL.includes('?__mock__') - ? { format: srcURL.split('?')[1].split('&')[1] } - : defaultGetFormat(srcURL, context, defaultGetFormat); + return nextResolve(specifier, context); } // generate mock sources for mocked modules @@ -87,26 +100,113 @@ function mockSource(mockURL) { } } -// return loader mocks as module sources -export async function getSource(srcURL, context, defaultGetSource) { - if (srcURL.includes('?__mock__')) return { source: mockSource(srcURL) }; - return defaultGetSource(srcURL, context, defaultGetSource); +// Nearest package.json `type`, mirroring how babel.config.cjs decides between +// its `modules: false` and `modules: 'commonjs'` overrides. The format we hand +// back to Node has to agree with what Babel actually emitted, so it is derived +// the same way rather than sniffed off the output. +const typeCache = new Map(); + +function packageType(filename) { + let dir = path.dirname(filename); + + while (dir.startsWith(ROOT)) { + if (typeCache.has(dir)) return typeCache.get(dir); + let pkg = path.join(dir, 'package.json'); + + if (fs.existsSync(pkg)) { + let type = JSON.parse(fs.readFileSync(pkg, 'utf8')).type === 'module' + ? 'module' : 'commonjs'; + typeCache.set(dir, type); + return type; + } + + let parent = path.dirname(dir); + if (parent === dir) break; + dir = parent; + } + + return 'commonjs'; +} + +// Read a module's source directly, for the cases where Node does not hand +// `source` to the load hook (notably CommonJS). +// +// Deliberately uses the real filesystem via realpath-free readFileSync on the +// *unspied* binding: while a test has fs mocked, `fs.readFileSync` is a jasmine +// spy backed by memfs, so reading through it would serve in-memory content for +// ordinary source files. Memfs-backed modules are handled separately, via the +// `?__mock__` branch in load(). +const realReadFileSync = fs.readFileSync; + +function readSource(loadURL) { + return realReadFileSync(url.fileURLToPath(loadURL.split('?')[0]), 'utf8'); } -// return loader mocks or transform sources using babel -export async function transformSource(source, context, defaultTransformSource) { - let callback = (src = source) => defaultTransformSource(src, context, defaultTransformSource); - if (context.format !== 'module' && context.format !== 'commonjs') return callback(); - if (context.url.startsWith('mock://')) return callback(); +// Return loader mocks, or transform sources using babel. +// +// Replaces the getFormat/getSource/transformSource trio this file used to +// export. Those hooks were removed in Node 16.12 and silently stopped being +// called, which is what pinned the suite to Node 14: without the Babel step +// the test files load as native ESM (frozen namespaces, no `__dirname`), and +// without a shared realm the `__MOCK_IMPORTS__` interception never matched. +// A single synchronous `load` registered via `module.registerHooks` restores +// both behaviours. See scripts/loader-register.js. +export function load(loadURL, context, nextLoad) { + // synthesized mock module, or a memfs-backed file + if (loadURL.includes('?__mock__')) { + let format = loadURL.split('?')[1].split('&')[1]; + let source = mockSource(loadURL); + + // `mock://` modules are generated re-export shims and must be left alone, + // but memfs-backed files are real sources -- the old transformSource hook + // skipped only the former, so keep transforming the latter. + if (!loadURL.startsWith('mock://')) { + source = transform(source, url.fileURLToPath(loadURL.split('?')[0]), format) ?? source; + } + + return { format, source, shortCircuit: true }; + } + + let result = nextLoad(loadURL, context); + let format = result.format ?? context.format; + + // only our own src/test files get transformed + if (format !== 'module' && format !== 'commonjs') return result; + if (!loadURL.startsWith('file:')) return result; + + let filename = url.fileURLToPath(loadURL.split('?')[0]); + if (!BABEL_REG.test(filename)) return result; + + let source = result.source; + + if (source == null) { + try { source = readSource(loadURL); } catch { return result; } + } + + let transformed = transform(source, filename, format); + + // `only` misses turn into a null result -- keep Node's original module. + if (transformed == null) return result; + + return { + format: packageType(filename), + source: transformed, + shortCircuit: true + }; +} - if (typeof source !== 'string') source = Buffer.from(source); - if (Buffer.isBuffer(source)) source = source.toString(); +// Babel-transform a module source, or return null when Babel's `only` filter +// does not match it. `unambiguous` lets Babel classify the input itself; the +// old code forwarded Node's format, but Babel's sourceType has no 'commonjs' +// member, so a CommonJS file would have been mis-declared. +function transform(source, filename, format) { + if (typeof source !== 'string') source = Buffer.from(source).toString('utf8'); - return callback((await babel.transformAsync(source, { - filename: url.fileURLToPath(context.url), - sourceType: context.format, + return babel.transformSync(source, { + filename, + sourceType: 'unambiguous', babelrcRoots: ['.'], rootMode: 'upward', only: [BABEL_REG] - }))?.code); + })?.code ?? null; } diff --git a/scripts/test.js b/scripts/test.js index 64cde2f8a..eb30227bf 100644 --- a/scripts/test.js +++ b/scripts/test.js @@ -88,8 +88,11 @@ async function main({ } else if (!process.send) { // test runners assume they have control over the entire process, so give them each forks let flags = flagify({ coverage, karma: karmaArgs }); - let loader = url.pathToFileURL(path.resolve(filename, '../loader.js')).href; - let opts = { execArgv: ['--loader', loader, ...process.execArgv] }; + // --import (not --loader): the hooks must run in-process and share a realm + // with the specs so global.__MOCK_IMPORTS__ is the same object on both + // sides. See scripts/loader-register.js. + let loader = url.pathToFileURL(path.resolve(filename, '../loader-register.js')).href; + let opts = { execArgv: ['--import', loader, ...process.execArgv] }; if (testNode) { await child('fork', filename, ['--node', ...flags], opts); From 4d31c58d34386d3370ef636402d8d4a5f81654d9 Mon Sep 17 00:00:00 2001 From: Aryan Kumar Date: Sat, 8 Aug 2026 21:24:36 +0530 Subject: [PATCH 3/8] fix(config): match fs bypass on normalized paths, not raw arguments 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. --- packages/config/test/helpers.js | 33 ++++++++++++++++++++++++++++++--- 1 file changed, 30 insertions(+), 3 deletions(-) diff --git a/packages/config/test/helpers.js b/packages/config/test/helpers.js index a7bd40736..4a17363fc 100644 --- a/packages/config/test/helpers.js +++ b/packages/config/test/helpers.js @@ -31,6 +31,28 @@ const INTERNAL_FILE_REG = new RegExp( // Used to mock javascript modules const JS_FILE_REG = /\.(c|m)?js$/; +// Normalize an fs path argument before matching it against the bypass list. +// +// `fs` accepts a path as a string, a Buffer, a `file:` URL, or a file +// descriptor, but every bypass matcher below is written against a string +// (`p.includes('node_modules')`, `p.match(INTERNAL_FILE_REG)`). On a URL object +// those are `undefined`, so the matcher silently returns falsy and the read is +// routed into the in-memory volume instead of being let through. +// +// This matters from Node 22: `module.registerHooks` intercepts `require()` as +// well as `import` (the old `--experimental-loader` did not), and Node reads +// CommonJS sources through the public `fs` using a URL. Without this, the first +// lazy `require()` of a real dependency inside a mockfs block -- cosmiconfig +// requiring js-yaml to parse a config file -- throws ENOENT. +// +// File descriptors are numbers and are passed through untouched, since the +// descriptor matcher below tests them directly. +function bypassTarget(filepath) { + if (filepath instanceof URL) return url.fileURLToPath(filepath); + if (Buffer.isBuffer(filepath)) return filepath.toString('utf8'); + return filepath; +} + // Mock and spy on fs methods using an in-memory filesystem export async function mockfs({ // set `true` to allow mocking files within `node_modules` (may cause dynamic import issues) @@ -76,9 +98,14 @@ export async function mockfs({ let installFakes = (og, fake) => { for (let k in og) { if (k in fake && typeof og[k] === 'function' && !FS_CLASSES.includes(k)) { - spyOn(og, k).and.callFake((...args) => bypass.some(p => ( - typeof p === 'function' ? p(...args) : (p === args[0]) - )) ? og[k].and.originalFn(...args) : fake[k](...args)); + spyOn(og, k).and.callFake((...args) => { + let [filepath, ...rest] = args; + let target = bypassTarget(filepath); + + return bypass.some(p => ( + typeof p === 'function' ? p(target, ...rest) : (p === target) + )) ? og[k].and.originalFn(...args) : fake[k](...args); + }); } } }; From 4e06e46768a2c693b01d7ff3ad1ed3843b36e397 Mon Sep 17 00:00:00 2001 From: Aryan Kumar Date: Sat, 8 Aug 2026 21:38:38 +0530 Subject: [PATCH 4/8] test(core,cli-exec): register loader hooks with --import in forked processes 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. --- packages/cli-exec/test/exec.test.js | 2 +- packages/core/post-install.js | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/cli-exec/test/exec.test.js b/packages/cli-exec/test/exec.test.js index b29cac633..3c2742d7b 100644 --- a/packages/cli-exec/test/exec.test.js +++ b/packages/cli-exec/test/exec.test.js @@ -386,7 +386,7 @@ describe('percy exec', () => { }); it('provides the child process with a percy server address env var', async () => { - let args = ['--no-warnings', '--input-type=module', '--loader=../../scripts/loader.js']; + let args = ['--no-warnings', '--input-type=module', '--import=../../scripts/loader-register.js']; await exec(['--port=4567', '--', 'node', ...args, '--eval', [ 'import { request } from "../cli-command/src/utils.js";', diff --git a/packages/core/post-install.js b/packages/core/post-install.js index c1c491025..ab60d899b 100644 --- a/packages/core/post-install.js +++ b/packages/core/post-install.js @@ -7,7 +7,9 @@ try { } else if (!process.send && fs.existsSync('./src')) { // In development, fork this script with the development loader and always install await import('child_process').then(cp => cp.fork('./post-install.js', { - execArgv: ['--no-warnings', '--loader=../../scripts/loader.js'], + // --import (not --loader): the hooks are registered in-process via + // module.registerHooks. See scripts/loader-register.js. + execArgv: ['--no-warnings', '--import=../../scripts/loader-register.js'], env: { PERCY_POSTINSTALL_BROWSER: true } })); } From 425527db850f8ce13494c01d25805f10c4f243fd Mon Sep 17 00:00:00 2001 From: Aryan Kumar Date: Sat, 8 Aug 2026 22:05:17 +0530 Subject: [PATCH 5/8] test(cli-exec): make the loader mocks and ci-log assertion non-vacuous 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. --- packages/cli-exec/test/exec.test.js | 30 ++++++++++++++++++++++------- 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/packages/cli-exec/test/exec.test.js b/packages/cli-exec/test/exec.test.js index 3c2742d7b..e22eb05c3 100644 --- a/packages/cli-exec/test/exec.test.js +++ b/packages/cli-exec/test/exec.test.js @@ -17,8 +17,13 @@ describe('percy exec', () => { spyOn(process, 'exit').and.callFake(c => c); process.env.PERCY_CLIENT_ERROR_LOGS = false; - // Ensure global.__MOCK_IMPORTS__ is defined - global.__MOCK_IMPORTS__ = global.__MOCK_IMPORTS__ || new Map(); + // The loader defines this registry when scripts/loader-register.js is + // imported. Substituting a plain Map when it is missing would be worse than + // useless: the loader would not consult it, so every mock below would + // silently no-op while the suite still reported green. + if (!global.__MOCK_IMPORTS__) { + throw new Error('global.__MOCK_IMPORTS__ is undefined — the test loader is not registered'); + } }); afterEach(() => { @@ -275,9 +280,14 @@ describe('percy exec', () => { '[percy] Finalized build #1: https://percy.io/test/test/123' ])); - expect(logger.instance.query(log => log.debug === 'ci')[0].message).toContain([ - 'Some error with secret: [REDACTED]' - ]); + // Each 'data' event on the child's stderr becomes its own ci log entry, so + // indexing [0] assumes the error arrives in the very first chunk. Node 22 + // emits an "[UNDICI-EHPA] EnvHttpProxyAgent is experimental" warning ahead + // of it, which took that slot; stream chunking makes the position + // unreliable in general. Assert the redacted secret is captured somewhere. + expect(logger.instance.query(log => log.debug === 'ci') + .map(log => log.message).join('') + ).toContain('Some error with secret: [REDACTED]'); expect(stderrSpy).toHaveBeenCalled(); }); @@ -325,15 +335,21 @@ describe('percy exec', () => { it('throws when the command receives an error event and stops percy', async () => { let { default: EventEmitter } = await import('events'); let [e, err] = [new EventEmitter(), new Error('spawn error')]; - let crossSpawn = () => (setImmediate(() => e.emit('error', err)), e); + // A spy, not a bare function: spawning the nonexistent `foobar` below fails + // with the real cross-spawn too, so without an assertion on the double this + // spec would pass even when the loader mock never applied. + let crossSpawn = jasmine.createSpy('crossSpawn').and.callFake(() => ( + setImmediate(() => e.emit('error', err)), e + )); global.__MOCK_IMPORTS__.set('cross-spawn', { default: crossSpawn }); let stdinSpy = spyOn(process.stdin, 'pipe').and.resolveTo('some response'); await expectAsync(exec(['--', 'foobar'])).toBeRejected(); + // proves the loader mock actually applied (see §9.1 of the migration plan) + expect(crossSpawn).toHaveBeenCalled(); expect(stdinSpy).toHaveBeenCalled(); - console.log(logger.stderr); expect(logger.stderr).toEqual(jasmine.arrayContaining([ '[percy] Detected error for percy build', '[percy] Failure: Snapshot command was not called', From 323324ba1c75b9a4c6df29c076f8e648eaea17f5 Mon Sep 17 00:00:00 2001 From: Aryan Kumar Date: Sat, 8 Aug 2026 22:21:43 +0530 Subject: [PATCH 6/8] test(loader): restore require.cache for CommonJS reached via ESM interop 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). --- scripts/loader.js | 33 ++++++++++++++++++++++++++++++++- 1 file changed, 32 insertions(+), 1 deletion(-) diff --git a/scripts/loader.js b/scripts/loader.js index e1a0fe6d0..195de001d 100644 --- a/scripts/loader.js +++ b/scripts/loader.js @@ -128,6 +128,37 @@ function packageType(filename) { return 'commonjs'; } +// Restore `require.cache` / `require.extensions` on Node 22. +// +// Registering a `load` hook makes Node translate CommonJS reached through +// ESM interop (`import` of a CJS package) via the ESM pipeline rather than the +// classic CJS loader. The `require` that pipeline builds is missing `cache` and +// `extensions` -- it carries only `main` and `resolve`. Verified against a +// bare pass-through `load` hook, so it is not caused by anything this loader +// does, and it is fixed in Node 24. +// +// Nothing can be done from the hook itself: returning no `source` for CommonJS +// is rejected with ERR_INVALID_RETURN_PROPERTY_VALUE. So patch the two +// properties back from inside the module, and only for sources that actually +// reference them -- in practice `import-fresh`, which cosmiconfig requires to +// load .percy.js config files, and which throws +// "Cannot read properties of undefined" on `require.cache[filePath]`. +const REQUIRE_STATICS_REG = /require\.(cache|extensions)\b/; + +const REQUIRE_STATICS_PRELUDE = + 'if(!require.cache){const m=require("module");' + + 'require.cache=m._cache;require.extensions=m._extensions;}'; + +function restoreRequireStatics(result, format) { + if (format !== 'commonjs' || result.source == null) return result; + + let source = typeof result.source === 'string' + ? result.source : Buffer.from(result.source).toString('utf8'); + if (!REQUIRE_STATICS_REG.test(source)) return result; + + return { ...result, source: REQUIRE_STATICS_PRELUDE + source, shortCircuit: true }; +} + // Read a module's source directly, for the cases where Node does not hand // `source` to the load hook (notably CommonJS). // @@ -175,7 +206,7 @@ export function load(loadURL, context, nextLoad) { if (!loadURL.startsWith('file:')) return result; let filename = url.fileURLToPath(loadURL.split('?')[0]); - if (!BABEL_REG.test(filename)) return result; + if (!BABEL_REG.test(filename)) return restoreRequireStatics(result, format); let source = result.source; From d78f20ed06266d8e8b52080836e929ad10789296 Mon Sep 17 00:00:00 2001 From: Aryan Kumar Date: Sat, 8 Aug 2026 22:31:10 +0530 Subject: [PATCH 7/8] test(cli-doctor): send Connection: close from the proxy test double MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- packages/cli-doctor/test/helpers.js | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/packages/cli-doctor/test/helpers.js b/packages/cli-doctor/test/helpers.js index 8869d9d12..639a738ad 100644 --- a/packages/cli-doctor/test/helpers.js +++ b/packages/cli-doctor/test/helpers.js @@ -55,6 +55,11 @@ export function createHttpServer(handler) { * @param {'block'} [opts.mode] Return 502 for everything * @returns {Promise<{server: net.Server, url: string, port: number, close: function}>} */ +// Minimal proxy double. Every response below closes the socket immediately, so +// each one must say `Connection: close`. Node's http.globalAgent enables +// keepAlive by default from Node 19, and without that header the client pools +// the socket and reuses it for the next request — which then fails with +// ECONNRESET ("socket hang up") because the server already ended it. export function createProxyServer(opts = {}) { return new Promise((resolve, reject) => { const sockets = new Set(); @@ -78,7 +83,9 @@ export function createProxyServer(opts = {}) { // ── block mode ────────────────────────────────────────────────────── if (opts.mode === 'block') { - clientSocket.end('HTTP/1.1 502 Bad Gateway\r\nContent-Length: 0\r\n\r\n'); + clientSocket.end( + 'HTTP/1.1 502 Bad Gateway\r\nConnection: close\r\nContent-Length: 0\r\n\r\n' + ); return; } @@ -89,6 +96,7 @@ export function createProxyServer(opts = {}) { clientSocket.end( 'HTTP/1.1 407 Proxy Authentication Required\r\n' + 'Proxy-Authenticate: Basic realm="proxy"\r\n' + + 'Connection: close\r\n' + 'Content-Length: 0\r\n\r\n' ); return; @@ -97,7 +105,8 @@ export function createProxyServer(opts = {}) { const [user, pass] = decoded.split(':'); if (user !== opts.auth.user || pass !== opts.auth.pass) { clientSocket.end( - 'HTTP/1.1 407 Proxy Authentication Required\r\nContent-Length: 0\r\n\r\n' + 'HTTP/1.1 407 Proxy Authentication Required\r\n' + + 'Connection: close\r\nContent-Length: 0\r\n\r\n' ); return; } @@ -126,7 +135,9 @@ export function createProxyServer(opts = {}) { // ── Plain HTTP proxy ──────────────────────────────────────────────── // For non-CONNECT requests just return 200 (sufficient for our tests) - clientSocket.end('HTTP/1.1 200 OK\r\nContent-Length: 0\r\n\r\n'); + clientSocket.end( + 'HTTP/1.1 200 OK\r\nConnection: close\r\nContent-Length: 0\r\n\r\n' + ); }; clientSocket.on('data', onData); From a42603c0974fd13aae816e35a58f6e8f88453eb9 Mon Sep 17 00:00:00 2001 From: Aryan Kumar Date: Sat, 8 Aug 2026 22:46:04 +0530 Subject: [PATCH 8/8] ci: move the toolchain from Node 14 to Node 22 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. --- .github/.cache-key | 2 +- .github/workflows/executable-check.yml | 2 +- .github/workflows/executable.yml | 4 ++-- .github/workflows/lint.yml | 6 +++--- .github/workflows/test.yml | 14 +++++++------- .github/workflows/typecheck.yml | 6 +++--- .github/workflows/windows.yml | 12 ++++++------ scripts/executable.sh | 14 ++++++++++++-- 8 files changed, 35 insertions(+), 25 deletions(-) diff --git a/.github/.cache-key b/.github/.cache-key index d05221f2f..eb15e7070 100644 --- a/.github/.cache-key +++ b/.github/.cache-key @@ -6,5 +6,5 @@ ; ; / \ _____________/_ __ \_____________ -Times we have broken CI: 4 +Times we have broken CI: 5 Times Windows has broken CI: 99+ diff --git a/.github/workflows/executable-check.yml b/.github/workflows/executable-check.yml index 1b4719914..f6614d2fb 100644 --- a/.github/workflows/executable-check.yml +++ b/.github/workflows/executable-check.yml @@ -37,7 +37,7 @@ jobs: - uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0 - uses: actions/setup-node@39370e3970a6d050c480ffad4ff0ed4d3fdee5af # v4.1.0 with: - node-version: 14 + node-version: 22 architecture: x64 - name: Build executables (no signing, no upload) run: ./scripts/executable.sh diff --git a/.github/workflows/executable.yml b/.github/workflows/executable.yml index 939f1dd3f..05c2dd0cf 100644 --- a/.github/workflows/executable.yml +++ b/.github/workflows/executable.yml @@ -14,7 +14,7 @@ jobs: - uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0 - uses: actions/setup-node@39370e3970a6d050c480ffad4ff0ed4d3fdee5af # v4.1.0 with: - node-version: 14 + node-version: 22 architecture: x64 - run: ./scripts/executable.sh env: @@ -53,7 +53,7 @@ jobs: - name: Set up Node.js uses: actions/setup-node@3235b876344d2a9aa001b8d1453c930bba69e610 # v3.9.1 with: - node-version: 14 + node-version: 22 - name: Install resedit run: npm install resedit - name: Update exe metadata diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index c86ab011a..8421b6fa1 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -24,7 +24,7 @@ jobs: - uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0 - uses: actions/setup-node@3235b876344d2a9aa001b8d1453c930bba69e610 # v3.9.1 with: - node-version: 14 + node-version: 22 - uses: actions/cache@f4b3439a656ba812b8cb417d2d49f9c810103092 # v3.4.0 with: path: | @@ -32,11 +32,11 @@ jobs: packages/*/node_modules packages/core/.local-chromium key: > - ${{ runner.os }}/node-14/ + ${{ runner.os }}/node-22/ ${{ hashFiles('.github/.cache-key') }}/ ${{ hashFiles('**/yarn.lock') }} restore-keys: > - ${{ runner.os }}/node-14/ + ${{ runner.os }}/node-22/ ${{ hashFiles('.github/.cache-key') }}/ - run: yarn - run: yarn lint diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index ad7389b22..6a0d213db 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -25,7 +25,7 @@ jobs: - uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0 - uses: actions/setup-node@3235b876344d2a9aa001b8d1453c930bba69e610 # v3.9.1 with: - node-version: 14 + node-version: 22 - uses: actions/cache@f4b3439a656ba812b8cb417d2d49f9c810103092 # v3.4.0 with: path: | @@ -33,11 +33,11 @@ jobs: packages/*/node_modules packages/core/.local-chromium key: > - ${{ runner.os }}/node-14/ + ${{ runner.os }}/node-22/ ${{ hashFiles('.github/.cache-key') }}/ ${{ hashFiles('**/yarn.lock') }} restore-keys: > - ${{ runner.os }}/node-14/ + ${{ runner.os }}/node-22/ ${{ hashFiles('.github/.cache-key') }}/ - run: yarn - run: yarn build @@ -58,7 +58,7 @@ jobs: strategy: matrix: os: [ubuntu-latest] - node: [14] + node: [22] package: - '@percy/env' - '@percy/client' @@ -171,7 +171,7 @@ jobs: fetch-depth: 50 - uses: actions/setup-node@3235b876344d2a9aa001b8d1453c930bba69e610 # v3.9.1 with: - node-version: 14 + node-version: 22 - uses: actions/cache@f4b3439a656ba812b8cb417d2d49f9c810103092 # v3.4.0 with: path: | @@ -179,11 +179,11 @@ jobs: packages/*/node_modules packages/core/.local-chromium key: > - ${{ runner.os }}/node-14/ + ${{ runner.os }}/node-22/ ${{ hashFiles('.github/.cache-key') }}/ ${{ hashFiles('**/yarn.lock') }} restore-keys: > - ${{ runner.os }}/node-14/ + ${{ runner.os }}/node-22/ ${{ hashFiles('.github/.cache-key') }}/ - uses: actions/download-artifact@634f93cb2916e3fdff6788551b99b062d0335ce0 # v5.0.0 with: diff --git a/.github/workflows/typecheck.yml b/.github/workflows/typecheck.yml index 28284162e..9e07dabc2 100644 --- a/.github/workflows/typecheck.yml +++ b/.github/workflows/typecheck.yml @@ -24,7 +24,7 @@ jobs: - uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0 - uses: actions/setup-node@3235b876344d2a9aa001b8d1453c930bba69e610 # v3.9.1 with: - node-version: 14 + node-version: 22 - uses: actions/cache@f4b3439a656ba812b8cb417d2d49f9c810103092 # v3.4.0 with: path: | @@ -32,11 +32,11 @@ jobs: packages/*/node_modules packages/core/.local-chromium key: > - ${{ runner.os }}/node-14/ + ${{ runner.os }}/node-22/ ${{ hashFiles('.github/.cache-key') }}/ ${{ hashFiles('**/yarn.lock') }} restore-keys: > - ${{ runner.os }}/node-14/ + ${{ runner.os }}/node-22/ ${{ hashFiles('.github/.cache-key') }}/ - run: yarn - run: yarn test:types diff --git a/.github/workflows/windows.yml b/.github/workflows/windows.yml index ff9f836d2..f16b56148 100644 --- a/.github/workflows/windows.yml +++ b/.github/workflows/windows.yml @@ -24,7 +24,7 @@ jobs: - uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0 - uses: actions/setup-node@3235b876344d2a9aa001b8d1453c930bba69e610 # v3.9.1 with: - node-version: 14 + node-version: 22 - uses: actions/cache@f4b3439a656ba812b8cb417d2d49f9c810103092 # v3.4.0 with: path: | @@ -32,11 +32,11 @@ jobs: packages/*/node_modules packages/core/.local-chromium key: > - ${{ runner.os }}/node-14/ + ${{ runner.os }}/node-22/ ${{ hashFiles('.github/.cache-key') }}/ ${{ hashFiles('**/yarn.lock') }} restore-keys: > - ${{ runner.os }}/node-14/ + ${{ runner.os }}/node-22/ ${{ hashFiles('.github/.cache-key') }}/ - run: yarn - run: yarn build @@ -85,7 +85,7 @@ jobs: - uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0 - uses: actions/setup-node@3235b876344d2a9aa001b8d1453c930bba69e610 # v3.9.1 with: - node-version: 14 + node-version: 22 - uses: actions/cache@f4b3439a656ba812b8cb417d2d49f9c810103092 # v3.4.0 with: path: | @@ -93,11 +93,11 @@ jobs: packages/*/node_modules packages/core/.local-chromium key: > - ${{ runner.os }}/node-14/ + ${{ runner.os }}/node-22/ ${{ hashFiles('.github/.cache-key') }}/ ${{ hashFiles('**/yarn.lock') }} restore-keys: > - ${{ runner.os }}/node-14/ + ${{ runner.os }}/node-22/ ${{ hashFiles('.github/.cache-key') }}/ - uses: actions/download-artifact@634f93cb2916e3fdff6788551b99b062d0335ce0 # v5.0.0 with: diff --git a/scripts/executable.sh b/scripts/executable.sh index a195ac8f0..22535f938 100755 --- a/scripts/executable.sh +++ b/scripts/executable.sh @@ -8,7 +8,11 @@ function cleanup { } brew install gnu-sed -npm install -g pkg +# vercel/pkg is archived; its final release (5.8.1) ships no Node 22 base +# binary, so it cannot build this CLI once the toolchain moves off Node 14. +# @yao-pkg/pkg is the maintained fork; its pkg-fetch v3.6 provides prebuilt +# Node 22 binaries for linux, macos and win on both x64 and arm64. +npm install -g @yao-pkg/pkg@6.22.0 yarn install yarn build @@ -47,7 +51,13 @@ cp -R ./build/* packages/ # Create executables. (No `-d`/`--debug`: it only adds per-file "included as # DISCLOSED code / asset content" logging — thousands of lines — without # changing the output binaries.) -pkg ./packages/cli/bin/run.js +# +# Targets are pinned explicitly. Unpinned, pkg infers them from the host, so a +# macOS arm64 runner would silently start emitting an arm64 `percy-osx` — a +# change to what customers download, which is not this migration's call to make. +# Keep the published matrix at x64 and move only the embedded Node to 22; +# whether to add arm64 assets is a separate release decision. +pkg --targets node22-linux-x64,node22-macos-x64,node22-win-x64 ./packages/cli/bin/run.js # Rename executables mv run-linux percy && chmod +x percy