From 04c1182a1e4441f8c6c7b674c117098fee12b8d5 Mon Sep 17 00:00:00 2001 From: Eugene Date: Tue, 7 Jan 2025 10:00:06 +0100 Subject: [PATCH] feat(core): Use node IDs instead of names for data mocking during test runs (no-changelog) (#12348) --- .../databases/entities/test-definition.ee.ts | 3 +- .../evaluation.ee/test-definition.schema.ts | 2 +- .../test-definition.service.ee.ts | 19 ++++- .../__tests__/create-pin-data.ee.test.ts | 58 ++++++++++++-- .../workflow.under-test-renamed-nodes.json | 78 +++++++++++++++++++ .../__tests__/test-runner.service.ee.test.ts | 12 ++- .../test-runner/test-runner.service.ee.ts | 12 ++- .../src/evaluation.ee/test-runner/utils.ee.ts | 28 +++++-- .../evaluation/test-definitions.api.test.ts | 3 +- 9 files changed, 195 insertions(+), 20 deletions(-) create mode 100644 packages/cli/src/evaluation.ee/test-runner/__tests__/mock-data/workflow.under-test-renamed-nodes.json diff --git a/packages/cli/src/databases/entities/test-definition.ee.ts b/packages/cli/src/databases/entities/test-definition.ee.ts index a7cb393b5d..bcbf701e79 100644 --- a/packages/cli/src/databases/entities/test-definition.ee.ts +++ b/packages/cli/src/databases/entities/test-definition.ee.ts @@ -9,7 +9,8 @@ import { jsonColumnType, WithTimestampsAndStringId } from './abstract-entity'; // Entity representing a node in a workflow under test, for which data should be mocked during test execution export type MockedNodeItem = { - name: string; + name?: string; + id: string; }; /** diff --git a/packages/cli/src/evaluation.ee/test-definition.schema.ts b/packages/cli/src/evaluation.ee/test-definition.schema.ts index 7760ae9dac..a2499f995c 100644 --- a/packages/cli/src/evaluation.ee/test-definition.schema.ts +++ b/packages/cli/src/evaluation.ee/test-definition.schema.ts @@ -16,6 +16,6 @@ export const testDefinitionPatchRequestBodySchema = z description: z.string().optional(), evaluationWorkflowId: z.string().min(1).optional(), annotationTagId: z.string().min(1).optional(), - mockedNodes: z.array(z.object({ name: z.string() })).optional(), + mockedNodes: z.array(z.object({ id: z.string(), name: z.string() })).optional(), }) .strict(); diff --git a/packages/cli/src/evaluation.ee/test-definition.service.ee.ts b/packages/cli/src/evaluation.ee/test-definition.service.ee.ts index 75fbeedfe9..ed393421b0 100644 --- a/packages/cli/src/evaluation.ee/test-definition.service.ee.ts +++ b/packages/cli/src/evaluation.ee/test-definition.service.ee.ts @@ -121,13 +121,26 @@ export class TestDefinitionService { relations: ['workflow'], }); - const existingNodeNames = new Set(existingTestDefinition.workflow.nodes.map((n) => n.name)); + const existingNodeNames = new Map( + existingTestDefinition.workflow.nodes.map((n) => [n.name, n]), + ); + const existingNodeIds = new Map(existingTestDefinition.workflow.nodes.map((n) => [n.id, n])); attrs.mockedNodes.forEach((node) => { - if (!existingNodeNames.has(node.name)) { - throw new BadRequestError(`Pinned node not found in the workflow: ${node.name}`); + if (!existingNodeIds.has(node.id) || (node.name && !existingNodeNames.has(node.name))) { + throw new BadRequestError( + `Pinned node not found in the workflow: ${node.id} (${node.name})`, + ); } }); + + // Update the node names OR node ids if they are not provided + attrs.mockedNodes = attrs.mockedNodes.map((node) => { + return { + id: node.id ?? (node.name && existingNodeNames.get(node.name)?.id), + name: node.name ?? (node.id && existingNodeIds.get(node.id)?.name), + }; + }); } // Update the test definition diff --git a/packages/cli/src/evaluation.ee/test-runner/__tests__/create-pin-data.ee.test.ts b/packages/cli/src/evaluation.ee/test-runner/__tests__/create-pin-data.ee.test.ts index 685c15552b..9603220432 100644 --- a/packages/cli/src/evaluation.ee/test-runner/__tests__/create-pin-data.ee.test.ts +++ b/packages/cli/src/evaluation.ee/test-runner/__tests__/create-pin-data.ee.test.ts @@ -7,13 +7,24 @@ const wfUnderTestJson = JSON.parse( readFileSync(path.join(__dirname, './mock-data/workflow.under-test.json'), { encoding: 'utf-8' }), ); +const wfUnderTestRenamedNodesJson = JSON.parse( + readFileSync(path.join(__dirname, './mock-data/workflow.under-test-renamed-nodes.json'), { + encoding: 'utf-8', + }), +); + const executionDataJson = JSON.parse( readFileSync(path.join(__dirname, './mock-data/execution-data.json'), { encoding: 'utf-8' }), ); describe('createPinData', () => { test('should create pin data from past execution data', () => { - const mockedNodes = ['When clicking ‘Test workflow’'].map((name) => ({ name })); + const mockedNodes = [ + { + id: '72256d90-3a67-4e29-b032-47df4e5768af', + name: 'When clicking ‘Test workflow’', + }, + ]; const pinData = createPinData(wfUnderTestJson, mockedNodes, executionDataJson); @@ -25,7 +36,7 @@ describe('createPinData', () => { }); test('should not create pin data for non-existing mocked nodes', () => { - const mockedNodes = ['Non-existing node'].map((name) => ({ name })); + const mockedNodes = ['non-existing-ID'].map((id) => ({ id })); const pinData = createPinData(wfUnderTestJson, mockedNodes, executionDataJson); @@ -33,9 +44,17 @@ describe('createPinData', () => { }); test('should create pin data for all mocked nodes', () => { - const mockedNodes = ['When clicking ‘Test workflow’', 'Edit Fields', 'Code'].map((name) => ({ - name, - })); + const mockedNodes = [ + { + id: '72256d90-3a67-4e29-b032-47df4e5768af', // 'When clicking ‘Test workflow’' + }, + { + id: '319f29bc-1dd4-4122-b223-c584752151a4', // 'Edit Fields' + }, + { + id: 'd2474215-63af-40a4-a51e-0ea30d762621', // 'Code' + }, + ]; const pinData = createPinData(wfUnderTestJson, mockedNodes, executionDataJson); @@ -53,4 +72,33 @@ describe('createPinData', () => { expect(pinData).toEqual({}); }); + + test('should create pin data for all mocked nodes with renamed nodes', () => { + const mockedNodes = [ + { + id: '72256d90-3a67-4e29-b032-47df4e5768af', // 'Manual Run' + }, + { + id: '319f29bc-1dd4-4122-b223-c584752151a4', // 'Set Attribute' + }, + { + id: 'd2474215-63af-40a4-a51e-0ea30d762621', // 'Code' + }, + ]; + + const pinData = createPinData( + wfUnderTestRenamedNodesJson, + mockedNodes, + executionDataJson, + wfUnderTestJson, // Pass original workflow JSON as pastWorkflowData + ); + + expect(pinData).toEqual( + expect.objectContaining({ + 'Manual Run': expect.anything(), + 'Set Attribute': expect.anything(), + Code: expect.anything(), + }), + ); + }); }); diff --git a/packages/cli/src/evaluation.ee/test-runner/__tests__/mock-data/workflow.under-test-renamed-nodes.json b/packages/cli/src/evaluation.ee/test-runner/__tests__/mock-data/workflow.under-test-renamed-nodes.json new file mode 100644 index 0000000000..d384687feb --- /dev/null +++ b/packages/cli/src/evaluation.ee/test-runner/__tests__/mock-data/workflow.under-test-renamed-nodes.json @@ -0,0 +1,78 @@ +{ + "name": "Workflow Under Test", + "nodes": [ + { + "parameters": {}, + "type": "n8n-nodes-base.manualTrigger", + "typeVersion": 1, + "position": [-80, 0], + "id": "72256d90-3a67-4e29-b032-47df4e5768af", + "name": "Manual Run" + }, + { + "parameters": { + "assignments": { + "assignments": [ + { + "id": "acfeecbe-443c-4220-b63b-d44d69216902", + "name": "foo", + "value": "bar", + "type": "string" + } + ] + }, + "options": {} + }, + "type": "n8n-nodes-base.set", + "typeVersion": 3.4, + "position": [140, 0], + "id": "319f29bc-1dd4-4122-b223-c584752151a4", + "name": "Set Attribute" + }, + { + "parameters": { + "jsCode": "for (const item of $input.all()) {\n item.json.random = Math.random();\n}\n\nreturn $input.all();" + }, + "type": "n8n-nodes-base.code", + "typeVersion": 2, + "position": [380, 0], + "id": "d2474215-63af-40a4-a51e-0ea30d762621", + "name": "Code" + } + ], + "connections": { + "Manual Run": { + "main": [ + [ + { + "node": "Set attribute", + "type": "main", + "index": 0 + } + ] + ] + }, + "Set attribute": { + "main": [ + [ + { + "node": "Wait", + "type": "main", + "index": 0 + } + ] + ] + }, + "Wait": { + "main": [ + [ + { + "node": "Code", + "type": "main", + "index": 0 + } + ] + ] + } + } +} diff --git a/packages/cli/src/evaluation.ee/test-runner/__tests__/test-runner.service.ee.test.ts b/packages/cli/src/evaluation.ee/test-runner/__tests__/test-runner.service.ee.test.ts index f923cb4b73..06b9653374 100644 --- a/packages/cli/src/evaluation.ee/test-runner/__tests__/test-runner.service.ee.test.ts +++ b/packages/cli/src/evaluation.ee/test-runner/__tests__/test-runner.service.ee.test.ts @@ -27,6 +27,12 @@ const wfUnderTestJson = JSON.parse( readFileSync(path.join(__dirname, './mock-data/workflow.under-test.json'), { encoding: 'utf-8' }), ); +const wfUnderTestRenamedNodesJson = JSON.parse( + readFileSync(path.join(__dirname, './mock-data/workflow.under-test-renamed-nodes.json'), { + encoding: 'utf-8', + }), +); + const wfEvaluationJson = JSON.parse( readFileSync(path.join(__dirname, './mock-data/workflow.evaluation.json'), { encoding: 'utf-8' }), ); @@ -60,6 +66,7 @@ const executionMocks = [ status: 'success', executionData: { data: stringify(executionDataJson), + workflowData: wfUnderTestJson, }, }), mock({ @@ -68,6 +75,7 @@ const executionMocks = [ status: 'success', executionData: { data: stringify(executionDataJson), + workflowData: wfUnderTestRenamedNodesJson, }, }), ]; @@ -250,7 +258,7 @@ describe('TestRunnerService', () => { mock({ workflowId: 'workflow-under-test-id', evaluationWorkflowId: 'evaluation-workflow-id', - mockedNodes: [{ name: 'When clicking ‘Test workflow’' }], + mockedNodes: [{ id: '72256d90-3a67-4e29-b032-47df4e5768af' }], }), ); @@ -347,7 +355,7 @@ describe('TestRunnerService', () => { mock({ workflowId: 'workflow-under-test-id', evaluationWorkflowId: 'evaluation-workflow-id', - mockedNodes: [{ name: 'When clicking ‘Test workflow’' }], + mockedNodes: [{ id: '72256d90-3a67-4e29-b032-47df4e5768af' }], }), ); diff --git a/packages/cli/src/evaluation.ee/test-runner/test-runner.service.ee.ts b/packages/cli/src/evaluation.ee/test-runner/test-runner.service.ee.ts index 65c6c2d300..926bc29b70 100644 --- a/packages/cli/src/evaluation.ee/test-runner/test-runner.service.ee.ts +++ b/packages/cli/src/evaluation.ee/test-runner/test-runner.service.ee.ts @@ -1,13 +1,14 @@ import { Service } from '@n8n/di'; import { parse } from 'flatted'; +import { NodeConnectionType, Workflow } from 'n8n-workflow'; import type { IDataObject, IRun, IRunData, IRunExecutionData, + IWorkflowBase, IWorkflowExecutionDataProcess, } from 'n8n-workflow'; -import { NodeConnectionType, Workflow } from 'n8n-workflow'; import assert from 'node:assert'; import { ActiveExecutions } from '@/active-executions'; @@ -94,11 +95,17 @@ export class TestRunnerService { private async runTestCase( workflow: WorkflowEntity, pastExecutionData: IRunExecutionData, + pastExecutionWorkflowData: IWorkflowBase, mockedNodes: MockedNodeItem[], userId: string, ): Promise { // Create pin data from the past execution data - const pinData = createPinData(workflow, mockedNodes, pastExecutionData); + const pinData = createPinData( + workflow, + mockedNodes, + pastExecutionData, + pastExecutionWorkflowData, + ); // Prepare the data to run the workflow const data: IWorkflowExecutionDataProcess = { @@ -235,6 +242,7 @@ export class TestRunnerService { const testCaseExecution = await this.runTestCase( workflow, executionData, + pastExecution.executionData.workflowData, test.mockedNodes, user.id, ); diff --git a/packages/cli/src/evaluation.ee/test-runner/utils.ee.ts b/packages/cli/src/evaluation.ee/test-runner/utils.ee.ts index 9cb516b430..be40b7ecfb 100644 --- a/packages/cli/src/evaluation.ee/test-runner/utils.ee.ts +++ b/packages/cli/src/evaluation.ee/test-runner/utils.ee.ts @@ -1,4 +1,5 @@ -import type { IRunExecutionData, IPinData } from 'n8n-workflow'; +import assert from 'assert'; +import type { IRunExecutionData, IPinData, IWorkflowBase } from 'n8n-workflow'; import type { MockedNodeItem } from '@/databases/entities/test-definition.ee'; import type { WorkflowEntity } from '@/databases/entities/workflow-entity'; @@ -13,16 +14,33 @@ export function createPinData( workflow: WorkflowEntity, mockedNodes: MockedNodeItem[], executionData: IRunExecutionData, + pastWorkflowData?: IWorkflowBase, ) { const pinData = {} as IPinData; - const workflowNodeNames = new Set(workflow.nodes.map((node) => node.name)); + const workflowNodeIds = new Map(workflow.nodes.map((node) => [node.id, node.name])); + + // If the past workflow data is provided, use it to create a map between node IDs and node names + const pastWorkflowNodeIds = new Map(); + if (pastWorkflowData) { + for (const node of pastWorkflowData.nodes) { + pastWorkflowNodeIds.set(node.id, node.name); + } + } for (const mockedNode of mockedNodes) { - if (workflowNodeNames.has(mockedNode.name)) { - const nodeData = executionData.resultData.runData[mockedNode.name]; + assert(mockedNode.id, 'Mocked node ID is missing'); + + const nodeName = workflowNodeIds.get(mockedNode.id); + + // If mocked node is still present in the workflow + if (nodeName) { + // Try to restore node name from past execution data (it might have been renamed between past execution and up-to-date workflow) + const pastNodeName = pastWorkflowNodeIds.get(mockedNode.id) ?? nodeName; + const nodeData = executionData.resultData.runData[pastNodeName]; + if (nodeData?.[0]?.data?.main?.[0]) { - pinData[mockedNode.name] = nodeData[0]?.data?.main?.[0]; + pinData[nodeName] = nodeData[0]?.data?.main?.[0]; } } } diff --git a/packages/cli/test/integration/evaluation/test-definitions.api.test.ts b/packages/cli/test/integration/evaluation/test-definitions.api.test.ts index ad359f8731..52bc72f6f9 100644 --- a/packages/cli/test/integration/evaluation/test-definitions.api.test.ts +++ b/packages/cli/test/integration/evaluation/test-definitions.api.test.ts @@ -405,13 +405,14 @@ describe('PATCH /evaluation/test-definitions/:id', () => { const resp = await authOwnerAgent.patch(`/evaluation/test-definitions/${newTest.id}`).send({ mockedNodes: [ { + id: 'uuid-1234', name: 'Schedule Trigger', }, ], }); expect(resp.statusCode).toBe(200); - expect(resp.body.data.mockedNodes).toEqual([{ name: 'Schedule Trigger' }]); + expect(resp.body.data.mockedNodes).toEqual([{ id: 'uuid-1234', name: 'Schedule Trigger' }]); }); test('should return error if pinned nodes are invalid', async () => {