diff --git a/packages/testing/janitor/src/cli.ts b/packages/testing/janitor/src/cli.ts index b4c41e4e393..365ebe245a7 100644 --- a/packages/testing/janitor/src/cli.ts +++ b/packages/testing/janitor/src/cli.ts @@ -22,6 +22,7 @@ import { distributeShards, selectTests, changedRuntimeDepsFromManifests, + changedOverrideTargets, } from '@n8n/test-impact'; import * as fs from 'node:fs'; import * as path from 'node:path'; @@ -78,7 +79,7 @@ import { formatMethodUsageIndexJSON, } from './core/method-usage-analyzer.js'; import { createProject } from './core/project-loader.js'; -import { readLockfileImporters } from './core/read-lockfile-importers.js'; +import { readLockfileImporters, readRuntimeClosure } from './core/read-lockfile-importers.js'; import { readManifestDiffs, readTsconfigDiffs } from './core/read-manifest-diffs.js'; import { toJSON, toConsole } from './core/reporter.js'; import { filterToFailedSpecs } from './core/retry-filter.js'; @@ -726,10 +727,18 @@ function runSelect(options: CliOptions): void { // Only parse the (large) lockfile when a RUNTIME dependency actually changed — // the only case the dep-graph selector (389) acts on. A devDep-only manifest // change would parse it for nothing. - const lockfileImporters = - manifests && changedRuntimeDepsFromManifests(manifests).length > 0 - ? readLockfileImporters() - : undefined; + const runtimeDepsChanged = manifests + ? changedRuntimeDepsFromManifests(manifests).length > 0 + : false; + const lockfileImporters = runtimeDepsChanged ? readLockfileImporters() : undefined; + // The closure classifies override pins; a runtime-dep change already forces + // broad, so don't compute it then. + const overridesChanged = + !runtimeDepsChanged && + Object.values(manifests ?? {}).some(({ before, after }) => { + const targets = changedOverrideTargets(before, after); + return targets === null || targets.length > 0; + }); const result = selectTests({ changedFiles, mapFile: options.mapFile, @@ -737,6 +746,7 @@ function runSelect(options: CliOptions): void { manifests, tsconfigs, lockfileImporters, + runtimeClosure: overridesChanged ? readRuntimeClosure() : undefined, }); console.log(JSON.stringify(result)); } diff --git a/packages/testing/janitor/src/core/read-lockfile-importers.ts b/packages/testing/janitor/src/core/read-lockfile-importers.ts index f5d9869512c..18528f12394 100644 --- a/packages/testing/janitor/src/core/read-lockfile-importers.ts +++ b/packages/testing/janitor/src/core/read-lockfile-importers.ts @@ -8,27 +8,44 @@ * lockfile) returns `{}`, which makes the dep-graph selector contribute nothing * (the change then resolves through the coverage map alone — fail-open). */ -import { RUNTIME_SECTIONS } from '@n8n/test-impact'; +import { + RUNTIME_SECTIONS, + runtimeClosure, + type LockfileImporters, + type LockfileSnapshots, +} from '@n8n/test-impact'; import { existsSync, readFileSync } from 'node:fs'; import { join } from 'node:path'; import { parse } from 'yaml'; import { getGitRoot } from '../utils/git-operations.js'; -type ImporterSections = Record | undefined>; +type Lockfile = { + importers?: LockfileImporters; + snapshots?: LockfileSnapshots; +}; -export function readLockfileImporters(): Record { +/** + * Closure seed: `packages/cli` is the `n8n` package the E2E container runs; + * its workspace `link:` edges cover the rest. See {@link runtimeClosure} for + * why this must not be "every importer". + */ +const DEPLOY_ROOTS = ['packages/cli'] as const; + +/** Parse the lockfile once, or `undefined` on any failure (fail-open). */ +function readLockfile(): Lockfile | undefined { const lockPath = join(getGitRoot(process.cwd()), 'pnpm-lock.yaml'); - if (!existsSync(lockPath)) return {}; - let doc: { importers?: Record }; + if (!existsSync(lockPath)) return undefined; try { - doc = parse(readFileSync(lockPath, 'utf8')) as typeof doc; + return parse(readFileSync(lockPath, 'utf8')) as Lockfile; } catch { - return {}; + return undefined; } - const importers = doc?.importers ?? {}; +} + +function importersFrom(doc: Lockfile): Record { const out: Record = {}; - for (const [dir, sections] of Object.entries(importers)) { + for (const [dir, sections] of Object.entries(doc.importers ?? {})) { const names = new Set(); for (const section of RUNTIME_SECTIONS) { const deps = sections?.[section]; @@ -38,3 +55,21 @@ export function readLockfileImporters(): Record { } return out; } + +export function readLockfileImporters(): Record { + const doc = readLockfile(); + return doc ? importersFrom(doc) : {}; +} + +/** + * Runtime closure for classifying `pnpm.overrides` changes. `undefined` on any + * read failure, which keeps them broad. + */ +export function readRuntimeClosure(): ReadonlySet | undefined { + const doc = readLockfile(); + if (!doc?.snapshots || !doc.importers) return undefined; + return runtimeClosure(doc.importers, doc.snapshots, { + deployRoots: DEPLOY_ROOTS, + runtimeSections: RUNTIME_SECTIONS, + }); +} diff --git a/packages/testing/test-impact/src/changes.test.ts b/packages/testing/test-impact/src/changes.test.ts index 5f2ad2195dd..d8d10979a28 100644 --- a/packages/testing/test-impact/src/changes.test.ts +++ b/packages/testing/test-impact/src/changes.test.ts @@ -8,6 +8,8 @@ import { tsconfigForcesBroad, classifyManifestChange, dropDevDepOnlyDeps, + overrideTargetName, + changedOverrideTargets, } from './changes.js'; describe('isNonImpactful', () => { @@ -228,3 +230,123 @@ describe('dropDevDepOnlyDeps (safety-critical)', () => { ).toEqual(files); }); }); + +const ovr = (overrides: Record, deps = {}, devDeps = {}) => + JSON.stringify({ name: 'x', dependencies: deps, devDependencies: devDeps, pnpm: { overrides } }); + +describe('overrideTargetName', () => { + it.each([ + ['@vitest/browser@<4.1.10', '@vitest/browser'], + ['brace-expansion@5', 'brace-expansion'], + ['node-gyp>undici', 'undici'], + ['@babel/traverse', '@babel/traverse'], + ['undici@7', 'undici'], + ['a>b>@scope/c@^1.0.0', '@scope/c'], + ['@n8n/typeorm>@sentry/node', '@sentry/node'], + // `>` inside a version range is not a parent separator + ['pkg@>=2.0.0', 'pkg'], + ['pkg@>2', 'pkg'], + ['pkg@1||>2', 'pkg'], + ['pkg@^1||>=2.0.0', 'pkg'], + ['pkg@>=1||>=2', 'pkg'], + ['a@1>b@>=2', 'b'], + ['a@=1.2.3>b', 'b'], + ['pkg@1=>2', 'pkg'], + ])('%s → %s', (selector, expected) => { + expect(overrideTargetName(selector)).toBe(expected); + }); + + it('returns null when no package name can be extracted', () => { + expect(overrideTargetName('>=2.0.0')).toBeNull(); + }); +}); + +describe('changedOverrideTargets', () => { + it('returns the target when a pin is added', () => { + expect(changedOverrideTargets(ovr({}), ovr({ 'ws@<8.21.1': '8.21.1' }))).toEqual(['ws']); + }); + it('returns the target when a pin is removed', () => { + expect(changedOverrideTargets(ovr({ 'ws@<8.21.1': '8.21.1' }), ovr({}))).toEqual(['ws']); + }); + it('returns the target when a pin version changes', () => { + expect( + changedOverrideTargets( + ovr({ 'brace-expansion@5': '5.0.7' }), + ovr({ 'brace-expansion@5': '5.0.9' }), + ), + ).toEqual(['brace-expansion']); + }); + it('de-duplicates selectors pinning the same package', () => { + expect( + changedOverrideTargets(ovr({}), ovr({ 'undici@7': '7.29.0', 'node-gyp>undici': '7.29.0' })), + ).toEqual(['undici']); + }); + it('ignores untouched pins', () => { + const same = ovr({ 'ws@<8.21.1': '8.21.1' }); + expect(changedOverrideTargets(same, same)).toEqual([]); + }); +}); + +describe('classifyManifestChange — overrides', () => { + it('override when only a pnpm.overrides pin moves', () => { + expect(classifyManifestChange(ovr({}), ovr({ 'ws@1': '1.0.1' }))).toBe('override'); + }); + it('runtime wins over a co-occurring override change', () => { + expect( + classifyManifestChange(ovr({}, { axios: '1' }), ovr({ 'ws@1': '1.0.1' }, { axios: '2' })), + ).toBe('runtime'); + }); + it('override wins over a co-occurring devDependencies change', () => { + expect( + classifyManifestChange( + ovr({}, {}, { vitest: '1' }), + ovr({ 'ws@1': '1.0.1' }, {}, { vitest: '2' }), + ), + ).toBe('override'); + }); +}); + +describe('dropDevDepOnlyDeps — overrides (safety-critical)', () => { + const files = ['pnpm-lock.yaml', 'package.json']; + const overrideDiff = (target: string) => ({ + 'package.json': { before: ovr({}), after: ovr({ [`${target}@<2`]: '2.0.0' }) }, + }); + const closure = new Set(['ajv', 'fast-uri']); + + it('drops when every override target is outside the runtime closure', () => { + expect(dropDevDepOnlyDeps(files, overrideDiff('@vitest/browser'), closure)).toEqual([]); + }); + it('KEEPS when the override target is inside the runtime closure', () => { + expect(dropDevDepOnlyDeps(files, overrideDiff('fast-uri'), closure)).toEqual(files); + }); + it('KEEPS when no closure is supplied', () => { + expect(dropDevDepOnlyDeps(files, overrideDiff('@vitest/browser'), undefined)).toEqual(files); + }); + it('KEEPS when the closure is empty (broken walk, not proof)', () => { + expect(dropDevDepOnlyDeps(files, overrideDiff('@vitest/browser'), new Set())).toEqual(files); + }); + it('KEEPS when any one of several targets is inside the closure', () => { + const manifests = { + 'package.json': { + before: ovr({}), + after: ovr({ '@vitest/browser@<2': '2.0.0', 'fast-uri@<4': '4.0.0' }), + }, + }; + expect(dropDevDepOnlyDeps(files, manifests, closure)).toEqual(files); + }); + it('KEEPS a runtime-section change even when its override target is outside the closure', () => { + const manifests = { + 'package.json': { + before: ovr({}, { axios: '1' }), + after: ovr({ '@vitest/browser@<2': '2.0.0' }, { axios: '2' }), + }, + }; + expect(dropDevDepOnlyDeps(files, manifests, closure)).toEqual(files); + }); + it('KEEPS when a changed selector cannot be attributed to a package', () => { + const manifests = { + 'package.json': { before: ovr({}), after: ovr({ '>=2.0.0': '2.0.0' }) }, + }; + expect(dropDevDepOnlyDeps(files, manifests, closure)).toEqual(files); + }); +}); diff --git a/packages/testing/test-impact/src/changes.ts b/packages/testing/test-impact/src/changes.ts index 290275a6ef6..788ecbea8ec 100644 --- a/packages/testing/test-impact/src/changes.ts +++ b/packages/testing/test-impact/src/changes.ts @@ -78,7 +78,7 @@ export function forcesBroad(file: string): boolean { } /** A package.json change classified by which dependency sections moved. */ -export type ManifestChangeKind = 'runtime' | 'devDep-only' | 'none'; +export type ManifestChangeKind = 'runtime' | 'devDep-only' | 'override' | 'none'; type ManifestJson = Record | undefined>; /** package.json sections whose changes can reach the runtime bundle. */ @@ -115,18 +115,83 @@ function sectionChanged(before: ManifestJson, after: ManifestJson, section: stri return changedKeysInSection(before, after, section).length > 0; } +/** `pnpm.overrides` selectors changed between two manifests, as written. */ +function changedOverrideSelectors(before: ManifestJson, after: ManifestJson): string[] { + const b = (before.pnpm as { overrides?: Record } | undefined)?.overrides ?? {}; + const a = (after.pnpm as { overrides?: Record } | undefined)?.overrides ?? {}; + const changed: string[] = []; + for (const key of new Set([...Object.keys(b), ...Object.keys(a)])) { + if (b[key] !== a[key]) changed.push(key); + } + return changed; +} + +const NPM_PACKAGE_NAME = /^(@[a-z0-9-~][a-z0-9-._~]*\/)?[a-z0-9-~][a-z0-9-._~]*$/i; + +/** + * The package an override selector pins (`node-gyp>undici` → `undici`, + * `@vitest/browser@<4.1.10` → `@vitest/browser`), or `null` when the selector + * can't be parsed with confidence — callers must then stay broad. + */ +export function overrideTargetName(selector: string): string | null { + // A range `>` (`pkg@>2`, `pkg@1||>2`) follows `@` or `|`, touches whitespace, + // or is `>=` — never a parent>child separator. + let child = selector; + for (let i = selector.length - 1; i > 0; i--) { + if (selector[i] !== '>') continue; + const prev = selector[i - 1]; + const next = selector[i + 1] ?? ''; + if ( + prev === '@' || + prev === '|' || + prev === '=' || + next === '=' || + /\s/.test(prev) || + /\s/.test(next) + ) { + continue; + } + child = selector.slice(i + 1); + break; + } + child = child.trim(); + // `> 0` keeps a leading scope `@` intact when stripping the version range. + const at = child.lastIndexOf('@'); + const name = at > 0 ? child.slice(0, at) : child; + return NPM_PACKAGE_NAME.test(name) ? name : null; +} + +/** + * Packages whose `pnpm.overrides` pin was added, removed or changed. `null` + * when any changed selector fails to parse — the diff can't be attributed. + */ +export function changedOverrideTargets(before: string, after: string): string[] | null { + const selectors = changedOverrideSelectors(parseManifest(before), parseManifest(after)); + const names = new Set(); + for (const selector of selectors) { + const name = overrideTargetName(selector); + if (name === null) return null; + names.add(name); + } + return [...names]; +} + /** * Classify a package.json change by the dependency sections it touched: * - `runtime` — a runtime section (dependencies / optional / peer) moved, so * it can reach the bundle the E2E suite exercises. + * - `override` — a `pnpm.overrides` pin moved; whether it reaches the + * runtime bundle takes a closure check (see {@link dropDevDepOnlyDeps}). * - `devDep-only` — only devDependencies moved → cannot reach the runtime bundle. * - `none` — no dependency section moved (scripts / version / engines / …). - * Unparseable content is treated as an empty manifest. + * Unparseable content is treated as an empty manifest. Checked most-impactful + * first: a mixed devDep+override change classifies as `override`. */ export function classifyManifestChange(before: string, after: string): ManifestChangeKind { const b = parseManifest(before); const a = parseManifest(after); if (RUNTIME_SECTIONS.some((s) => sectionChanged(b, a, s))) return 'runtime'; + if (changedOverrideSelectors(b, a).length > 0) return 'override'; return sectionChanged(b, a, 'devDependencies') ? 'devDep-only' : 'none'; } @@ -217,14 +282,21 @@ export function stripDependencyFiles(files: string[]): string[] { * bundle, so it must not force broad. `manifests` maps each changed package.json * path to its before/after content (the caller reads these from git). * + * An override pins a TRANSITIVE package, which no declared section mentions — + * `fast-uri` (reaches runtime via `ajv`) and `@vitest/browser` (dev-only) look + * identical there. Only `runtimeClosure` membership can tell them apart. + * * Conservative by construction — never drops without positive evidence: * - any runtime-section change → keep everything (real dep change); * - a changed package.json with no supplied diff → treated as runtime; - * - a lockfile change with no changed package.json at all (transitive bump) → kept. + * - a lockfile change with no changed package.json at all (transitive bump) → kept; + * - an override change with a missing/empty closure, an unparseable selector, + * or any target inside the closure → kept. */ export function dropDevDepOnlyDeps( files: string[], manifests: Record, + runtimeClosure?: ReadonlySet, ): string[] { const changedManifests = files.filter(isManifest); if (changedManifests.length === 0) return files; @@ -232,6 +304,21 @@ export function dropDevDepOnlyDeps( manifests[f] ? classifyManifestChange(manifests[f].before, manifests[f].after) : 'runtime', ); if (kinds.includes('runtime')) return files; + + if (kinds.includes('override')) { + // An empty closure is a broken walk, not proof nothing reaches runtime. + if (!runtimeClosure || runtimeClosure.size === 0) return files; + const targets = new Set(); + for (const f of changedManifests) { + if (!manifests[f]) continue; + const names = changedOverrideTargets(manifests[f].before, manifests[f].after); + if (names === null) return files; + for (const name of names) targets.add(name); + } + if (targets.size === 0 || [...targets].some((t) => runtimeClosure.has(t))) return files; + return stripDependencyFiles(files); + } + if (!kinds.includes('devDep-only')) return files; return stripDependencyFiles(files); } diff --git a/packages/testing/test-impact/src/dep-graph.test.ts b/packages/testing/test-impact/src/dep-graph.test.ts index af87efc47b6..26960e5f2da 100644 --- a/packages/testing/test-impact/src/dep-graph.test.ts +++ b/packages/testing/test-impact/src/dep-graph.test.ts @@ -5,7 +5,13 @@ import { changedRuntimeDepsFromManifests, stripDependencyFiles, } from './changes.js'; -import { dependentDirs } from './dep-graph.js'; +import { + dependentDirs, + runtimeClosure, + snapshotKeyToName, + type LockfileImporters, + type LockfileSnapshots, +} from './dep-graph.js'; import type { ImpactMap } from './impact-map.js'; import { DependencyGraphStrategy } from './select/dep-graph-strategy.js'; @@ -111,3 +117,98 @@ describe('DependencyGraphStrategy', () => { expect(r).toEqual({ specs: [], unmapped: [], mode: 'scoped' }); }); }); + +describe('snapshotKeyToName', () => { + it.each([ + ['ajv@8.18.0', 'ajv'], + ['@scope/pkg@1.0.0', '@scope/pkg'], + // peer suffix carries `@`s — must not confuse the version separator + ['@vitest/coverage-v8@4.1.9(vitest@4.1.9)', '@vitest/coverage-v8'], + ['vite@5.0.0(sass@1.98.0)(terser@5.0.0)', 'vite'], + ['plain-name', 'plain-name'], + ])('%s → %s', (key, expected) => { + expect(snapshotKeyToName(key)).toBe(expected); + }); +}); + +describe('runtimeClosure', () => { + // cli links core (runtime) → ajv → fast-uri; test-utils declares vitest but + // is only reachable via devDependencies, so it must stay out. + const importers: LockfileImporters = { + 'packages/cli': { + dependencies: { + 'n8n-core': { specifier: 'workspace:*', version: 'link:../core' }, + axios: { specifier: '^1.0.0', version: '1.0.0' }, + }, + devDependencies: { + 'test-utils': { specifier: 'workspace:*', version: 'link:../test-utils' }, + }, + }, + 'packages/core': { + dependencies: { ajv: { specifier: '^8.0.0', version: '8.18.0' } }, + }, + 'packages/test-utils': { + dependencies: { vitest: { specifier: '^4.0.0', version: '4.1.9' } }, + }, + }; + const snapshots: LockfileSnapshots = { + 'ajv@8.18.0': { dependencies: { 'fast-uri': '3.1.3' } }, + 'fast-uri@3.1.3': {}, + 'axios@1.0.0': {}, + 'vitest@4.1.9': { dependencies: { '@vitest/browser': '4.1.9' } }, + '@vitest/browser@4.1.9': {}, + }; + const opts = { + deployRoots: ['packages/cli'], + runtimeSections: ['dependencies', 'optionalDependencies'], + }; + + it('follows workspace link: edges from the deploy roots', () => { + const closure = runtimeClosure(importers, snapshots, opts); + expect(closure.has('ajv')).toBe(true); + expect(closure.has('axios')).toBe(true); + }); + it('reaches a dep only present via a transitive snapshot edge (the fast-uri shape)', () => { + expect(runtimeClosure(importers, snapshots, opts).has('fast-uri')).toBe(true); + }); + it('excludes deps of a workspace package reachable only via devDependencies (the vitest shape)', () => { + const closure = runtimeClosure(importers, snapshots, opts); + expect(closure.has('vitest')).toBe(false); + expect(closure.has('@vitest/browser')).toBe(false); + }); + it('with empty snapshots the closure is just the declared external roots', () => { + expect([...runtimeClosure(importers, {}, opts)].sort()).toEqual(['ajv', 'axios']); + }); + it('roots the real package behind an npm: alias in an importer (the zod-from-json-schema shape)', () => { + const aliased: LockfileImporters = { + 'packages/cli': { + dependencies: { + 'zod-v3': { + specifier: 'npm:zod-from-json-schema@^0.0.5', + version: 'zod-from-json-schema@0.0.5', + }, + }, + }, + }; + const snaps: LockfileSnapshots = { + 'zod-from-json-schema@0.0.5': { dependencies: { zod: '3.25.76' } }, + }; + const closure = runtimeClosure(aliased, snaps, opts); + expect(closure.has('zod-from-json-schema')).toBe(true); + expect(closure.has('zod')).toBe(true); + }); + it('follows an aliased dep inside a snapshot (the string-width-cjs shape)', () => { + const snaps: LockfileSnapshots = { + 'ajv@8.18.0': { dependencies: { 'string-width-cjs': 'string-width@4.2.3' } }, + 'string-width@4.2.3': { dependencies: { 'emoji-regex': '8.0.0' } }, + }; + const closure = runtimeClosure(importers, snaps, opts); + expect(closure.has('string-width')).toBe(true); + expect(closure.has('emoji-regex')).toBe(true); + }); + it('unknown deploy root → empty closure, no throw', () => { + expect( + runtimeClosure(importers, snapshots, { ...opts, deployRoots: ['packages/nope'] }).size, + ).toBe(0); + }); +}); diff --git a/packages/testing/test-impact/src/dep-graph.ts b/packages/testing/test-impact/src/dep-graph.ts index ec7816fa108..3ec97a25fe4 100644 --- a/packages/testing/test-impact/src/dep-graph.ts +++ b/packages/testing/test-impact/src/dep-graph.ts @@ -18,3 +18,141 @@ export function dependentDirs(deps: string[], importers: WorkspaceImporters): st } return dirs.sort(); } + +/** + * pnpm-lock.yaml `snapshots`: resolved-package key → its dependency sections. + * Keys carry a version and, for peer-resolved packages, a suffix — e.g. + * `ajv@8.18.0`, `@vitest/coverage-v8@4.1.9(vitest@4.1.9)`. + */ +export type LockfileSnapshots = Record< + string, + Record | undefined> | undefined +>; + +/** + * Package name from a snapshot key. The peer suffix must be stripped BEFORE + * looking for the version separator, because it contains `@` of its own — + * otherwise `@vitest/coverage-v8@4.1.9(vitest@4.1.9)` resolves to the wrong + * name, the variant is never visited, and the closure silently under-includes. + */ +export function snapshotKeyToName(key: string): string { + const base = key.replace(/\(.*$/, ''); + const at = base.lastIndexOf('@'); + return at > 0 ? base.slice(0, at) : base; +} + +/** + * Real package behind an aliased dependency value (`string-width@4.2.3`), or + * null for a plain version. Aliases resolve under the REAL package's snapshot + * key, so a walk following only the alias name never visits them. + */ +function aliasedDepName(value: string): string | null { + const name = snapshotKeyToName(value); + return name === value.replace(/\(.*$/, '') ? null : name; +} + +/** A dependency entry in pnpm-lock.yaml's `importers`. */ +type ImporterEntry = { specifier?: string; version?: string } | string; +/** pnpm-lock.yaml `importers`: workspace dir → dependency sections. */ +export type LockfileImporters = Record< + string, + Record | undefined> | undefined +>; + +const LINK_PREFIX = 'link:'; + +/** The `link:` target of a workspace dependency entry, or undefined if external. */ +function workspaceLinkTarget(entry: ImporterEntry): string | undefined { + const candidates = + typeof entry === 'string' ? [entry] : [entry?.specifier ?? '', entry?.version ?? '']; + const link = candidates.find((v) => v.startsWith(LINK_PREFIX)); + return link?.slice(LINK_PREFIX.length); +} + +/** Resolve a `link:../foo` target, relative to the declaring package's dir. */ +function resolveLink(fromDir: string, target: string): string { + const segments = `${fromDir}/${target}`.split('/'); + const out: string[] = []; + for (const segment of segments) { + if (segment === '' || segment === '.') continue; + if (segment === '..') out.pop(); + else out.push(segment); + } + return out.join('/'); +} + +/** + * External dependency names the *deployed* workspace packages declare, following + * workspace `link:` edges through RUNTIME sections only. Runtime-only edges keep + * dev-only workspace packages out: `@n8n/backend-test-utils` declares `vitest` + * in `dependencies` but is itself reachable only via `devDependencies`. + */ +function deployedExternalDeps( + importers: LockfileImporters, + deployRoots: readonly string[], + runtimeSections: readonly string[], +): Set { + const visited = new Set(); + const external = new Set(); + const queue = [...deployRoots]; + while (queue.length > 0) { + const dir = queue.pop(); + if (dir === undefined || visited.has(dir)) continue; + visited.add(dir); + const sections = importers[dir]; + if (!sections) continue; + for (const section of runtimeSections) { + for (const [name, entry] of Object.entries(sections[section] ?? {})) { + const link = workspaceLinkTarget(entry); + if (link !== undefined) { + queue.push(resolveLink(dir, link)); + continue; + } + external.add(name); + const version = typeof entry === 'string' ? entry : (entry?.version ?? ''); + const real = aliasedDepName(version); + if (real !== null) external.add(real); + } + } + } + return external; +} + +/** + * Every package name transitively reachable from the deploy roots' runtime + * dependencies — a name OUTSIDE this set cannot reach the bundle E2E exercises. + * Errs toward including: over-inclusion costs a missed optimisation, + * under-inclusion would skip tests for a live runtime change. + */ +export function runtimeClosure( + importers: LockfileImporters, + snapshots: LockfileSnapshots, + opts: { deployRoots: readonly string[]; runtimeSections: readonly string[] }, +): Set { + const { deployRoots, runtimeSections } = opts; + const variantsByName = new Map(); + for (const key of Object.keys(snapshots)) { + const name = snapshotKeyToName(key); + const existing = variantsByName.get(name); + if (existing) existing.push(key); + else variantsByName.set(name, [key]); + } + + const closure = new Set(); + const queue = [...deployedExternalDeps(importers, deployRoots, runtimeSections)]; + while (queue.length > 0) { + const name = queue.pop(); + if (name === undefined || closure.has(name)) continue; + closure.add(name); + for (const key of variantsByName.get(name) ?? []) { + for (const section of runtimeSections) { + for (const [dep, version] of Object.entries(snapshots[key]?.[section] ?? {})) { + if (!closure.has(dep)) queue.push(dep); + const real = aliasedDepName(version); + if (real !== null && !closure.has(real)) queue.push(real); + } + } + } + } + return closure; +} diff --git a/packages/testing/test-impact/src/index.ts b/packages/testing/test-impact/src/index.ts index 4c53a25456f..d2ff0b20f65 100644 --- a/packages/testing/test-impact/src/index.ts +++ b/packages/testing/test-impact/src/index.ts @@ -17,11 +17,18 @@ export { dropDevDepOnlyDeps, changedRuntimeDeps, changedRuntimeDepsFromManifests, + changedOverrideTargets, stripDependencyFiles, RUNTIME_SECTIONS, type ManifestChangeKind, } from './changes.js'; -export { dependentDirs, type WorkspaceImporters } from './dep-graph.js'; +export { + dependentDirs, + runtimeClosure, + type WorkspaceImporters, + type LockfileImporters, + type LockfileSnapshots, +} from './dep-graph.js'; export type { DiscoveredSpec } from './types.js'; // Strategy + Pipeline selection layer. diff --git a/packages/testing/test-impact/src/select.test.ts b/packages/testing/test-impact/src/select.test.ts index 70e40c6a859..d96f411d708 100644 --- a/packages/testing/test-impact/src/select.test.ts +++ b/packages/testing/test-impact/src/select.test.ts @@ -297,3 +297,61 @@ describe('selectTests — fail-open contract', () => { }); }); }); + +describe('selectTests — pnpm.overrides changes', () => { + let tempDir: string; + + beforeEach(() => { + tempDir = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'select-ovr-'))); + }); + + afterEach(() => { + fs.rmSync(tempDir, { recursive: true, force: true }); + }); + + const ALL_SPECS = ['tests/e2e/a.spec.ts', 'tests/e2e/b.spec.ts']; + const changedFiles = ['package.json', 'pnpm-lock.yaml']; + + const setup = (target: string) => { + const map: ImpactMap = { 'packages/cli/src/x.ts': { '10': ['tests/e2e/a.spec.ts'] } }; + const mapFile = path.join(tempDir, 'map.json'); + fs.writeFileSync(mapFile, JSON.stringify(map)); + const allSpecsFile = path.join(tempDir, 'all-specs.txt'); + fs.writeFileSync(allSpecsFile, ALL_SPECS.join('\n')); + const manifests = { + 'package.json': { + before: JSON.stringify({ pnpm: { overrides: {} } }), + after: JSON.stringify({ pnpm: { overrides: { [`${target}@<2`]: '2.0.0' } } }), + }, + }; + return { mapFile, allSpecsFile, manifests }; + }; + + it('target outside the runtime closure → scoped with no specs (skip)', () => { + const result = selectTests({ + changedFiles, + ...setup('@vitest/browser'), + runtimeClosure: new Set(['ajv', 'fast-uri']), + }); + expect(result.mode).toBe('scoped'); + expect(result.specs).toEqual([]); + }); + + it('target inside the runtime closure → broad', () => { + const result = selectTests({ + changedFiles, + ...setup('fast-uri'), + runtimeClosure: new Set(['ajv', 'fast-uri']), + }); + expect(result.mode).toBe('broad'); + expect(result.specs).toEqual([...ALL_SPECS].sort()); + }); + + it('no runtime closure supplied → broad (cannot prove dev-only)', () => { + const result = selectTests({ + changedFiles, + ...setup('@vitest/browser'), + }); + expect(result.mode).toBe('broad'); + }); +}); diff --git a/packages/testing/test-impact/src/select.ts b/packages/testing/test-impact/src/select.ts index 6de4797b72b..1861981330e 100644 --- a/packages/testing/test-impact/src/select.ts +++ b/packages/testing/test-impact/src/select.ts @@ -54,6 +54,10 @@ export interface SelectTestsInput { * walked to its declaring packages and scoped via the map instead * of forcing broad. */ lockfileImporters?: WorkspaceImporters; + /** Package names reachable from the deployed packages' runtime deps (see + * `runtimeClosure` in dep-graph). Classifies `pnpm.overrides` changes; + * omitted → an override change stays broad. */ + runtimeClosure?: ReadonlySet; } export interface SelectTestsResult extends ResolveResult { @@ -127,8 +131,11 @@ export function selectTests(input: SelectTestsInput): SelectTestsResult { impactful = impactful.filter((f) => !isTsconfig(f)); } - // devDependency-only change can't reach the runtime bundle → dropped. - if (input.manifests) impactful = dropDevDepOnlyDeps(impactful, input.manifests); + // devDependency-only change can't reach the runtime bundle → dropped. Same for + // a `pnpm.overrides` pin whose target is outside the runtime closure. + if (input.manifests) { + impactful = dropDevDepOnlyDeps(impactful, input.manifests, input.runtimeClosure); + } // Runtime-dep change: walk it to the packages that declare it and drop // the dep files from the coverage path. @@ -160,7 +167,10 @@ export function selectTests(input: SelectTestsInput): SelectTestsResult { if (!/(^|\/)package\.json$/.test(f)) return false; const manifest = input.manifests?.[f]; if (!manifest) return true; - return classifyManifestChange(manifest.before, manifest.after) === 'runtime'; + // An `override` still present here failed the closure check → unproven, + // treat like runtime. + const kind = classifyManifestChange(manifest.before, manifest.after); + return kind === 'runtime' || kind === 'override'; }); if (lockfileRemains || unscopedRuntimeManifest) return broad(impactful);