mirror of
https://github.com/n8n-io/n8n.git
synced 2026-07-27 19:15:01 +02:00
test: Harden e2e suite against tsconfig resolution changes (#34623)
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Charlie Kolb <charlie@n8n.io>
This commit is contained in:
parent
8e5fdb4b2d
commit
ccd5a87adb
|
|
@ -79,7 +79,7 @@ import {
|
|||
} from './core/method-usage-analyzer.js';
|
||||
import { createProject } from './core/project-loader.js';
|
||||
import { readLockfileImporters } from './core/read-lockfile-importers.js';
|
||||
import { readManifestDiffs } from './core/read-manifest-diffs.js';
|
||||
import { readManifestDiffs, readTsconfigDiffs } from './core/read-manifest-diffs.js';
|
||||
import { toJSON, toConsole } from './core/reporter.js';
|
||||
import { filterToFailedSpecs } from './core/retry-filter.js';
|
||||
import { computeScope, formatScope } from './core/scope-analyzer.js';
|
||||
|
|
@ -686,6 +686,8 @@ function runSelect(options: CliOptions): void {
|
|||
// devDependency-only classifier can drop a devDep-only lockfile change.
|
||||
// No base (local dev) → omit manifests → conservative (keep lockfile broad).
|
||||
const manifests = options.baseRef ? readManifestDiffs(changedFiles, options.baseRef) : undefined;
|
||||
// Same for tsconfig diffs, feeding the resolution-key classifier.
|
||||
const tsconfigs = options.baseRef ? readTsconfigDiffs(changedFiles, options.baseRef) : undefined;
|
||||
// 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.
|
||||
|
|
@ -698,6 +700,7 @@ function runSelect(options: CliOptions): void {
|
|||
mapFile: options.mapFile,
|
||||
allSpecsFile: options.allSpecsFile,
|
||||
manifests,
|
||||
tsconfigs,
|
||||
lockfileImporters,
|
||||
});
|
||||
console.log(JSON.stringify(result));
|
||||
|
|
|
|||
|
|
@ -1,7 +1,7 @@
|
|||
/**
|
||||
* before/after content of each changed package.json, for the `@n8n/test-impact`
|
||||
* devDependency classifier. Any read failure → '' so the classifier stays
|
||||
* conservative (an unreadable manifest is treated as a non-devDep change, kept).
|
||||
* before/after content of changed package.json / tsconfig files, for the
|
||||
* `@n8n/test-impact` classifiers. Any read failure → '' so a classifier stays
|
||||
* conservative (an unreadable file is treated as impactful).
|
||||
*/
|
||||
import { existsSync, readFileSync } from 'node:fs';
|
||||
import { join } from 'node:path';
|
||||
|
|
@ -9,6 +9,7 @@ import { join } from 'node:path';
|
|||
import { getFileAtRef, getGitRoot } from '../utils/git-operations.js';
|
||||
|
||||
const isManifest = (file: string): boolean => /(^|\/)package\.json$/.test(file);
|
||||
const isTsconfig = (file: string): boolean => /(^|\/)tsconfig([.\w-]*)\.json$/.test(file);
|
||||
|
||||
/** Read the working-tree file, or '' on any failure (missing, unreadable, or a
|
||||
* TOCTOU delete between the existsSync check and the read). */
|
||||
|
|
@ -20,20 +21,35 @@ function readWorkingTree(abs: string): string {
|
|||
}
|
||||
}
|
||||
|
||||
export function readManifestDiffs(
|
||||
/** before/after content of each changed file matching `predicate`. */
|
||||
function readFileDiffs(
|
||||
changedFiles: string[],
|
||||
baseRef: string,
|
||||
predicate: (file: string) => boolean,
|
||||
): Record<string, { before: string; after: string }> {
|
||||
const manifests = changedFiles.filter(isManifest);
|
||||
if (manifests.length === 0) return {};
|
||||
const matched = changedFiles.filter(predicate);
|
||||
if (matched.length === 0) return {};
|
||||
const root = getGitRoot(process.cwd());
|
||||
const out: Record<string, { before: string; after: string }> = {};
|
||||
for (const file of manifests) {
|
||||
const abs = join(root, file);
|
||||
for (const file of matched) {
|
||||
out[file] = {
|
||||
before: getFileAtRef(file, baseRef) ?? '',
|
||||
after: readWorkingTree(abs),
|
||||
after: readWorkingTree(join(root, file)),
|
||||
};
|
||||
}
|
||||
return out;
|
||||
}
|
||||
|
||||
export function readManifestDiffs(
|
||||
changedFiles: string[],
|
||||
baseRef: string,
|
||||
): Record<string, { before: string; after: string }> {
|
||||
return readFileDiffs(changedFiles, baseRef, isManifest);
|
||||
}
|
||||
|
||||
export function readTsconfigDiffs(
|
||||
changedFiles: string[],
|
||||
baseRef: string,
|
||||
): Record<string, { before: string; after: string }> {
|
||||
return readFileDiffs(changedFiles, baseRef, isTsconfig);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -1,8 +1,8 @@
|
|||
import { workflow, trigger, node } from '@n8n/workflow-sdk';
|
||||
import { expect } from '@playwright/test';
|
||||
import type { IWorkflowBase } from 'n8n-workflow';
|
||||
import { nanoid } from 'nanoid';
|
||||
|
||||
import { workflow, trigger, node } from '../../../../@n8n/workflow-sdk/src';
|
||||
import type { n8nPage } from '../../pages/n8nPage';
|
||||
import type { ApiHelpers } from '../../services/api-helper';
|
||||
import type { TestUser } from '../../services/user-api-helper';
|
||||
|
|
|
|||
|
|
@ -1,8 +1,8 @@
|
|||
import { workflow, trigger, node } from '@n8n/workflow-sdk';
|
||||
import flatted from 'flatted';
|
||||
import type { IWorkflowBase } from 'n8n-workflow';
|
||||
import { nanoid } from 'nanoid';
|
||||
|
||||
import { workflow, trigger, node } from '../../../../../@n8n/workflow-sdk/src';
|
||||
import { N8N_AUTH_COOKIE } from '../../../config/constants';
|
||||
import { test, expect } from '../../../fixtures/base';
|
||||
|
||||
|
|
|
|||
|
|
@ -1,8 +1,8 @@
|
|||
import { workflow, trigger, node } from '@n8n/workflow-sdk';
|
||||
import flatted from 'flatted';
|
||||
import type { IWorkflowBase } from 'n8n-workflow';
|
||||
import { nanoid } from 'nanoid';
|
||||
|
||||
import { workflow, trigger, node } from '../../../../../@n8n/workflow-sdk/src';
|
||||
import { test, expect } from '../../../fixtures/base';
|
||||
|
||||
const TRIGGER_NAME = 'Manual Trigger';
|
||||
|
|
|
|||
|
|
@ -4,6 +4,8 @@ import {
|
|||
isNonImpactful,
|
||||
filterImpactfulChanges,
|
||||
forcesBroad,
|
||||
isTsconfig,
|
||||
tsconfigForcesBroad,
|
||||
classifyManifestChange,
|
||||
dropDevDepOnlyDeps,
|
||||
} from './changes.js';
|
||||
|
|
@ -25,8 +27,6 @@ describe('isNonImpactful', () => {
|
|||
'assets/logo.svg',
|
||||
'scripts/release/build.mjs',
|
||||
'turbo.json',
|
||||
'tsconfig.json',
|
||||
'packages/cli/tsconfig.build.json',
|
||||
'biome.jsonc',
|
||||
'packages/testing/test-impact/vitest.config.ts',
|
||||
'.eslintrc.js',
|
||||
|
|
@ -47,6 +47,9 @@ describe('isNonImpactful', () => {
|
|||
// Container + patches affect runtime / a dependency — never ignored
|
||||
'docker/images/n8n/Dockerfile',
|
||||
'patches/some-dep.patch',
|
||||
// tsconfig can carry module-resolution keys → routed via tsconfigForcesBroad
|
||||
'tsconfig.json',
|
||||
'packages/cli/tsconfig.build.json',
|
||||
])('treats %s as impactful', (file) => {
|
||||
expect(isNonImpactful(file)).toBe(false);
|
||||
});
|
||||
|
|
@ -96,6 +99,70 @@ describe('forcesBroad', () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe('isTsconfig', () => {
|
||||
it.each(['tsconfig.json', 'packages/cli/tsconfig.build.json', 'a/tsconfig.go.json'])(
|
||||
'recognises %s',
|
||||
(file) => {
|
||||
expect(isTsconfig(file)).toBe(true);
|
||||
},
|
||||
);
|
||||
|
||||
it.each(['tsconfig.ts', 'packages/cli/package.json', 'config.json'])(
|
||||
'does not match %s',
|
||||
(file) => {
|
||||
expect(isTsconfig(file)).toBe(false);
|
||||
},
|
||||
);
|
||||
});
|
||||
|
||||
describe('tsconfigForcesBroad', () => {
|
||||
const ts = (compilerOptions = {}, extra = {}) => JSON.stringify({ compilerOptions, ...extra });
|
||||
|
||||
it('forces broad when compilerOptions.paths changes', () => {
|
||||
const before = ts({ paths: { 'esprima-next': ['./x'] } });
|
||||
const after = ts({ paths: { 'n8n-workflow': ['./src/index.ts'], 'esprima-next': ['./x'] } });
|
||||
expect(tsconfigForcesBroad(before, after)).toBe(true);
|
||||
});
|
||||
|
||||
it('forces broad when baseUrl changes', () => {
|
||||
expect(tsconfigForcesBroad(ts({ baseUrl: '.' }), ts({ baseUrl: './src' }))).toBe(true);
|
||||
});
|
||||
|
||||
it('forces broad when moduleResolution changes', () => {
|
||||
expect(
|
||||
tsconfigForcesBroad(ts({ moduleResolution: 'node' }), ts({ moduleResolution: 'bundler' })),
|
||||
).toBe(true);
|
||||
});
|
||||
|
||||
it('forces broad when customConditions changes', () => {
|
||||
const before = ts({ customConditions: ['development'] });
|
||||
const after = ts({ customConditions: ['production'] });
|
||||
expect(tsconfigForcesBroad(before, after)).toBe(true);
|
||||
});
|
||||
|
||||
it('forces broad when extends changes', () => {
|
||||
const before = ts({}, { extends: '@n8n/typescript-config/modern/tsconfig.json' });
|
||||
const after = ts({}, { extends: '@n8n/typescript-config/modern/tsconfig.go.json' });
|
||||
expect(tsconfigForcesBroad(before, after)).toBe(true);
|
||||
});
|
||||
|
||||
it('does NOT force broad for a resolution-neutral flag change', () => {
|
||||
const before = ts({ strict: true, paths: { a: ['./a'] } });
|
||||
const after = ts({ strict: false, target: 'ES2022', paths: { a: ['./a'] } });
|
||||
expect(tsconfigForcesBroad(before, after)).toBe(false);
|
||||
});
|
||||
|
||||
it('tolerates comments and trailing commas', () => {
|
||||
const before = '{\n // base\n "compilerOptions": { "paths": { "a": ["./a"] } },\n}';
|
||||
const after = '{\n // base\n "compilerOptions": { "paths": { "a": ["./b"] } },\n}';
|
||||
expect(tsconfigForcesBroad(before, after)).toBe(true);
|
||||
});
|
||||
|
||||
it('forces broad when content is unparseable (conservative)', () => {
|
||||
expect(tsconfigForcesBroad('{ not json', '{ still not')).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
const pkg = (deps = {}, devDeps = {}, extra = {}) =>
|
||||
JSON.stringify({ name: 'x', dependencies: deps, devDependencies: devDeps, ...extra });
|
||||
|
||||
|
|
|
|||
|
|
@ -13,8 +13,9 @@
|
|||
*
|
||||
* NOT ignored (deliberately): `docker`/Dockerfiles (define the container the
|
||||
* E2E suite runs in), `pnpm-lock.yaml`/`package.json` (dependency changes —
|
||||
* handled by the devDep classifier / dep-graph selector), and `patches/**`
|
||||
* (edits a dependency's code = a dependency change).
|
||||
* handled by the devDep classifier / dep-graph selector), `patches/**`
|
||||
* (edits a dependency's code = a dependency change), and `tsconfig*.json`
|
||||
* (can carry module-resolution keys — see {@link tsconfigForcesBroad}).
|
||||
*/
|
||||
|
||||
const NON_IMPACTFUL: Array<(f: string) => boolean> = [
|
||||
|
|
@ -31,9 +32,10 @@ const NON_IMPACTFUL: Array<(f: string) => boolean> = [
|
|||
(f) => /^(docs|assets)\/.*\.(png|jpe?g|gif|svg|webp)$/.test(f),
|
||||
// Repo automation scripts (not shipped, not exercised by E2E)
|
||||
(f) => f.startsWith('scripts/'),
|
||||
// Named build / lint / test-runner config (NOT all json/yaml)
|
||||
// Named build / lint / test-runner config (NOT all json/yaml). `tsconfig*` is
|
||||
// deliberately absent — it's routed through tsconfigForcesBroad instead.
|
||||
(f) =>
|
||||
/(^|\/)(turbo\.json|tsconfig([.\w-]*)\.json|biome\.jsonc?|vitest\.config\.[cm]?[jt]s|\.eslintrc[\w.]*|\.prettierrc[\w.]*)$/.test(
|
||||
/(^|\/)(turbo\.json|biome\.jsonc?|vitest\.config\.[cm]?[jt]s|\.eslintrc[\w.]*|\.prettierrc[\w.]*)$/.test(
|
||||
f,
|
||||
),
|
||||
];
|
||||
|
|
@ -158,6 +160,52 @@ export function changedRuntimeDepsFromManifests(
|
|||
const isManifest = (f: string): boolean => /(^|\/)package\.json$/.test(f);
|
||||
const isLockfile = (f: string): boolean => f === 'pnpm-lock.yaml';
|
||||
|
||||
export const isTsconfig = (f: string): boolean => /(^|\/)tsconfig([.\w-]*)\.json$/.test(f);
|
||||
|
||||
/** compilerOptions keys that change which module a bare import resolves to. */
|
||||
const TSCONFIG_RESOLUTION_KEYS = [
|
||||
'paths',
|
||||
'baseUrl',
|
||||
'moduleResolution',
|
||||
'customConditions',
|
||||
] as const;
|
||||
|
||||
/** Tolerant parse for tsconfig (allows comments + trailing commas). Null when
|
||||
* unparseable, which the caller treats as "force broad". */
|
||||
function parseTsconfig(raw: string): Record<string, unknown> | null {
|
||||
if (!raw.trim()) return null;
|
||||
try {
|
||||
return JSON.parse(raw) as Record<string, unknown>;
|
||||
} catch {
|
||||
try {
|
||||
const stripped = raw
|
||||
.replace(/\/\*[\s\S]*?\*\//g, '')
|
||||
.replace(/(^|[^:])\/\/.*$/gm, '$1')
|
||||
.replace(/,(\s*[}\]])/g, '$1');
|
||||
return JSON.parse(stripped) as Record<string, unknown>;
|
||||
} catch {
|
||||
return null;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* True when a tsconfig change touches a resolution key (`paths`/`baseUrl`/
|
||||
* `moduleResolution`/`customConditions`/`extends`), which re-points imports for
|
||||
* every spec and can't be attributed in the coverage map → force the full
|
||||
* suite. A type-check-only edit (`strict`, `target`, …) resolves to the same
|
||||
* modules and is non-impactful.
|
||||
*/
|
||||
export function tsconfigForcesBroad(before: string, after: string): boolean {
|
||||
const b = parseTsconfig(before);
|
||||
const a = parseTsconfig(after);
|
||||
if (!b || !a) return true;
|
||||
if (JSON.stringify(b.extends) !== JSON.stringify(a.extends)) return true;
|
||||
const bco = (b.compilerOptions ?? {}) as Record<string, unknown>;
|
||||
const aco = (a.compilerOptions ?? {}) as Record<string, unknown>;
|
||||
return TSCONFIG_RESOLUTION_KEYS.some((k) => JSON.stringify(bco[k]) !== JSON.stringify(aco[k]));
|
||||
}
|
||||
|
||||
/** Remove the lockfile + every package.json from a changed-file set. */
|
||||
export function stripDependencyFiles(files: string[]): string[] {
|
||||
return files.filter((f) => !isLockfile(f) && !isManifest(f));
|
||||
|
|
|
|||
|
|
@ -216,6 +216,64 @@ describe('selectTests — fail-open contract', () => {
|
|||
expect(result.mode).toBe('broad');
|
||||
});
|
||||
|
||||
// A stray `paths` entry re-points every spec's imports — the regression this guards.
|
||||
it('tsconfig resolution-key change (paths) → broad, never declared uncovered', () => {
|
||||
const map: ImpactMap = { 'packages/cli/src/x.ts': { '10': ['tests/e2e/a.spec.ts'] } };
|
||||
const mapPath = path.join(tempDir, 'map-tsconfig-paths.json');
|
||||
fs.writeFileSync(mapPath, JSON.stringify(map));
|
||||
const result = selectTests({
|
||||
changedFiles: ['packages/workflow/tsconfig.json'],
|
||||
mapFile: mapPath,
|
||||
allSpecsFile: writeAllSpecs(ALL_SPECS.join('\n')),
|
||||
tsconfigs: {
|
||||
'packages/workflow/tsconfig.json': {
|
||||
before: JSON.stringify({ compilerOptions: { paths: { 'esprima-next': ['./x'] } } }),
|
||||
after: JSON.stringify({
|
||||
compilerOptions: {
|
||||
paths: { 'n8n-workflow': ['./src/index.ts'], 'esprima-next': ['./x'] },
|
||||
},
|
||||
}),
|
||||
},
|
||||
},
|
||||
});
|
||||
expect(result.mode).toBe('broad');
|
||||
expect(result.specs).toEqual([...ALL_SPECS].sort());
|
||||
expect(result.uncovered).toBeUndefined();
|
||||
});
|
||||
|
||||
it('tsconfig resolution-neutral change (strict) → dropped, source still scopes', () => {
|
||||
const map: ImpactMap = { 'packages/cli/src/x.ts': { '10': ['tests/e2e/a.spec.ts'] } };
|
||||
const mapPath = path.join(tempDir, 'map-tsconfig-neutral.json');
|
||||
fs.writeFileSync(mapPath, JSON.stringify(map));
|
||||
const result = selectTests({
|
||||
changedFiles: ['packages/cli/tsconfig.json', 'packages/cli/src/x.ts'],
|
||||
mapFile: mapPath,
|
||||
allSpecsFile: writeAllSpecs(ALL_SPECS.join('\n')),
|
||||
tsconfigs: {
|
||||
'packages/cli/tsconfig.json': {
|
||||
before: JSON.stringify({ compilerOptions: { strict: true } }),
|
||||
after: JSON.stringify({ compilerOptions: { strict: false } }),
|
||||
},
|
||||
},
|
||||
});
|
||||
expect(result.mode).toBe('scoped');
|
||||
expect(result.specs).toEqual(['tests/e2e/a.spec.ts']);
|
||||
expect(result.uncovered).toBeUndefined();
|
||||
});
|
||||
|
||||
it('tsconfig change with no diff metadata → broad (cannot classify)', () => {
|
||||
const map: ImpactMap = { 'packages/cli/src/x.ts': { '10': ['tests/e2e/a.spec.ts'] } };
|
||||
const mapPath = path.join(tempDir, 'map-tsconfig-nometa.json');
|
||||
fs.writeFileSync(mapPath, JSON.stringify(map));
|
||||
const result = selectTests({
|
||||
changedFiles: ['packages/workflow/tsconfig.json'],
|
||||
mapFile: mapPath,
|
||||
allSpecsFile: writeAllSpecs(ALL_SPECS.join('\n')),
|
||||
});
|
||||
expect(result.mode).toBe('broad');
|
||||
expect(result.specs).toEqual([...ALL_SPECS].sort());
|
||||
});
|
||||
|
||||
describe('--all-specs parsing', () => {
|
||||
const triggerBroad = (allSpecsFile: string) =>
|
||||
selectTests({
|
||||
|
|
|
|||
|
|
@ -18,7 +18,9 @@ import {
|
|||
dropDevDepOnlyDeps,
|
||||
filterImpactfulChanges,
|
||||
forcesBroad,
|
||||
isTsconfig,
|
||||
stripDependencyFiles,
|
||||
tsconfigForcesBroad,
|
||||
} from './changes.js';
|
||||
import type { WorkspaceImporters } from './dep-graph.js';
|
||||
import {
|
||||
|
|
@ -44,6 +46,9 @@ export interface SelectTestsInput {
|
|||
* the lockfile + manifests from selection — a devDep can't reach the runtime
|
||||
* bundle the E2E suite exercises. */
|
||||
manifests?: Record<string, { before: string; after: string }>;
|
||||
/** before/after content of each changed tsconfig (caller reads from git).
|
||||
* Fed to {@link tsconfigForcesBroad}; omitted (local dev) → conservative broad. */
|
||||
tsconfigs?: Record<string, { before: string; after: string }>;
|
||||
/** Workspace package dir → runtime dependency names it declares (parsed from
|
||||
* pnpm-lock.yaml's `importers`). With `manifests`, a changed runtime dep is
|
||||
* walked to its declaring packages and scoped via the map instead
|
||||
|
|
@ -110,6 +115,18 @@ export function selectTests(input: SelectTestsInput): SelectTestsResult {
|
|||
const forcing = impactful.filter(forcesBroad);
|
||||
if (forcing.length > 0) return broad(forcing);
|
||||
|
||||
// A resolution-changing tsconfig edit forces broad; a resolution-neutral one
|
||||
// is dropped (see tsconfigForcesBroad). No diff metadata → conservative broad.
|
||||
const tsconfigChanges = impactful.filter(isTsconfig);
|
||||
if (tsconfigChanges.length > 0) {
|
||||
const forcingTsconfig = tsconfigChanges.filter((f) => {
|
||||
const diff = input.tsconfigs?.[f];
|
||||
return !diff || tsconfigForcesBroad(diff.before, diff.after);
|
||||
});
|
||||
if (forcingTsconfig.length > 0) return broad(forcingTsconfig);
|
||||
impactful = impactful.filter((f) => !isTsconfig(f));
|
||||
}
|
||||
|
||||
// devDependency-only change can't reach the runtime bundle → dropped.
|
||||
if (input.manifests) impactful = dropDevDepOnlyDeps(impactful, input.manifests);
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue
Block a user