From bc8fac9bd65cdabbf035ee4b5d4143889c42fb14 Mon Sep 17 00:00:00 2001 From: "n8n-cat-bot[bot]" Date: Wed, 23 Sep 2026 14:37:55 +0000 Subject: [PATCH] test(workflow): retire legacy expression test project --- packages/workflow/stryker.config.mjs | 5 +- .../test/EXPRESSION_ENGINE_VALIDATION.md | 49 +++++++++++++++++++ .../test/ExpressionExtensions/helpers.ts | 3 +- .../expression-array-proxy-semantics.test.ts | 49 +++++++------------ .../test/expression-compatibility.test.ts | 13 ++--- ...ession-item-accessor-engine-parity.test.ts | 3 +- .../expression-items-engine-parity.test.ts | 16 +++--- .../test/expression-nested-data.test.ts | 4 +- .../test/webhook-description-fields.test.ts | 3 +- packages/workflow/vitest.config.ts | 7 --- packages/workflow/vitest.stryker.config.ts | 13 ++--- 11 files changed, 86 insertions(+), 79 deletions(-) create mode 100644 packages/workflow/test/EXPRESSION_ENGINE_VALIDATION.md diff --git a/packages/workflow/stryker.config.mjs b/packages/workflow/stryker.config.mjs index d23fd03b304..c10ed426365 100644 --- a/packages/workflow/stryker.config.mjs +++ b/packages/workflow/stryker.config.mjs @@ -3,8 +3,7 @@ export default { packageManager: 'pnpm', testRunner: 'vitest', plugins: ['@stryker-mutator/vitest-runner'], - // Use the Stryker-specific vitest config (legacy-engine only) — see - // vitest.stryker.config.ts for the rationale. + // Use the QuickJS project to avoid VM teardown failures in Stryker workers. vitest: { configFile: 'vitest.stryker.config.ts', }, @@ -23,7 +22,7 @@ export default { jsonReporter: { fileName: 'reports/mutation/raw.json' }, // Cap low: a string/utils mutant running 15s+ is a spinning loop-condition mutant; fail it fast. timeoutMS: 15_000, - // Each Stryker worker spawns vitest with one project (legacy-engine, via + // Each Stryker worker spawns vitest with one project (QuickJS, via // vitest.stryker.config.ts). Default 4 is fine on CI runners; lower // locally if you hit OOM during the initial dry run via // `STRYKER_CONCURRENCY=1`. diff --git a/packages/workflow/test/EXPRESSION_ENGINE_VALIDATION.md b/packages/workflow/test/EXPRESSION_ENGINE_VALIDATION.md new file mode 100644 index 00000000000..22d0d1be657 --- /dev/null +++ b/packages/workflow/test/EXPRESSION_ENGINE_VALIDATION.md @@ -0,0 +1,49 @@ +# Expression engine test matrix + +The workflow Vitest suite runs on VM and QuickJS. Both projects run the same +expression contracts. The runtime `legacy` option remains available, but the +normal suite no longer compares each case against it. Stryker uses QuickJS +because VM teardown aborts in its workers (isolated-vm #464). + +## Local performance check + +Run from `packages/testing/performance`: + +```bash +pnpm bench benchmarks/expression-engine/patterns-vm.bench.ts benchmarks/expression-engine/patterns-legacy.bench.ts +``` + +One local run on Node 24.15.0 measured warm expression evaluation through +`Workflow.expression`. Lower mean latency is better. Results depend on the +runner; these numbers are not a CI performance baseline. + +| Workload | VM mean | Legacy mean | +| --- | ---: | ---: | +| Simple property, small data | 0.159 ms | 0.033 ms | +| Nested property, depth 4 | 0.185 ms | 0.034 ms | +| Extension call, `isEmpty` | 0.217 ms | 0.036 ms | +| Map 100 items | 0.830 ms | 0.037 ms | +| Filter 100 items | 1.701 ms | 0.036 ms | +| Map 10,000 items | 61.665 ms | 0.054 ms | + +VM is slower on these workloads, especially on array iteration. The ticket +does not state a numeric acceptance limit. This run does not establish that +VM meets a performance bar. Use the same benchmark on production-like hardware +and agree on a limit before treating performance as validated. + +## Compatibility check + +Before removing the legacy project, the compatibility corpus, array proxy, +and `$item(index)` suites passed on legacy, VM, and QuickJS (231 tests per +project). All three projects shared the same contract cases. The only +engine-specific expected result in that corpus was `Number.format()`: +legacy returned `undefined`; VM and QuickJS return the unformatted number +as a string. Array and object writes also differ: legacy changes the input +data, while VM and QuickJS keep writes within the evaluation. The VM and +QuickJS assertions still cover both behaviors they share. The matrix no +longer checks new changes against legacy automatically. + +After the change, `pnpm test` in `packages/workflow` passed 180 test files +across VM and QuickJS (6,521 passed; one skipped). The Stryker QuickJS +configuration passed all 90 test files (3,260 passed; one skipped). A narrow +Stryker run on `get-connected-nodes.ts:37` killed three of three mutants. diff --git a/packages/workflow/test/ExpressionExtensions/helpers.ts b/packages/workflow/test/ExpressionExtensions/helpers.ts index 669f0a5c6eb..bcba00e5019 100644 --- a/packages/workflow/test/ExpressionExtensions/helpers.ts +++ b/packages/workflow/test/ExpressionExtensions/helpers.ts @@ -23,8 +23,7 @@ export const workflow = new Workflow({ }); export const expression = workflow.expression; -// acquireIsolate/releaseIsolate are no-ops for the legacy engine, so these -// hooks are safe to register unconditionally. +// Acquire and release the isolate for both VM and QuickJS projects. beforeAll(async () => { await expression.acquireIsolate(); }); diff --git a/packages/workflow/test/expression-array-proxy-semantics.test.ts b/packages/workflow/test/expression-array-proxy-semantics.test.ts index 1cd440319db..4e71fd89752 100644 --- a/packages/workflow/test/expression-array-proxy-semantics.test.ts +++ b/packages/workflow/test/expression-array-proxy-semantics.test.ts @@ -4,9 +4,7 @@ import * as Helpers from './helpers'; import type { INodeExecutionData } from '../src/interfaces'; import { Workflow } from '../src/workflow'; -// Engine-parity tests for `$json` arrays beyond plain indexed access. Return -// values must match on both engines; mutation persistence intentionally -// diverges (see the isVm branches below). +// `$json` array behavior beyond plain indexed access on VM and QuickJS. describe('Expression — array proxy semantics (engine parity)', () => { const workflow = new Workflow({ @@ -39,7 +37,7 @@ describe('Expression — array proxy semantics (engine parity)', () => { return expression.getParameterValue(value, null, 0, 0, 'node', data, 'manual', {}); }; - // Both engines reject property-descriptor access from inside an expression: + // Both projects reject property-descriptor access from inside an expression: // `getOwnPropertyDescriptor` is on the sanitizer's blocklist, so the // expression is rejected before evaluation. Documented so a future // divergence is caught; neither engine intends to expose the data this way. @@ -81,58 +79,51 @@ describe('Expression — array proxy semantics (engine parity)', () => { ]); }); - // Mutating array methods run natively on both engines. Return values are - // identical; persistence intentionally diverges: the vm engine mutates an - // evaluation-scoped copy (fixture untouched), the legacy engine writes - // through to the underlying workflow data. + // Mutating array methods change only an evaluation-scoped copy. describe('mutating array methods follow native semantics', () => { - const isVm = process.env.N8N_EXPRESSION_ENGINE !== 'legacy'; - it('sort() returns the sorted array', () => { const json = { arr: ['Mango', 'Apple', 'Kiwi', 'Orange'] }; const sorted = ['Apple', 'Kiwi', 'Mango', 'Orange']; expect(evaluate('={{ $json.arr.sort() }}', json)).toEqual(sorted); - expect(json.arr).toEqual(isVm ? ['Mango', 'Apple', 'Kiwi', 'Orange'] : sorted); + expect(json.arr).toEqual(['Mango', 'Apple', 'Kiwi', 'Orange']); }); it('sort() forwards the comparator', () => { const json = { arr: [3, 1, 10, 2] }; expect(evaluate('={{ $json.arr.sort((a, b) => b - a) }}', json)).toEqual([10, 3, 2, 1]); - expect(json.arr).toEqual(isVm ? [3, 1, 10, 2] : [10, 3, 2, 1]); + expect(json.arr).toEqual([3, 1, 10, 2]); }); it('splice() returns the removed elements', () => { const json = { arr: ['Mango', 'Apple', 'Kiwi', 'Orange'] }; expect(evaluate('={{ $json.arr.splice(0, 2) }}', json)).toEqual(['Mango', 'Apple']); - expect(json.arr).toEqual(isVm ? ['Mango', 'Apple', 'Kiwi', 'Orange'] : ['Kiwi', 'Orange']); + expect(json.arr).toEqual(['Mango', 'Apple', 'Kiwi', 'Orange']); }); it('fill() returns the filled array', () => { const json = { arr: ['Mango', 'Apple', 'Kiwi', 'Orange'] }; const filled = ['X', 'X', 'Kiwi', 'Orange']; expect(evaluate('={{ $json.arr.fill("X", 0, 2) }}', json)).toEqual(filled); - expect(json.arr).toEqual(isVm ? ['Mango', 'Apple', 'Kiwi', 'Orange'] : filled); + expect(json.arr).toEqual(['Mango', 'Apple', 'Kiwi', 'Orange']); }); it('shift() returns the removed first element', () => { const json = { arr: ['Mango', 'Apple', 'Kiwi'] }; expect(evaluate('={{ $json.arr.shift() }}', json)).toBe('Mango'); - expect(json.arr).toEqual(isVm ? ['Mango', 'Apple', 'Kiwi'] : ['Apple', 'Kiwi']); + expect(json.arr).toEqual(['Mango', 'Apple', 'Kiwi']); }); it('unshift() returns the new length', () => { const json = { arr: ['Mango', 'Apple', 'Kiwi'] }; expect(evaluate('={{ $json.arr.unshift("Peach", "Grape") }}', json)).toBe(5); - expect(json.arr).toEqual( - isVm ? ['Mango', 'Apple', 'Kiwi'] : ['Peach', 'Grape', 'Mango', 'Apple', 'Kiwi'], - ); + expect(json.arr).toEqual(['Mango', 'Apple', 'Kiwi']); }); it('copyWithin() returns the copied-within array', () => { const json = { arr: ['Mango', 'Apple', 'Kiwi', 'Orange'] }; const copied = ['Kiwi', 'Orange', 'Kiwi', 'Orange']; expect(evaluate('={{ $json.arr.copyWithin(0, 2, 4) }}', json)).toEqual(copied); - expect(json.arr).toEqual(isVm ? ['Mango', 'Apple', 'Kiwi', 'Orange'] : copied); + expect(json.arr).toEqual(['Mango', 'Apple', 'Kiwi', 'Orange']); }); it('mutation is visible to a subsequent sibling read', () => { @@ -186,20 +177,14 @@ describe('Expression — array proxy semantics (engine parity)', () => { }); }); - // Direct writes on $json data. Both engines apply the write within the - // evaluation; they intentionally diverge on persistence: the vm engine's - // proxies are copy-on-write scoped to a single evaluation, while the legacy - // engine writes through to the underlying workflow data (long-standing - // behaviour for non-scripting nodes, where data is not augmented). + // Direct writes on $json data stay within the current evaluation. describe('direct writes on $json data', () => { - const isVm = process.env.N8N_EXPRESSION_ENGINE !== 'legacy'; - it('index assignment is visible to a later read in the same evaluation', () => { const json = { arr: ['a', 'b', 'c'] }; expect(evaluate('={{ (() => { $json.arr[0] = "X"; return $json.arr[0]; })() }}', json)).toBe( 'X', ); - expect(json.arr).toEqual(isVm ? ['a', 'b', 'c'] : ['X', 'b', 'c']); + expect(json.arr).toEqual(['a', 'b', 'c']); }); it('existing object-key assignment is visible to a later read', () => { @@ -207,7 +192,7 @@ describe('Expression — array proxy semantics (engine parity)', () => { expect( evaluate('={{ (() => { $json.user.name = "Zed"; return $json.user.name; })() }}', json), ).toBe('Zed'); - expect(json.user.name).toBe(isVm ? 'Alice' : 'Zed'); + expect(json.user.name).toBe('Alice'); }); it('delete removes the key for the rest of the evaluation', () => { @@ -218,7 +203,7 @@ describe('Expression — array proxy semantics (engine parity)', () => { json, ), ).toBe(true); - expect('email' in json.user).toBe(isVm); + expect('email' in json.user).toBe(true); }); it('push() updates length within the evaluation', () => { @@ -226,13 +211,13 @@ describe('Expression — array proxy semantics (engine parity)', () => { expect( evaluate('={{ (() => { $json.arr.push("d"); return $json.arr.length; })() }}', json), ).toBe(4); - expect(json.arr.length).toBe(isVm ? 3 : 4); + expect(json.arr.length).toBe(3); }); - it('writes do not leak into a subsequent evaluation on the vm engine', () => { + it('writes do not leak into a subsequent evaluation', () => { const json = { arr: ['a', 'b', 'c'] }; evaluate('={{ (() => { $json.arr[0] = "X"; return $json.arr[0]; })() }}', json); - expect(evaluate('={{ $json.arr[0] }}', json)).toBe(isVm ? 'a' : 'X'); + expect(evaluate('={{ $json.arr[0] }}', json)).toBe('a'); }); }); }); diff --git a/packages/workflow/test/expression-compatibility.test.ts b/packages/workflow/test/expression-compatibility.test.ts index 344676f82af..355c868dc37 100644 --- a/packages/workflow/test/expression-compatibility.test.ts +++ b/packages/workflow/test/expression-compatibility.test.ts @@ -9,7 +9,7 @@ import { Workflow } from '../src/workflow'; // Compatibility corpus for expression syntax. // -// Both engines run every case (see `vitest.config.ts`). +// The VM and QuickJS projects run every case (see `vitest.config.ts`). describe('Expression — compatibility corpus', () => { const workflow = new Workflow({ @@ -251,8 +251,6 @@ describe('Expression — compatibility corpus', () => { $pageCount: 2, }; - const ENGINE = process.env.N8N_EXPRESSION_ENGINE ?? 'vm'; - const evaluate = (expr: string) => expression.getParameterValue( expr, @@ -882,12 +880,7 @@ describe('Expression — compatibility corpus', () => { expect(() => evaluate(expr)).toThrow(); }); - // ── known engine divergences ────────────────────────────────────────── - const ENGINE_DIVERGENCES: Array<[string, string, unknown, unknown]> = [ - ['Number.format()', "={{ (1234.5678).format('0,0.00') }}", undefined, '1234.5678'], - ]; - - it.each(ENGINE_DIVERGENCES)('divergent: %s', (_name, expr, legacyWant, vmWant) => { - expect(evaluate(expr)).toStrictEqual(ENGINE === 'legacy' ? legacyWant : vmWant); + it('returns the original number from Number.format()', () => { + expect(evaluate("={{ (1234.5678).format('0,0.00') }}")).toBe('1234.5678'); }); }); diff --git a/packages/workflow/test/expression-item-accessor-engine-parity.test.ts b/packages/workflow/test/expression-item-accessor-engine-parity.test.ts index 64be434b54e..0c7a751aa9b 100644 --- a/packages/workflow/test/expression-item-accessor-engine-parity.test.ts +++ b/packages/workflow/test/expression-item-accessor-engine-parity.test.ts @@ -13,8 +13,7 @@ import { Workflow } from '../src/workflow'; // `$item(0).$nodeId` resolved to undefined at runtime while the legacy engine // (and the editor preview) returned the correct value (CAT-3496). // -// These cases run under both the legacy and VM engines (see `vitest.config.ts`) -// so any divergence fails exactly one project. +// VM and QuickJS run these contracts (see `vitest.config.ts`). describe('Expression — legacy $item(index) accessor surface (engine parity)', () => { const workflow = new Workflow({ diff --git a/packages/workflow/test/expression-items-engine-parity.test.ts b/packages/workflow/test/expression-items-engine-parity.test.ts index 525e6b7584b..35582f1d5d9 100644 --- a/packages/workflow/test/expression-items-engine-parity.test.ts +++ b/packages/workflow/test/expression-items-engine-parity.test.ts @@ -8,10 +8,8 @@ import { Workflow } from '../src/workflow'; // Engine-parity tests for `$items()` boundary behaviour. // // `$items()` was migrated to a typed-RPC under the VM engine in CAT-2982. -// This file pins the behaviour of edge-case `runIndex` values across both -// the legacy and VM engines so any divergence in how the runtime handles -// host-side errors fails exactly one project. See `vitest.config.ts` for -// the dual-engine setup. +// This file pins edge-case `runIndex` values on VM and QuickJS. +// See `vitest.config.ts` for the two projects. describe('Expression — $items() boundary behaviour (engine parity)', () => { const workflow = new Workflow({ @@ -97,19 +95,17 @@ describe('Expression — $items() boundary behaviour (engine parity)', () => { it('$items("Source", 0, 999) throws ExpressionError "Run X not found"', () => { // Host throws a structured ExpressionError when the runIndex is past - // the known length. Both engines should surface this as-is. + // the known length. Both projects should surface this as-is. expect(() => evaluate('={{ $items("Source", 0, 999) }}')).toThrowError(ExpressionError); }); - it('$items("Source", 0, -2) returns undefined — host TypeError is swallowed by both engines', () => { + it('$items("Source", 0, -2) returns undefined when the host TypeError is swallowed', () => { // Host's `getNodeExecutionData` only special-cases -1; any other // negative runIndex skips the `length <= runIndex` bounds check // (because length > negative) and dereferences `runData[name][-2].data`, - // which throws a plain TypeError. Both engines' E() handler swallows + // which throws a plain TypeError. Both projects' E() handler swallows // non-ExpressionError throws, so the observable result is undefined. - // If a future change makes the VM engine surface the TypeError as an - // error (instead of undefined), this test fails on the vm-engine - // project and the divergence is caught. + // If an engine starts surfacing the TypeError, this contract fails. expect(evaluate('={{ $items("Source", 0, -2) }}')).toBeUndefined(); }); }); diff --git a/packages/workflow/test/expression-nested-data.test.ts b/packages/workflow/test/expression-nested-data.test.ts index 91ff93f4f27..717494d33eb 100644 --- a/packages/workflow/test/expression-nested-data.test.ts +++ b/packages/workflow/test/expression-nested-data.test.ts @@ -5,9 +5,7 @@ import type { INodeExecutionData } from '../src/interfaces'; import { Workflow } from '../src/workflow'; // Feature-parity regressions for `$json` access across nested data shapes. -// This file lives in `packages/workflow/test/` so vitest runs it under both the -// `legacy-engine` and `vm-engine` projects (see vitest.config.ts). Any divergence -// between the two engines fails one project but not the other. +// Vitest runs these contracts under VM and QuickJS (see vitest.config.ts). describe('Expression — nested $json shapes (engine parity)', () => { const workflow = new Workflow({ diff --git a/packages/workflow/test/webhook-description-fields.test.ts b/packages/workflow/test/webhook-description-fields.test.ts index c841c93f58b..6be21f2bee6 100644 --- a/packages/workflow/test/webhook-description-fields.test.ts +++ b/packages/workflow/test/webhook-description-fields.test.ts @@ -79,8 +79,7 @@ const nodeWithParameters = (parameters: INodeParameters) => { return { workflow, node: workflow.getNode('Webhook')! }; }; -// This file runs under both vitest projects (legacy and vm), so parity is -// pinned against both engines; the vm engine needs an acquired isolate. +// Both VM and QuickJS need an acquired isolate for this expression path. const viaEngine = async (parameters: INodeParameters, template: string) => { const { workflow, node } = nodeWithParameters(parameters); await workflow.expression.acquireIsolate(); diff --git a/packages/workflow/vitest.config.ts b/packages/workflow/vitest.config.ts index a011d7ab377..eea29100b61 100644 --- a/packages/workflow/vitest.config.ts +++ b/packages/workflow/vitest.config.ts @@ -18,13 +18,6 @@ export default defineConfig({ env: { N8N_EXPRESSION_ENGINE: 'vm' }, }, }, - { - test: { - ...sharedTestConfig, - name: 'legacy-engine', - env: { N8N_EXPRESSION_ENGINE: 'legacy' }, - }, - }, { test: { ...sharedTestConfig, diff --git a/packages/workflow/vitest.stryker.config.ts b/packages/workflow/vitest.stryker.config.ts index 0ae053a3669..2870092aee0 100644 --- a/packages/workflow/vitest.stryker.config.ts +++ b/packages/workflow/vitest.stryker.config.ts @@ -1,16 +1,12 @@ -// Vitest config used by Stryker only — NOT by `pnpm test`, NOT by CI unit runs. -// -// Runs the legacy expression engine, not vm/isolated-vm: the vm evaluator -// SIGABRTs in Stryker's worker on teardown (upstream isolated-vm #464, repros on -// Node 22 + 24). Trade-off: vm-only branches in expression.ts — the production -// default engine — go unscored. Swap -// back to N8N_EXPRESSION_ENGINE=vm once #464 is fixed or we pnpm-patch the guard. +// Stryker uses QuickJS because VM teardown aborts in Stryker workers +// (isolated-vm #464 on Node 22 and 24). The normal suite uses VM and QuickJS. import { defineConfig } from 'vitest/config'; import { createBaseInlineConfig } from '@n8n/vitest-config/node'; const { reporters, outputFile, ...sharedTestConfig } = createBaseInlineConfig({ include: ['test/**/*.test.ts'], + setupFiles: ['./test/setup-vm-evaluator.ts'], }); export default defineConfig({ @@ -21,7 +17,8 @@ export default defineConfig({ { test: { ...sharedTestConfig, - name: 'legacy-engine', + name: 'quickjs-engine', + env: { N8N_EXPRESSION_ENGINE: 'quickjs' }, }, }, ],