From ff3657ded74e1292ae8570aa6695d658ddaed7f2 Mon Sep 17 00:00:00 2001 From: "n8n-cat-bot[bot]" <283985454+n8n-cat-bot[bot]@users.noreply.github.com> Date: Thu, 4 Jun 2026 16:15:01 +0000 Subject: [PATCH] ci: Activate V8 E2E impact map filter in PR CI (#31757) Co-authored-by: n8n-cat-bot[bot] Co-authored-by: Claude Opus 4.7 --- packages/testing/janitor/src/cli.ts | 74 +++---- .../testing/janitor/src/cli/arg-parser.ts | 5 + .../janitor/src/core/select-e2e.test.ts | 126 +++++++++++ .../testing/janitor/src/core/select-e2e.ts | 72 ++++++ .../playwright/scripts/distribute-tests.mjs | 205 +++++++++++++++--- .../scripts/select-affected-e2e.mjs | 61 ++++-- .../scripts/select-affected-e2e.test.ts | 72 ++++++ 7 files changed, 527 insertions(+), 88 deletions(-) create mode 100644 packages/testing/janitor/src/core/select-e2e.test.ts create mode 100644 packages/testing/janitor/src/core/select-e2e.ts create mode 100644 packages/testing/playwright/scripts/select-affected-e2e.test.ts diff --git a/packages/testing/janitor/src/cli.ts b/packages/testing/janitor/src/cli.ts index 37e03488896..798e0a04f65 100644 --- a/packages/testing/janitor/src/cli.ts +++ b/packages/testing/janitor/src/cli.ts @@ -45,14 +45,7 @@ import { formatBaselineInfo, getBaselinePath, } from './core/baseline.js'; -import { - type ImpactMap, - type InternedImpactMap, - decodeImpactMap, - encodeImpactMap, - mergeCoverage, - resolveImpact, -} from './core/coverage-map.js'; +import { encodeImpactMap, mergeCoverage } from './core/coverage-map.js'; import { extractDiffs } from './core/extract-diffs.js'; import { ImpactAnalyzer, @@ -83,6 +76,7 @@ import { createProject } from './core/project-loader.js'; import { toJSON, toConsole } from './core/reporter.js'; import { filterToFailedSpecs } from './core/retry-filter.js'; import { computeScope, formatScope } from './core/scope-analyzer.js'; +import { selectE2e } from './core/select-e2e.js'; import { TcrExecutor, formatTcrResultConsole, formatTcrResultJSON } from './core/tcr-executor.js'; import { TestDiscoveryAnalyzer } from './core/test-discovery-analyzer.js'; import { runTestScoped } from './core/test-scoped-runner.js'; @@ -539,6 +533,24 @@ async function runOrchestrate(options: CliOptions): Promise { } } + // Composable allowlist filter. distribute-tests.mjs pre-computes the union + // of AST + V8 selection and writes it here; orchestrate then balances shards + // against that subset instead of the full discovered set. + if (options.includeSpecsFile) { + const includeRaw = fs.readFileSync(options.includeSpecsFile, 'utf-8'); + const include = new Set( + includeRaw + .split(/[\n,]+/) + .map((s) => s.trim()) + .filter(Boolean), + ); + const totalBefore = specs.length; + specs = specs.filter((s) => include.has(s.path)); + console.error( + `Include: ${specs.length}/${totalBefore} specs after applying allowlist (${include.size} entries)`, + ); + } + const metrics: Record = {}; if (config.orchestration.metricsPath) { const metricsPath = path.isAbsolute(config.orchestration.metricsPath) @@ -669,45 +681,15 @@ function runMergeCoverage(options: CliOptions): void { ); } -/** select-e2e: changed files + impact map → spec list (JSON). - * - * Two layers of safety, both biased to OVER-select (never miss a regression): - * - FAIL-OPEN on the map source: a missing/unreadable/corrupt map → broad - * (run everything). This is what makes swapping the committed file for a - * remote webhook safe — a fetch failure degrades to running the full suite, - * never to skipping tests. - * - DEFAULT-BROAD on content: any changed file absent from a loaded map → broad. - */ +/** select-e2e: changed files + impact map → spec list (JSON). I/O wrapper + * around {@link selectE2e}, where the fail-open safety contract lives. */ function runSelectE2e(options: CliOptions): void { - const changed = (readChangedFiles(options) ?? []).map((file) => ({ file })); - const allSpecs = options.allSpecsFile - ? fs - .readFileSync(options.allSpecsFile, 'utf8') - .split(/[\n,]+/) - .map((s) => s.trim()) - .filter(Boolean) - : undefined; - - let map: ImpactMap = {}; - let failOpen: string | undefined; - if (options.mapFile && fs.existsSync(options.mapFile)) { - try { - const parsed: unknown = JSON.parse(fs.readFileSync(options.mapFile, 'utf8')); - // Interned form ({specs, files}) is decoded; a plain ImpactMap is used as-is. - const isInterned = - typeof parsed === 'object' && parsed !== null && 'specs' in parsed && 'files' in parsed; - map = isInterned ? decodeImpactMap(parsed as InternedImpactMap) : (parsed as ImpactMap); - } catch (error) { - failOpen = `unreadable map: ${String(error)}`; - } - } else { - failOpen = options.mapFile ? `map not found: ${options.mapFile}` : 'no --map provided'; - } - - // With an empty map every changed file is "unmapped" → resolveImpact returns - // broad, so fail-open falls out of the same code path — no special-casing. - const result = resolveImpact(changed, map, { allSpecs }); - console.log(JSON.stringify({ ...result, failOpen })); + const result = selectE2e({ + changedFiles: readChangedFiles(options) ?? [], + mapFile: options.mapFile, + allSpecsFile: options.allSpecsFile, + }); + console.log(JSON.stringify(result)); } async function main(): Promise { diff --git a/packages/testing/janitor/src/cli/arg-parser.ts b/packages/testing/janitor/src/cli/arg-parser.ts index 50c35ab637a..a667fe9dc29 100644 --- a/packages/testing/janitor/src/cli/arg-parser.ts +++ b/packages/testing/janitor/src/cli/arg-parser.ts @@ -69,6 +69,8 @@ export interface CliOptions { outMap?: string; mapFile?: string; allSpecsFile?: string; + /** Path to a newline-separated allowlist of spec paths (orchestrate). */ + includeSpecsFile?: string; } const SUBCOMMANDS: Record = { @@ -224,6 +226,9 @@ const VALUE_FLAG_HANDLERS: Record { opts.allSpecsFile = value; }, + '--include-specs-file=': (opts, value) => { + opts.includeSpecsFile = value; + }, }; function createDefaultOptions(): CliOptions { diff --git a/packages/testing/janitor/src/core/select-e2e.test.ts b/packages/testing/janitor/src/core/select-e2e.test.ts new file mode 100644 index 00000000000..76d347b9916 --- /dev/null +++ b/packages/testing/janitor/src/core/select-e2e.test.ts @@ -0,0 +1,126 @@ +import * as fs from 'node:fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; + +import { type ImpactMap, type InternedImpactMap, encodeImpactMap } from './coverage-map.js'; +import { selectE2e } from './select-e2e.js'; + +// The handler's fail-open contract is its safety guarantee: every failure mode +// of the map source must degrade to mode 'broad', never to an empty spec set +// (which would silently skip real tests). These cases pin that contract so a +// future refactor of resolveImpact / loadMap can't quietly invert it. + +describe('selectE2e — fail-open contract', () => { + let tempDir: string; + + beforeEach(() => { + tempDir = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'select-e2e-'))); + }); + + afterEach(() => { + fs.rmSync(tempDir, { recursive: true, force: true }); + }); + + const ALL_SPECS = ['tests/e2e/a.spec.ts', 'tests/e2e/b.spec.ts']; + + const writeAllSpecs = (content: string): string => { + const p = path.join(tempDir, 'all-specs.txt'); + fs.writeFileSync(p, content); + return p; + }; + + it('no --map provided → broad with failOpen reason', () => { + const result = selectE2e({ + changedFiles: ['packages/cli/src/x.ts'], + allSpecsFile: writeAllSpecs(ALL_SPECS.join('\n')), + }); + expect(result.mode).toBe('broad'); + expect(result.failOpen).toBe('no --map provided'); + expect(result.specs).toEqual([...ALL_SPECS].sort()); + }); + + it("map path that doesn't exist → broad with failOpen reason", () => { + const missing = path.join(tempDir, 'never-existed.json'); + const result = selectE2e({ + changedFiles: ['packages/cli/src/x.ts'], + mapFile: missing, + allSpecsFile: writeAllSpecs(ALL_SPECS.join('\n')), + }); + expect(result.mode).toBe('broad'); + expect(result.failOpen).toBe(`map not found: ${missing}`); + expect(result.specs).toEqual([...ALL_SPECS].sort()); + }); + + it('corrupt / non-JSON map → broad with failOpen reason', () => { + const mapPath = path.join(tempDir, 'corrupt.json'); + fs.writeFileSync(mapPath, '{not valid json,,,'); + const result = selectE2e({ + changedFiles: ['packages/cli/src/x.ts'], + mapFile: mapPath, + allSpecsFile: writeAllSpecs(ALL_SPECS.join('\n')), + }); + expect(result.mode).toBe('broad'); + expect(result.failOpen).toMatch(/^unreadable map:/); + expect(result.specs).toEqual([...ALL_SPECS].sort()); + }); + + // THE COLLAPSE: empty map → every changed file is unmapped → resolveImpact + // returns broad. If a refactor ever makes resolveImpact({}, …) return scoped/ + // empty, every other test still passes — this one fails. That is the point. + it('empty map {} → broad (the fail-open collapse)', () => { + const mapPath = path.join(tempDir, 'empty.json'); + fs.writeFileSync(mapPath, '{}'); + const result = selectE2e({ + changedFiles: ['packages/cli/src/x.ts'], + mapFile: mapPath, + allSpecsFile: writeAllSpecs(ALL_SPECS.join('\n')), + }); + expect(result.mode).toBe('broad'); + // File was loaded successfully — no failOpen reason. The broad comes from + // resolveImpact's DEFAULT-BROAD rule (changed file absent from the map). + expect(result.failOpen).toBeUndefined(); + expect(result.unmapped).toEqual(['packages/cli/src/x.ts']); + expect(result.specs).toEqual([...ALL_SPECS].sort()); + }); + + it('interned {specs, files} map → decoded correctly, resolves scoped', () => { + const map: ImpactMap = { + 'packages/cli/src/x.ts': { '10': ['tests/e2e/a.spec.ts'] }, + }; + const interned: InternedImpactMap = encodeImpactMap(map); + const mapPath = path.join(tempDir, 'interned.json'); + fs.writeFileSync(mapPath, JSON.stringify(interned)); + + const result = selectE2e({ + changedFiles: ['packages/cli/src/x.ts'], + mapFile: mapPath, + }); + expect(result.mode).toBe('scoped'); + expect(result.failOpen).toBeUndefined(); + expect(result.specs).toEqual(['tests/e2e/a.spec.ts']); + }); + + describe('--all-specs parsing', () => { + const triggerBroad = (allSpecsFile: string) => + selectE2e({ + changedFiles: ['packages/never/covered.ts'], + allSpecsFile, + }); + + it('parses a newline-separated list', () => { + const file = writeAllSpecs('tests/e2e/a.spec.ts\ntests/e2e/b.spec.ts\n'); + expect(triggerBroad(file).specs).toEqual(['tests/e2e/a.spec.ts', 'tests/e2e/b.spec.ts']); + }); + + it('parses a comma-separated list', () => { + const file = writeAllSpecs('tests/e2e/a.spec.ts,tests/e2e/b.spec.ts'); + expect(triggerBroad(file).specs).toEqual(['tests/e2e/a.spec.ts', 'tests/e2e/b.spec.ts']); + }); + + it('parses mixed newline + comma + trims empties', () => { + const file = writeAllSpecs('tests/e2e/a.spec.ts,\n,tests/e2e/b.spec.ts,\n\n'); + expect(triggerBroad(file).specs).toEqual(['tests/e2e/a.spec.ts', 'tests/e2e/b.spec.ts']); + }); + }); +}); diff --git a/packages/testing/janitor/src/core/select-e2e.ts b/packages/testing/janitor/src/core/select-e2e.ts new file mode 100644 index 00000000000..8b62ab8ab96 --- /dev/null +++ b/packages/testing/janitor/src/core/select-e2e.ts @@ -0,0 +1,72 @@ +/** + * `select-e2e` handler: changed files + impact map → spec list. + * + * The file-system-aware wrapper around the pure {@link resolveImpact} resolver. + * This is where the FAIL-OPEN safety contract lives — every failure mode + * (missing map, unreadable map, corrupt JSON, empty map) must degrade to + * `mode: 'broad'` so the caller runs the full suite, never an empty one. + * + * Extracted from {@link runSelectE2e} in `cli.ts` so the contract can be + * exhaustively unit-tested without spawning a subprocess. + */ + +import * as fs from 'node:fs'; + +import { + type ImpactMap, + type InternedImpactMap, + type ResolveResult, + decodeImpactMap, + resolveImpact, +} from './coverage-map.js'; + +export interface SelectE2eInput { + /** Changed files (file paths). */ + changedFiles: string[]; + /** Path to the impact map JSON. Missing/unreadable → fail-open broad. */ + mapFile?: string; + /** Path to a newline/comma separated list of every known spec. */ + allSpecsFile?: string; +} + +export interface SelectE2eResult extends ResolveResult { + /** Set when the map could not be loaded; the result is broad as a safety. */ + failOpen?: string; +} + +function parseSpecList(raw: string): string[] { + return raw + .split(/[\n,]+/) + .map((s) => s.trim()) + .filter(Boolean); +} + +function loadMap(mapFile: string | undefined): { map: ImpactMap; failOpen?: string } { + if (!mapFile) return { map: {}, failOpen: 'no --map provided' }; + if (!fs.existsSync(mapFile)) return { map: {}, failOpen: `map not found: ${mapFile}` }; + try { + const parsed: unknown = JSON.parse(fs.readFileSync(mapFile, 'utf8')); + // Interned form ({specs, files}) is decoded; a plain ImpactMap is used as-is. + const isInterned = + typeof parsed === 'object' && parsed !== null && 'specs' in parsed && 'files' in parsed; + const map = isInterned ? decodeImpactMap(parsed as InternedImpactMap) : (parsed as ImpactMap); + return { map }; + } catch (error) { + return { map: {}, failOpen: `unreadable map: ${String(error)}` }; + } +} + +/** + * Resolve changed files against the impact map, biased to OVER-select. With an + * empty/missing map every changed file is "unmapped" → {@link resolveImpact} + * returns `mode: 'broad'`, so fail-open falls out of the same code path. + */ +export function selectE2e(input: SelectE2eInput): SelectE2eResult { + const allSpecs = input.allSpecsFile + ? parseSpecList(fs.readFileSync(input.allSpecsFile, 'utf8')) + : undefined; + const { map, failOpen } = loadMap(input.mapFile); + const changed = input.changedFiles.map((file) => ({ file })); + const result = resolveImpact(changed, map, { allSpecs }); + return { ...result, failOpen }; +} diff --git a/packages/testing/playwright/scripts/distribute-tests.mjs b/packages/testing/playwright/scripts/distribute-tests.mjs index 04d00208706..cbdb8622342 100644 --- a/packages/testing/playwright/scripts/distribute-tests.mjs +++ b/packages/testing/playwright/scripts/distribute-tests.mjs @@ -7,6 +7,18 @@ * Thin wrapper that calls `janitor orchestrate` for generic shard distribution, * then maps capabilities to n8n-specific Docker images for the CI matrix. * + * Impact scoping is a domain-partitioned UNION of two analyzers (DEVP-364): + * - app-source partition (changes outside packages/testing/playwright/) + * → V8 coverage map via `janitor select-e2e` (see select-affected-e2e.mjs) + * - playwright-internal partition (changes inside the playwright package) + * → AST walk via `janitor impact --test-list` + * Union the two spec sets, write to a temp file, then `orchestrate + * --include-specs-file=` balances shards against the union. + * + * FAIL-OPEN at every layer: if EITHER analyzer bails to broad (V8 map missing, + * AST throws, etc.), broad wins — `orchestrate` runs without the allowlist and + * the full suite ships. Never skip a test on uncertainty. + * * Usage: * node distribute-tests.mjs --matrix --orchestrate # GitHub Actions matrix with images * node distribute-tests.mjs --matrix # Simple matrix (no distribution) @@ -14,6 +26,8 @@ */ import { execFileSync } from 'child_process'; +import { mkdtempSync, writeFileSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; import path from 'path'; import { fileURLToPath } from 'url'; @@ -21,6 +35,7 @@ const __dirname = path.dirname(fileURLToPath(import.meta.url)); const PLAYWRIGHT_DIR = path.resolve(__dirname, '..'); const REPO_ROOT = path.resolve(__dirname, '..', '..', '..', '..'); const JANITOR_CLI = path.resolve(__dirname, '..', '..', 'janitor', 'dist', 'cli.js'); +const SELECT_AFFECTED_E2E = path.resolve(__dirname, 'select-affected-e2e.mjs'); const PLAYWRIGHT_PREFIX = path.relative(REPO_ROOT, PLAYWRIGHT_DIR) + path.sep; const CONTAINER_STARTUP_TIME = 22_500; // 22.5s average per fixture @@ -53,37 +68,115 @@ function getRequiredImages(capabilities) { return [...images].sort(); } -function hasExternalFiles(files) { - const externalFiles = files.split(',').filter((f) => !f.startsWith(PLAYWRIGHT_PREFIX)); - if (externalFiles.length === 0) return false; +/** + * Partition CSV of changed files into (a) inside the playwright package, + * (b) everywhere else. The AST analyzer can only trace (a); the V8 coverage + * map keys on (b). Empty CSVs short-circuit to empty arrays. + */ +function partitionFiles(filesCsv) { + const internal = []; + const external = []; + for (const raw of filesCsv.split(',')) { + const f = raw.trim(); + if (!f) continue; + if (f.startsWith(PLAYWRIGHT_PREFIX)) internal.push(f); + else external.push(f); + } + return { internal, external }; +} - // Files outside the playwright project can't be traced by the impact analyzer - console.error( - `Impact: ${externalFiles.length} file(s) outside playwright project — running all tests`, +/** + * Query the V8 coverage map for specs affected by app-source changes. + * + * Returns `{ broad: true, reason }` whenever the underlying selection is broad + * (map missing/stale/corrupt, or any unmapped change file). Broad propagates + * up the union as "broad wins" — orchestrate runs without the allowlist. + * + * Any thrown error from the wrapper is caught and treated as broad — fail-open. + */ +function selectV8Specs(externalFiles) { + if (externalFiles.length === 0) + return { broad: false, specs: new Set(), reason: 'no app-source changes' }; + try { + const out = execFileSync('node', [SELECT_AFFECTED_E2E, ...externalFiles], { + encoding: 'utf-8', + stdio: ['pipe', 'pipe', 'inherit'], + }); + const parsed = JSON.parse(out); + if (parsed.mode === 'broad') { + return { + broad: true, + reason: parsed.failOpen ?? `unmapped: ${(parsed.unmapped ?? []).join(', ')}`, + }; + } + return { broad: false, specs: new Set(parsed.specs ?? []) }; + } catch (error) { + return { broad: true, reason: `select-e2e failed: ${String(error)}` }; + } +} + +/** + * Query the AST walker for specs affected by playwright-internal changes. + * + * Returns a Set of spec paths (playwright-root-relative) or `{ broad: true }` + * if the analyzer fails. The AST walk only sees relationships expressed in + * source — a change to a non-test file the walker doesn't recognise is + * silently dropped, but that is the AST's own concern; we only fail-open on + * thrown errors, matching the V8 path. + */ +function selectAstSpecs(internalFiles, base) { + if (internalFiles.length === 0) return { broad: false, specs: new Set() }; + // Strip the playwright prefix — janitor runs with cwd=PLAYWRIGHT_DIR. + const normalized = internalFiles.map((f) => + f.startsWith(PLAYWRIGHT_PREFIX) ? f.slice(PLAYWRIGHT_PREFIX.length) : f, ); - for (const f of externalFiles) console.error(` - ${f}`); - return true; + const cliArgs = ['impact', '--test-list', `--files=${normalized.join(',')}`]; + if (base) cliArgs.push(`--base=${base}`); + try { + const out = execFileSync('node', [JANITOR_CLI, ...cliArgs], { + cwd: PLAYWRIGHT_DIR, + encoding: 'utf-8', + stdio: ['pipe', 'pipe', 'inherit'], + }); + const specs = out + .split('\n') + .map((s) => s.trim()) + .filter(Boolean); + return { broad: false, specs: new Set(specs) }; + } catch (error) { + return { broad: true, reason: `ast impact failed: ${String(error)}` }; + } +} + +function logSelectionDecision(decision) { + console.error('\n🎯 Impact selection:'); + console.error(` mode: ${decision.mode}`); + if (decision.v8) { + const v8 = decision.v8; + console.error( + ` v8 (app-source): ${v8.broad ? `broad (${v8.reason})` : `${v8.specs.size} specs`}`, + ); + } else { + console.error(' v8 (app-source): skipped (no app-source changes)'); + } + if (decision.ast) { + const ast = decision.ast; + console.error( + ` ast (playwright): ${ast.broad ? `broad (${ast.reason})` : `${ast.specs.size} specs`}`, + ); + } else { + console.error(' ast (playwright): skipped (no playwright changes)'); + } + if (decision.unionSize !== undefined) { + console.error(` union: ${decision.unionSize} specs`); + } + console.error(''); } function getOrchestration(numShards, options = {}) { const cliArgs = ['orchestrate', `--shards=${numShards}`]; - // Disable impact when explicit files include non-playwright paths the analyzer can't trace. - // When no files are provided, janitor auto-detects via git — impact stays enabled. - const useImpact = options.impact && !(options.files && hasExternalFiles(options.files)); - - if (useImpact) { - cliArgs.push('--impact'); - if (options.base) cliArgs.push(`--base=${options.base}`); - } - if (useImpact && options.files) { - // Normalize repo-root-relative paths to playwright-root-relative - // git diff gives 'packages/testing/playwright/foo.ts', janitor expects 'foo.ts' - const normalized = options.files - .split(',') - .map((f) => (f.startsWith(PLAYWRIGHT_PREFIX) ? f.slice(PLAYWRIGHT_PREFIX.length) : f)) - .join(','); - cliArgs.push(`--files=${normalized}`); - } + const includeFile = options.includeSpecsFile; + if (includeFile) cliArgs.push(`--include-specs-file=${includeFile}`); const output = execFileSync('node', [JANITOR_CLI, ...cliArgs], { cwd: PLAYWRIGHT_DIR, encoding: 'utf-8', @@ -92,6 +185,42 @@ function getOrchestration(numShards, options = {}) { return JSON.parse(output); } +/** + * Resolve the impact decision for a CI run: union AST + V8 selection over the + * changed-files partition, OR broad if either analyzer bails. Writes the union + * to a temp file and returns its path (the caller passes it to orchestrate). + * + * Returns `null` when broad — caller orchestrates without the allowlist. + */ +function resolveImpactAllowlist(filesCsv, base) { + const { internal, external } = partitionFiles(filesCsv); + const v8 = selectV8Specs(external); + const ast = selectAstSpecs(internal, base); + + // Fail-open: either bails → broad wins. Run everything. + if (v8.broad || ast.broad) { + logSelectionDecision({ + mode: 'broad', + v8: external.length > 0 ? v8 : null, + ast: internal.length > 0 ? ast : null, + }); + return null; + } + + const union = new Set([...(v8.specs ?? []), ...(ast.specs ?? [])]); + logSelectionDecision({ + mode: 'scoped', + v8: external.length > 0 ? v8 : null, + ast: internal.length > 0 ? ast : null, + unionSize: union.size, + }); + + const tmp = mkdtempSync(path.join(tmpdir(), 'distribute-tests-')); + const allowPath = path.join(tmp, 'include-specs.txt'); + writeFileSync(allowPath, [...union].join('\n')); + return allowPath; +} + const args = process.argv.slice(2); const matrixMode = args.includes('--matrix'); const orchestrateMode = args.includes('--orchestrate'); @@ -106,6 +235,25 @@ if (!shards || shards < 1) { process.exit(1); } +let includeSpecsFile; +let cleanupPaths = []; +if (impactMode && filesArg) { + includeSpecsFile = resolveImpactAllowlist(filesArg, baseArg); + if (includeSpecsFile) cleanupPaths.push(path.dirname(includeSpecsFile)); +} else if (impactMode) { + console.error('Impact: no --files provided — running full suite'); +} + +function cleanup() { + for (const p of cleanupPaths) { + try { + rmSync(p, { recursive: true, force: true }); + } catch { + // best-effort + } + } +} + if (matrixMode) { if (!orchestrateMode) { const matrix = Array.from({ length: shards }, (_, i) => ({ @@ -115,7 +263,7 @@ if (matrixMode) { })); console.log(JSON.stringify(matrix)); } else { - const result = getOrchestration(shards, { impact: impactMode, files: filesArg, base: baseArg }); + const result = getOrchestration(shards, { includeSpecsFile }); if (result.shards.length === 0) { console.error('\n⏭️ No specs to run — all filtered out by discovery/impact. Skipping.\n'); @@ -152,11 +300,14 @@ if (matrixMode) { const index = parseInt(args[1]); if (isNaN(index) || index < 0 || index >= shards) { console.error(`Index must be between 0 and ${shards - 1}`); + cleanup(); process.exit(1); } - const result = getOrchestration(shards, { impact: impactMode, files: filesArg, base: baseArg }); + const result = getOrchestration(shards, { includeSpecsFile }); const shard = result.shards[index]; if (shard) { console.log(shard.specs.filter((s) => !QUARANTINE.has(s)).join('\n')); } } + +cleanup(); diff --git a/packages/testing/playwright/scripts/select-affected-e2e.mjs b/packages/testing/playwright/scripts/select-affected-e2e.mjs index fba4bd8112f..d485f409849 100644 --- a/packages/testing/playwright/scripts/select-affected-e2e.mjs +++ b/packages/testing/playwright/scripts/select-affected-e2e.mjs @@ -17,30 +17,61 @@ */ import { execFileSync } from 'node:child_process'; -import { existsSync } from 'node:fs'; +import { existsSync, accessSync, constants as fsConstants } from 'node:fs'; import path from 'node:path'; import { fileURLToPath } from 'node:url'; -const __dirname = path.dirname(fileURLToPath(import.meta.url)); +const __filename = fileURLToPath(import.meta.url); +const __dirname = path.dirname(__filename); const REPO_ROOT = path.resolve(__dirname, '..', '..', '..', '..'); const JANITOR_CLI = path.resolve(__dirname, '..', '..', 'janitor', 'dist', 'cli.js'); const COMMITTED_MAP = path.join(REPO_ROOT, '.github', 'test-metrics', 'e2e-impact-map.json'); -/** @returns {string | null} path to a readable impact map, or null → fail-open broad. */ -function resolveMapPath() { +/** + * @param {{ mapPath?: string }} [opts] + * @returns {string | null} path to a readable impact map, or null → fail-open broad. + * + * Wrapped in try/catch so any I/O failure (race deletion, perms, fs error) is + * swallowed into a null return — never throws. The caller then omits --map, + * which makes select-e2e fail open to broad (run the full suite). + */ +export function resolveMapPath(opts = {}) { // FUTURE: fetch a remote webhook to a temp file here and return that path; - // wrap in try/catch and return null on failure (fail-open). - return existsSync(COMMITTED_MAP) ? COMMITTED_MAP : null; + // keep the try/catch — return null on failure (fail-open). + const target = opts.mapPath ?? COMMITTED_MAP; + try { + if (!existsSync(target)) return null; + accessSync(target, fsConstants.R_OK); + return target; + } catch { + return null; + } } -const changedFiles = process.argv.slice(2).join(',') || process.env.CHANGED_FILES || ''; -const mapPath = resolveMapPath(); +/** + * Build the janitor argv for `select-e2e`. Pure (no I/O) so the wrapper's glue + * can be tested without spawning a subprocess. If `mapPath` is null we omit + * `--map`, which makes select-e2e fail open to broad. + * + * @param {{ changedFiles: string, mapPath: string | null, allSpecs?: string }} input + * @returns {string[]} + */ +export function buildArgs({ changedFiles, mapPath, allSpecs }) { + const args = ['select-e2e', `--changed-files=${changedFiles}`]; + if (mapPath) args.push(`--map=${mapPath}`); + if (allSpecs) args.push(`--all-specs=${allSpecs}`); + return args; +} -const args = ['select-e2e', `--changed-files=${changedFiles}`]; -// Omitting --map (or pointing at a missing file) makes select-e2e fail open to broad. -if (mapPath) args.push(`--map=${mapPath}`); -const allSpecs = process.env.ALL_SPECS_FILE; -if (allSpecs) args.push(`--all-specs=${allSpecs}`); +function runAsScript() { + const changedFiles = process.argv.slice(2).join(',') || process.env.CHANGED_FILES || ''; + const mapPath = resolveMapPath(); + const args = buildArgs({ changedFiles, mapPath, allSpecs: process.env.ALL_SPECS_FILE }); + const out = execFileSync('node', [JANITOR_CLI, ...args], { encoding: 'utf-8' }); + process.stdout.write(out); +} -const out = execFileSync('node', [JANITOR_CLI, ...args], { encoding: 'utf-8' }); -process.stdout.write(out); +// Only execute when invoked directly — keeps the module importable for tests. +if (process.argv[1] && path.resolve(process.argv[1]) === __filename) { + runAsScript(); +} diff --git a/packages/testing/playwright/scripts/select-affected-e2e.test.ts b/packages/testing/playwright/scripts/select-affected-e2e.test.ts new file mode 100644 index 00000000000..cab66a03ed1 --- /dev/null +++ b/packages/testing/playwright/scripts/select-affected-e2e.test.ts @@ -0,0 +1,72 @@ +import * as fs from 'node:fs'; +import * as os from 'node:os'; +import * as path from 'node:path'; + +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; + +// @ts-expect-error — JS module without types; runtime export is what we test. +import { buildArgs, resolveMapPath } from './select-affected-e2e.mjs'; + +// The wrapper is the bridge between CI (raw changed-files list) and janitor +// select-e2e. Its only job is to keep selection FAIL-OPEN: any failure in +// locating the map must degrade to broad — never throw, never hide selection. + +describe('select-affected-e2e wrapper — fail-open contract', () => { + let tempDir: string; + + beforeEach(() => { + tempDir = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'select-wrapper-'))); + }); + + afterEach(() => { + fs.rmSync(tempDir, { recursive: true, force: true }); + }); + + describe('resolveMapPath()', () => { + it('returns null when the committed map is missing (wrapper degrades to broad)', () => { + const missing = path.join(tempDir, 'never-existed.json'); + expect(resolveMapPath({ mapPath: missing })).toBeNull(); + }); + + it('returns the path when the map exists and is readable', () => { + const p = path.join(tempDir, 'present.json'); + fs.writeFileSync(p, '{}'); + expect(resolveMapPath({ mapPath: p })).toBe(p); + }); + + it('returns null on an unreadable map (e.g. perms), never throws', () => { + // On non-root environments chmod 000 makes accessSync(R_OK) throw. + // Skip the assertion on root (CI sometimes runs as root and can read anything). + if (process.getuid?.() === 0) return; + const p = path.join(tempDir, 'unreadable.json'); + fs.writeFileSync(p, '{}'); + fs.chmodSync(p, 0o000); + try { + expect(resolveMapPath({ mapPath: p })).toBeNull(); + } finally { + fs.chmodSync(p, 0o644); + } + }); + }); + + describe('buildArgs()', () => { + // Omitting --map is exactly how the wrapper signals fail-open broad to + // the janitor CLI — assert by absence, not by a placeholder value. + it('omits --map when mapPath is null (fail-open broad)', () => { + const args = buildArgs({ changedFiles: 'a.ts,b.ts', mapPath: null }); + expect(args).toEqual(['select-e2e', '--changed-files=a.ts,b.ts']); + expect(args.some((a: string) => a.startsWith('--map='))).toBe(false); + }); + + it('passes --map and --all-specs through when provided', () => { + expect( + buildArgs({ changedFiles: 'a.ts', mapPath: '/tmp/m.json', allSpecs: '/tmp/s.txt' }), + ).toEqual([ + 'select-e2e', + '--changed-files=a.ts', + '--map=/tmp/m.json', + '--all-specs=/tmp/s.txt', + ]); + }); + }); +});