mirror of
https://github.com/n8n-io/n8n.git
synced 2026-09-21 04:37:50 +08:00
ci: Wire workflow/core through janitor + fix global-trigger scope gap (DEVP-195) (#31415)
Co-authored-by: n8n-cat-bot[bot] <n8n-cat-bot[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> Co-authored-by: Declan Carroll <declan@n8n.io>
This commit is contained in:
co-authored by
n8n-cat-bot[bot]
Claude Opus 4.7
Declan Carroll
parent
e620545c93
commit
9e39914cf4
@@ -22,6 +22,7 @@
|
||||
"watch": "tsc-watch -p tsconfig.build.json --onCompilationComplete \"tsc-alias -p tsconfig.build.json\"",
|
||||
"test": "vitest run",
|
||||
"test:unit": "vitest run",
|
||||
"test:changed": "janitor test-scoped --runner=vitest",
|
||||
"test:dev": "vitest --silent=false"
|
||||
},
|
||||
"files": [
|
||||
@@ -30,6 +31,7 @@
|
||||
],
|
||||
"devDependencies": {
|
||||
"@n8n/errors": "workspace:*",
|
||||
"@n8n/playwright-janitor": "workspace:*",
|
||||
"@n8n/typescript-config": "workspace:*",
|
||||
"@types/express": "catalog:",
|
||||
"@types/jsonwebtoken": "catalog:",
|
||||
|
||||
@@ -243,6 +243,28 @@ plus setup files at `<pkg>/jest.setup.*`, `<pkg>/vitest.setup.*`, and
|
||||
`../cli/src/public-api/v1/**/*.yml` is honoured — a change to that yml
|
||||
marks nodes-base as affected.
|
||||
|
||||
**Global triggers force a full workspace run.** Some changes are invisible to
|
||||
a per-package import-graph walk: a lockfile / root-manifest change, or an edit
|
||||
to a universal sink (`packages/@n8n/db`, `packages/workflow`, `packages/core`)
|
||||
whose runtime coupling to downstream packages isn't expressed as a static
|
||||
import the test file can see. For these, scoping to "files in this package"
|
||||
would find nothing and emit `SKIP` on every downstream — a silent false green.
|
||||
|
||||
The trigger list lives in one place, `core/global-triggers.ts`
|
||||
(`GLOBAL_TRIGGER_FILES` for exact filenames, `GLOBAL_TRIGGER_PREFIXES` for
|
||||
directories), and is consulted at **both** layers of the pipeline:
|
||||
|
||||
* `affectedPackages()` returns every package, so all jobs are listed as
|
||||
affected; and
|
||||
* `computeScope()` returns `RUN_FULL` for the package, so each job actually
|
||||
runs its full suite instead of skipping.
|
||||
|
||||
Both checks are required — `affectedPackages` alone only decides which jobs are
|
||||
*listed*; without the `computeScope` check the job would still `SKIP`. The
|
||||
trade-off is over-testing on the rare PRs that touch these paths (the failure
|
||||
mode is "ran too much", never "ran nothing"). To add a new universal sink, add
|
||||
its directory prefix to `GLOBAL_TRIGGER_PREFIXES`.
|
||||
|
||||
## Rules
|
||||
|
||||
### Architecture Rules
|
||||
|
||||
@@ -76,18 +76,41 @@ describe('affectedPackages', () => {
|
||||
});
|
||||
|
||||
it('includes transitive downstream packages', () => {
|
||||
// Uses non-global-trigger package names so this exercises the dep-graph
|
||||
// walk, not the workspace-wide bailout (workflow/core ARE global triggers).
|
||||
const rootDir = makeFixture({
|
||||
patterns: ['packages/*'],
|
||||
packages: {
|
||||
'packages/workflow': { name: 'workflow' },
|
||||
'packages/core': { name: 'core', deps: ['workflow'] },
|
||||
'packages/cli': { name: 'cli', deps: ['core'] },
|
||||
'packages/lib': { name: 'lib' },
|
||||
'packages/mid': { name: 'mid', deps: ['lib'] },
|
||||
'packages/app': { name: 'app', deps: ['mid'] },
|
||||
'packages/unrelated': { name: 'unrelated' },
|
||||
},
|
||||
});
|
||||
expect(affectedPackages({ rootDir, changedFiles: ['packages/workflow/src/index.ts'] })).toEqual(
|
||||
['cli', 'core', 'workflow'],
|
||||
);
|
||||
expect(affectedPackages({ rootDir, changedFiles: ['packages/lib/src/index.ts'] })).toEqual([
|
||||
'app',
|
||||
'lib',
|
||||
'mid',
|
||||
]);
|
||||
});
|
||||
|
||||
it('expands all packages when a universal sink (workflow/core) changes', () => {
|
||||
const rootDir = makeFixture({
|
||||
patterns: ['packages/*'],
|
||||
packages: {
|
||||
'packages/workflow': { name: 'n8n-workflow' },
|
||||
'packages/core': { name: 'n8n-core' },
|
||||
'packages/unrelated': { name: 'unrelated' },
|
||||
},
|
||||
});
|
||||
expect(
|
||||
affectedPackages({ rootDir, changedFiles: ['packages/workflow/src/Workflow.ts'] }),
|
||||
).toEqual(['n8n-core', 'n8n-workflow', 'unrelated']);
|
||||
expect(affectedPackages({ rootDir, changedFiles: ['packages/core/src/x.ts'] })).toEqual([
|
||||
'n8n-core',
|
||||
'n8n-workflow',
|
||||
'unrelated',
|
||||
]);
|
||||
});
|
||||
|
||||
it('expands all packages when pnpm-lock.yaml changes', () => {
|
||||
|
||||
@@ -10,6 +10,7 @@ import { existsSync, readFileSync } from 'node:fs';
|
||||
import { join, relative } from 'node:path';
|
||||
import { parse as parseYaml } from 'yaml';
|
||||
|
||||
import { matchesGlobalTrigger } from './global-triggers.js';
|
||||
import { findWorkspaceRoot, toPosix } from './path-utils.js';
|
||||
|
||||
function parseJsonFile<T>(path: string): T {
|
||||
@@ -34,18 +35,6 @@ export interface AnalyzeOptions {
|
||||
changedFiles: string[] | null;
|
||||
}
|
||||
|
||||
const GLOBAL_TRIGGER_FILES = new Set(['pnpm-lock.yaml', 'package.json']);
|
||||
// Directory prefixes whose contents force RUN_FULL across the workspace. Used
|
||||
// when a package's contract is consumed at runtime by every other package and
|
||||
// the workspace symlink/dep-graph alone can't catch the coupling:
|
||||
// - packages/@n8n/db — schema + entities resolved by cli integration tests
|
||||
// through the DI container at runtime; jest --findRelatedTests on the
|
||||
// test file alone wouldn't see the migration/entity file in the import
|
||||
// graph and would silently SKIP. Bailing the workspace is over-broad on
|
||||
// rare db PRs but keeps the failure mode "ran too much" rather than
|
||||
// "ran nothing".
|
||||
const GLOBAL_TRIGGER_PREFIXES = ['packages/@n8n/db/'];
|
||||
|
||||
function loadWorkspacePackages(rootDir: string): WorkspacePackage[] {
|
||||
const wsFile = join(rootDir, 'pnpm-workspace.yaml');
|
||||
if (!existsSync(wsFile)) throw new Error(`pnpm-workspace.yaml not found at ${wsFile}`);
|
||||
@@ -131,14 +120,7 @@ export function affectedPackages(options: AnalyzeOptions): string[] {
|
||||
// No signal (local dev, missing env) → safest default: everything.
|
||||
if (options.changedFiles === null) return allNames;
|
||||
|
||||
if (
|
||||
options.changedFiles.some(
|
||||
(f) =>
|
||||
GLOBAL_TRIGGER_FILES.has(f) ||
|
||||
GLOBAL_TRIGGER_PREFIXES.some((prefix) => f.startsWith(prefix)),
|
||||
)
|
||||
)
|
||||
return allNames;
|
||||
if (options.changedFiles.some(matchesGlobalTrigger)) return allNames;
|
||||
|
||||
const direct = new Set<string>();
|
||||
for (const file of options.changedFiles) {
|
||||
|
||||
@@ -0,0 +1,41 @@
|
||||
/**
|
||||
* Files and directory prefixes whose changes force RUN_FULL across the whole
|
||||
* workspace.
|
||||
*
|
||||
* These MUST be consulted at BOTH layers of the scoping pipeline:
|
||||
* - `affectedPackages()` — returns every package so all test jobs are listed
|
||||
* as affected and provision a runner.
|
||||
* - `computeScope()` — returns RUN_FULL so each job actually executes its
|
||||
* full suite instead of SKIPping on "no changed files in package".
|
||||
*
|
||||
* Both checks are required. `affectedPackages` alone only decides which jobs
|
||||
* are *listed*; without the `computeScope` check a global-trigger change makes
|
||||
* every scoped package SKIP — running zero tests and reporting a false green.
|
||||
*/
|
||||
export const GLOBAL_TRIGGER_FILES = new Set(['pnpm-lock.yaml', 'package.json']);
|
||||
|
||||
/**
|
||||
* Directory prefixes consumed at runtime by packages that don't import them in
|
||||
* a way the test runner's import-graph walk can see. A change here is invisible
|
||||
* to `jest --findRelatedTests` / `vitest related` on the test file, so we bail
|
||||
* the workspace to full rather than silently skip:
|
||||
* - `packages/@n8n/db/` — schema + entities resolved through the DI container
|
||||
* by every consuming package's integration tests.
|
||||
* - `packages/workflow/`, `packages/core/` — universal sinks imported by
|
||||
* ~everything; a behaviour change that keeps the same type signature is not
|
||||
* visible to a downstream import-graph walk (typecheck only catches the
|
||||
* contract). DEVP-195.
|
||||
*
|
||||
* Over-broad on the rare PRs that touch these (keeps the failure mode "ran too
|
||||
* much" rather than "ran nothing"), which is the intended trade-off.
|
||||
*/
|
||||
export const GLOBAL_TRIGGER_PREFIXES = [
|
||||
'packages/@n8n/db/',
|
||||
'packages/workflow/',
|
||||
'packages/core/',
|
||||
];
|
||||
|
||||
/** True when a repo-root-relative path forces a full workspace run. */
|
||||
export function matchesGlobalTrigger(file: string): boolean {
|
||||
return GLOBAL_TRIGGER_FILES.has(file) || GLOBAL_TRIGGER_PREFIXES.some((p) => file.startsWith(p));
|
||||
}
|
||||
@@ -252,4 +252,58 @@ describe('computeScope', () => {
|
||||
expect(result.kind).toBe('scoped');
|
||||
});
|
||||
});
|
||||
|
||||
describe('global triggers force RUN_FULL', () => {
|
||||
it.each([
|
||||
['pnpm-lock.yaml', 'pnpm-lock.yaml'],
|
||||
['root package.json', 'package.json'],
|
||||
['@n8n/db entity', 'packages/@n8n/db/src/entities/user.entity.ts'],
|
||||
['workflow source', 'packages/workflow/src/Workflow.ts'],
|
||||
['core source', 'packages/core/src/x.ts'],
|
||||
])('bails to full on %s even when nothing changed in-package', (_label, changed) => {
|
||||
const rootDir = makePackageDir('packages/cli');
|
||||
const result = computeScope({
|
||||
runner: 'jest',
|
||||
jestVariant: 'integration',
|
||||
packageDir: 'packages/cli',
|
||||
rootDir,
|
||||
changedFiles: [changed],
|
||||
});
|
||||
expect(result.kind).toBe('full');
|
||||
expect(formatScope(result)).toBe('RUN_FULL');
|
||||
});
|
||||
|
||||
it('a universal-sink change forces full for a vitest downstream too', () => {
|
||||
const rootDir = makePackageDir('packages/nodes-base');
|
||||
const result = computeScope({
|
||||
runner: 'vitest',
|
||||
packageDir: 'packages/nodes-base',
|
||||
rootDir,
|
||||
changedFiles: ['packages/workflow/src/Workflow.ts'],
|
||||
});
|
||||
expect(result.kind).toBe('full');
|
||||
});
|
||||
|
||||
it('does NOT treat a per-package package.json as a global trigger', () => {
|
||||
const rootDir = makePackageDir('packages/cli');
|
||||
const result = computeScope({
|
||||
runner: 'jest',
|
||||
packageDir: 'packages/cli',
|
||||
rootDir,
|
||||
changedFiles: ['packages/other/package.json'],
|
||||
});
|
||||
expect(result.kind).toBe('skip');
|
||||
});
|
||||
|
||||
it('still SKIPs an unrelated cross-package change', () => {
|
||||
const rootDir = makePackageDir('packages/cli');
|
||||
const result = computeScope({
|
||||
runner: 'jest',
|
||||
packageDir: 'packages/cli',
|
||||
rootDir,
|
||||
changedFiles: ['packages/nodes-base/nodes/Slack/Slack.node.ts'],
|
||||
});
|
||||
expect(result.kind).toBe('skip');
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -8,6 +8,7 @@
|
||||
import { existsSync } from 'node:fs';
|
||||
import { isAbsolute, relative, resolve } from 'node:path';
|
||||
|
||||
import { matchesGlobalTrigger } from './global-triggers.js';
|
||||
import { toPosix } from './path-utils.js';
|
||||
|
||||
export type Runner = 'jest' | 'vitest';
|
||||
@@ -95,6 +96,16 @@ export function computeScope(options: ComputeScopeOptions): ScopeResult {
|
||||
return { kind: 'full', reason: 'No CHANGED_FILES signal (local dev)' };
|
||||
}
|
||||
|
||||
// Workspace-wide triggers (lockfile, root manifest, universal sinks like
|
||||
// @n8n/db / workflow / core) force RUN_FULL regardless of which package we
|
||||
// are scoping. The dep-graph in affected-packages lists the package as
|
||||
// affected, but the in-package filter below would otherwise SKIP it because
|
||||
// the trigger file lives outside the package — a silent false green.
|
||||
const globalTrigger = options.changedFiles.find(matchesGlobalTrigger);
|
||||
if (globalTrigger) {
|
||||
return { kind: 'full', reason: 'Global trigger changed', trigger: globalTrigger };
|
||||
}
|
||||
|
||||
const absolute = isAbsolute(options.packageDir)
|
||||
? options.packageDir
|
||||
: resolve(options.rootDir, options.packageDir);
|
||||
|
||||
@@ -31,6 +31,7 @@
|
||||
"watch": "tsc --build tsconfig.build.esm.json tsconfig.build.cjs.json --watch",
|
||||
"test": "vitest run",
|
||||
"test:unit": "vitest run",
|
||||
"test:changed": "janitor test-scoped --runner=vitest",
|
||||
"test:dev": "vitest --watch",
|
||||
"mutate": "node scripts/mutate.mjs"
|
||||
},
|
||||
@@ -40,6 +41,7 @@
|
||||
"devDependencies": {
|
||||
"@langchain/core": "catalog:",
|
||||
"@n8n/config": "workspace:*",
|
||||
"@n8n/playwright-janitor": "workspace:*",
|
||||
"@n8n/typescript-config": "workspace:*",
|
||||
"@n8n/vitest-config": "workspace:*",
|
||||
"@stryker-mutator/core": "catalog:",
|
||||
|
||||
Generated
+6
@@ -3369,6 +3369,9 @@ importers:
|
||||
'@n8n/errors':
|
||||
specifier: workspace:*
|
||||
version: link:../@n8n/errors
|
||||
'@n8n/playwright-janitor':
|
||||
specifier: workspace:*
|
||||
version: link:../testing/janitor
|
||||
'@n8n/typescript-config':
|
||||
specifier: workspace:*
|
||||
version: link:../@n8n/typescript-config
|
||||
@@ -5079,6 +5082,9 @@ importers:
|
||||
'@n8n/config':
|
||||
specifier: workspace:*
|
||||
version: link:../@n8n/config
|
||||
'@n8n/playwright-janitor':
|
||||
specifier: workspace:*
|
||||
version: link:../testing/janitor
|
||||
'@n8n/typescript-config':
|
||||
specifier: workspace:*
|
||||
version: link:../@n8n/typescript-config
|
||||
|
||||
Reference in New Issue
Block a user