From 0a2e29839c996a68d7fdda4924b21199dd085e19 Mon Sep 17 00:00:00 2001 From: Eugene Date: Thu, 13 Feb 2025 14:08:10 +0300 Subject: [PATCH] feat(core): Change evaluation workflow input data format (no-changelog) (#13109) --- .../test-definitions.controller.ee.ts | 16 +++ .../test-definitions.types.ee.ts | 7 ++ ...-test-case-execution-input-data.ee.test.ts | 84 ++++++++++++++++ .../__tests__/test-runner.service.ee.test.ts | 62 ++++++++++-- .../test-runner/test-runner.service.ee.ts | 97 +++++++++++++++---- .../src/evaluation.ee/test-runner/utils.ee.ts | 80 ++++++++++++++- .../editor-ui/src/api/testDefinition.ee.ts | 12 +++ .../src/stores/testDefinition.store.ee.ts | 9 ++ .../TestDefinition/TestDefinitionEditView.vue | 24 ++--- 9 files changed, 349 insertions(+), 42 deletions(-) create mode 100644 packages/cli/src/evaluation.ee/test-runner/__tests__/format-test-case-execution-input-data.ee.test.ts diff --git a/packages/cli/src/evaluation.ee/test-definitions.controller.ee.ts b/packages/cli/src/evaluation.ee/test-definitions.controller.ee.ts index bd4a841948..0cec91638b 100644 --- a/packages/cli/src/evaluation.ee/test-definitions.controller.ee.ts +++ b/packages/cli/src/evaluation.ee/test-definitions.controller.ee.ts @@ -145,4 +145,20 @@ export class TestDefinitionsController { res.status(202).json({ success: true }); } + + @Get('/:id/example-evaluation-input') + async exampleEvaluationInput(req: TestDefinitionsRequest.ExampleEvaluationInput) { + const { id: testDefinitionId } = req.params; + const { annotationTagId } = req.query; + + const workflowIds = await getSharedWorkflowIds(req.user, ['workflow:read']); + + const testDefinition = await this.testDefinitionService.findOne(testDefinitionId, workflowIds); + if (!testDefinition) throw new NotFoundError('Test definition not found'); + + return await this.testRunnerService.getExampleEvaluationInputData( + testDefinition, + annotationTagId, + ); + } } diff --git a/packages/cli/src/evaluation.ee/test-definitions.types.ee.ts b/packages/cli/src/evaluation.ee/test-definitions.types.ee.ts index 36d9715acc..98feea7e5d 100644 --- a/packages/cli/src/evaluation.ee/test-definitions.types.ee.ts +++ b/packages/cli/src/evaluation.ee/test-definitions.types.ee.ts @@ -38,6 +38,13 @@ export declare namespace TestDefinitionsRequest { type Delete = AuthenticatedRequest; type Run = AuthenticatedRequest; + + type ExampleEvaluationInput = AuthenticatedRequest< + RouteParams.TestId, + {}, + {}, + { annotationTagId: string } + >; } // ---------------------------------- diff --git a/packages/cli/src/evaluation.ee/test-runner/__tests__/format-test-case-execution-input-data.ee.test.ts b/packages/cli/src/evaluation.ee/test-runner/__tests__/format-test-case-execution-input-data.ee.test.ts new file mode 100644 index 0000000000..dbbd543c8e --- /dev/null +++ b/packages/cli/src/evaluation.ee/test-runner/__tests__/format-test-case-execution-input-data.ee.test.ts @@ -0,0 +1,84 @@ +import { readFileSync } from 'fs'; +import { mock } from 'jest-mock-extended'; +import path from 'path'; + +import type { TestCaseRunMetadata } from '@/evaluation.ee/test-runner/test-runner.service.ee'; +import { formatTestCaseExecutionInputData } from '@/evaluation.ee/test-runner/utils.ee'; + +const wfUnderTestJson = JSON.parse( + readFileSync(path.join(__dirname, './mock-data/workflow.under-test.json'), { encoding: 'utf-8' }), +); + +const executionDataJson = JSON.parse( + readFileSync(path.join(__dirname, './mock-data/execution-data.json'), { encoding: 'utf-8' }), +); + +describe('formatTestCaseExecutionInputData', () => { + test('should format the test case execution input data correctly', () => { + const data = formatTestCaseExecutionInputData( + executionDataJson.resultData.runData, + wfUnderTestJson, + executionDataJson.resultData.runData, + wfUnderTestJson, + mock({ + pastExecutionId: 'exec-id', + highlightedData: [], + annotation: { + vote: 'up', + tags: [{ id: 'tag-id', name: 'tag-name' }], + }, + }), + ); + + // Check data have all expected properties + expect(data.json).toMatchObject({ + originalExecution: expect.anything(), + newExecution: expect.anything(), + annotations: expect.anything(), + }); + + // Check original execution contains all the expected nodes + expect(data.json.originalExecution).toHaveProperty('72256d90-3a67-4e29-b032-47df4e5768af'); + expect(data.json.originalExecution).toHaveProperty('319f29bc-1dd4-4122-b223-c584752151a4'); + expect(data.json.originalExecution).toHaveProperty('d2474215-63af-40a4-a51e-0ea30d762621'); + + // Check format of specific node data + expect(data.json.originalExecution).toMatchObject({ + '72256d90-3a67-4e29-b032-47df4e5768af': { + nodeName: 'When clicking ‘Test workflow’', + runs: [ + { + executionTime: 0, + rootNode: true, + output: { + main: [ + [ + { + query: 'First item', + }, + { + query: 'Second item', + }, + { + query: 'Third item', + }, + ], + ], + }, + }, + ], + }, + }); + + // Check annotations + expect(data).toMatchObject({ + json: { + annotations: { + vote: 'up', + tags: [{ id: 'tag-id', name: 'tag-name' }], + highlightedData: {}, + }, + }, + }); + }); +}); 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 30f1659836..caf176d01f 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 @@ -75,22 +75,29 @@ const executionDataMultipleTriggersJson2 = JSON.parse( const executionMocks = [ mock({ - id: 'some-execution-id', + id: 'past-execution-id', workflowId: 'workflow-under-test-id', status: 'success', executionData: { data: stringify(executionDataJson), workflowData: wfUnderTestJson, }, + metadata: [ + { + key: 'testRunId', + value: 'test-run-id', + }, + ], }), mock({ - id: 'some-execution-id-2', + id: 'past-execution-id-2', workflowId: 'workflow-under-test-id', status: 'success', executionData: { data: stringify(executionDataRenamedNodesJson), workflowData: wfUnderTestRenamedNodesJson, }, + metadata: [], }), ]; @@ -179,10 +186,10 @@ describe('TestRunnerService', () => { executionsQbMock.getMany.mockResolvedValueOnce(executionMocks); executionRepository.createQueryBuilder.mockReturnValueOnce(executionsQbMock); executionRepository.findOne - .calledWith(expect.objectContaining({ where: { id: 'some-execution-id' } })) + .calledWith(expect.objectContaining({ where: { id: 'past-execution-id' } })) .mockResolvedValueOnce(executionMocks[0]); executionRepository.findOne - .calledWith(expect.objectContaining({ where: { id: 'some-execution-id-2' } })) + .calledWith(expect.objectContaining({ where: { id: 'past-execution-id-2' } })) .mockResolvedValueOnce(executionMocks[1]); testRunRepository.createTestRun.mockResolvedValue(mock({ id: 'test-run-id' })); @@ -242,20 +249,20 @@ describe('TestRunnerService', () => { ...wfEvaluationJson, }); - workflowRunner.run.mockResolvedValue('test-execution-id'); + workflowRunner.run.mockResolvedValue('some-execution-id'); await testRunnerService.runTest( mock(), mock({ workflowId: 'workflow-under-test-id', evaluationWorkflowId: 'evaluation-workflow-id', - mockedNodes: [], + mockedNodes: [{ id: '72256d90-3a67-4e29-b032-47df4e5768af' }], }), ); expect(executionRepository.createQueryBuilder).toHaveBeenCalledTimes(1); expect(executionRepository.findOne).toHaveBeenCalledTimes(2); - expect(workflowRunner.run).toHaveBeenCalledTimes(2); + expect(workflowRunner.run).toHaveBeenCalled(); }); test('should run both workflow under test and evaluation workflow', async () => { @@ -676,6 +683,47 @@ describe('TestRunnerService', () => { }); }); + test('should properly run test when nodes were renamed', async () => { + const testRunnerService = new TestRunnerService( + logger, + telemetry, + workflowRepository, + workflowRunner, + executionRepository, + activeExecutions, + testRunRepository, + testCaseExecutionRepository, + testMetricRepository, + mockNodeTypes, + errorReporter, + ); + + workflowRepository.findById.calledWith('workflow-under-test-id').mockResolvedValueOnce({ + id: 'workflow-under-test-id', + ...wfUnderTestJson, + }); + + workflowRepository.findById.calledWith('evaluation-workflow-id').mockResolvedValueOnce({ + id: 'evaluation-workflow-id', + ...wfEvaluationJson, + }); + + workflowRunner.run.mockResolvedValue('test-execution-id'); + + await testRunnerService.runTest( + mock(), + mock({ + workflowId: 'workflow-under-test-id', + evaluationWorkflowId: 'evaluation-workflow-id', + mockedNodes: [{ id: '72256d90-3a67-4e29-b032-47df4e5768af' }], + }), + ); + + expect(executionRepository.createQueryBuilder).toHaveBeenCalledTimes(1); + expect(executionRepository.findOne).toHaveBeenCalledTimes(2); + expect(workflowRunner.run).toHaveBeenCalledTimes(2); + }); + test('should properly choose trigger when it was renamed', async () => { const testRunnerService = new TestRunnerService( logger, 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 89d5ca2282..2c6c45ba68 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 @@ -5,7 +5,6 @@ import { ExecutionCancelledError, NodeConnectionType, Workflow } from 'n8n-workf import type { IDataObject, IRun, - IRunData, IRunExecutionData, IWorkflowBase, IWorkflowExecutionDataProcess, @@ -30,15 +29,21 @@ import { getRunData } from '@/workflow-execute-additional-data'; import { WorkflowRunner } from '@/workflow-runner'; import { EvaluationMetrics } from './evaluation-metrics.ee'; -import { createPinData, getPastExecutionTriggerNode } from './utils.ee'; +import { + createPinData, + formatTestCaseExecutionInputData, + getPastExecutionTriggerNode, +} from './utils.ee'; -interface TestRunMetadata { +export interface TestRunMetadata { testRunId: string; userId: string; } -interface TestCaseRunMetadata extends TestRunMetadata { +export interface TestCaseRunMetadata extends TestRunMetadata { pastExecutionId: string; + annotation: ExecutionEntity['annotation']; + highlightedData: ExecutionEntity['metadata']; } /** @@ -197,8 +202,7 @@ export class TestRunnerService { */ private async runTestCaseEvaluation( evaluationWorkflow: IWorkflowBase, - expectedData: IRunData, - actualData: IRunData, + evaluationInputData: any, abortSignal: AbortSignal, metadata: TestCaseRunMetadata, ) { @@ -207,15 +211,6 @@ export class TestRunnerService { return; } - // Prepare the evaluation wf input data. - // Provide both the expected data and the actual data - const evaluationInputData = { - json: { - originalExecution: expectedData, - newExecution: actualData, - }, - }; - // Prepare the data to run the evaluation workflow const data = await getRunData(evaluationWorkflow, [evaluationInputData]); data.executionMode = 'integrated'; @@ -374,7 +369,7 @@ export class TestRunnerService { // Fetch past execution with data const pastExecution = await this.executionRepository.findOne({ where: { id: pastExecutionId }, - relations: ['executionData', 'metadata'], + relations: ['executionData', 'metadata', 'annotation', 'annotation.tags'], }); assert(pastExecution, 'Execution not found'); @@ -383,6 +378,8 @@ export class TestRunnerService { const testCaseMetadata = { ...testRunMetadata, pastExecutionId, + highlightedData: pastExecution.metadata, + annotation: pastExecution.annotation, }; // Run the test case and wait for it to finish @@ -418,11 +415,18 @@ export class TestRunnerService { // Get the original runData from the test case execution data const originalRunData = executionData.resultData.runData; + const evaluationInputData = formatTestCaseExecutionInputData( + originalRunData, + pastExecution.executionData.workflowData, + testCaseRunData, + workflow, + testCaseMetadata, + ); + // Run the evaluation workflow with the original and new run data const evalExecution = await this.runTestCaseEvaluation( evaluationWorkflow, - originalRunData, - testCaseRunData, + evaluationInputData, abortSignal, testCaseMetadata, ); @@ -555,4 +559,61 @@ export class TestRunnerService { }); } } + + /** + * Returns the example evaluation WF input for the test definition. + * It uses the latest execution of a workflow under test as a source and formats it + * the same way as the evaluation input would be formatted. + * We explicitly provide annotation tag here (and DO NOT use the one from DB), because the test definition + * might not be saved to the DB with the updated annotation tag at the moment we need to get the example data. + */ + async getExampleEvaluationInputData(test: TestDefinition, annotationTagId: string) { + // Select the id of latest execution with the annotation tag and workflow ID of the test + const lastPastExecution: Pick | null = await this.executionRepository + .createQueryBuilder('execution') + .select('execution.id') + .leftJoin('execution.annotation', 'annotation') + .leftJoin('annotation.tags', 'annotationTag') + .where('annotationTag.id = :tagId', { tagId: annotationTagId }) + .andWhere('execution.workflowId = :workflowId', { workflowId: test.workflowId }) + .orderBy('execution.createdAt', 'DESC') + .getOne(); + + if (lastPastExecution === null) { + return null; + } + + // Fetch past execution with data + const pastExecution = await this.executionRepository.findOne({ + where: { + id: lastPastExecution.id, + }, + relations: ['executionData', 'metadata', 'annotation', 'annotation.tags'], + }); + assert(pastExecution, 'Execution not found'); + + const executionData = parse(pastExecution.executionData.data) as IRunExecutionData; + + const sampleTestCaseMetadata = { + testRunId: 'sample-test-run-id', + userId: 'sample-user-id', + pastExecutionId: lastPastExecution.id, + highlightedData: pastExecution.metadata, + annotation: pastExecution.annotation, + }; + + // Get the original runData from the test case execution data + const originalRunData = executionData.resultData.runData; + + // We use the same execution data for the original and new run data format example + const evaluationInputData = formatTestCaseExecutionInputData( + originalRunData, + pastExecution.executionData.workflowData, + originalRunData, + pastExecution.executionData.workflowData, + sampleTestCaseMetadata, + ); + + return evaluationInputData.json; + } } 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 25f5ddf820..cdf8c86638 100644 --- a/packages/cli/src/evaluation.ee/test-runner/utils.ee.ts +++ b/packages/cli/src/evaluation.ee/test-runner/utils.ee.ts @@ -1,10 +1,19 @@ import assert from 'assert'; -import type { IRunExecutionData, IPinData, IWorkflowBase } from 'n8n-workflow'; +import { mapValues, pick } from 'lodash'; +import type { + IRunExecutionData, + IPinData, + IWorkflowBase, + IRunData, + ITaskData, + INode, +} from 'n8n-workflow'; import type { TestCaseExecution } from '@/databases/entities/test-case-execution.ee'; import type { MockedNodeItem } from '@/databases/entities/test-definition.ee'; import type { TestRunFinalResult } from '@/databases/repositories/test-run.repository.ee'; import { TestCaseExecutionError } from '@/evaluation.ee/test-runner/errors.ee'; +import type { TestCaseRunMetadata } from '@/evaluation.ee/test-runner/test-runner.service.ee'; /** * Extracts the execution data from the past execution @@ -90,3 +99,72 @@ export function getTestRunFinalResult(testCaseExecutions: TestCaseExecution[]): return finalResult; } + +/** + * Function to check if the node is root node or sub-node. + * Sub-node is a node which does not have the main output (the only exception is Stop and Error node) + */ +function isSubNode(node: INode, nodeData: ITaskData[]) { + return ( + !node.type.endsWith('stopAndError') && + nodeData.some((nodeRunData) => !(nodeRunData.data && 'main' in nodeRunData.data)) + ); +} + +/** + * Transform execution data and workflow data into a more user-friendly format to supply to evaluation workflow + */ +function formatExecutionData(data: IRunData, workflow: IWorkflowBase) { + const formattedData = {} as Record; + + for (const [nodeName, nodeData] of Object.entries(data)) { + const node = workflow.nodes.find((n) => n.name === nodeName); + + assert(node, `Node "${nodeName}" not found in the workflow`); + + const rootNode = !isSubNode(node, nodeData); + + const runs = nodeData.map((nodeRunData) => ({ + executionTime: nodeRunData.executionTime, + rootNode, + output: nodeRunData.data + ? mapValues(nodeRunData.data, (connections) => + connections.map((singleOutputData) => singleOutputData?.map((item) => item.json) ?? []), + ) + : null, + })); + + formattedData[node.id] = { nodeName, runs }; + } + + return formattedData; +} + +/** + * Prepare the evaluation wf input data. + * Provide both the expected data (past execution) and the actual data (new execution), + * as well as any annotations or highlighted data associated with the past execution + */ +export function formatTestCaseExecutionInputData( + originalExecutionData: IRunData, + _originalWorkflowData: IWorkflowBase, + newExecutionData: IRunData, + _newWorkflowData: IWorkflowBase, + metadata: TestCaseRunMetadata, +) { + const annotations = { + vote: metadata.annotation?.vote, + tags: metadata.annotation?.tags?.map((tag) => pick(tag, ['id', 'name'])), + highlightedData: Object.fromEntries( + metadata.highlightedData?.map(({ key, value }) => [key, value]), + ), + }; + + return { + json: { + annotations, + originalExecution: formatExecutionData(originalExecutionData, _originalWorkflowData), + newExecution: formatExecutionData(newExecutionData, _newWorkflowData), + }, + }; +} diff --git a/packages/editor-ui/src/api/testDefinition.ee.ts b/packages/editor-ui/src/api/testDefinition.ee.ts index 4225a93559..775157d942 100644 --- a/packages/editor-ui/src/api/testDefinition.ee.ts +++ b/packages/editor-ui/src/api/testDefinition.ee.ts @@ -126,6 +126,18 @@ export async function deleteTestDefinition(context: IRestApiContext, id: string) return await makeRestApiRequest<{ success: boolean }>(context, 'DELETE', `${endpoint}/${id}`); } +export async function getExampleEvaluationInput( + context: IRestApiContext, + testDefinitionId: string, + annotationTagId: string, +) { + return await makeRestApiRequest | null>( + context, + 'GET', + `${endpoint}/${testDefinitionId}/example-evaluation-input?annotationTagId=${annotationTagId}`, + ); +} + // Metrics export interface TestMetricRecord { id: string; diff --git a/packages/editor-ui/src/stores/testDefinition.store.ee.ts b/packages/editor-ui/src/stores/testDefinition.store.ee.ts index 9fa69d007a..fa2cfdf657 100644 --- a/packages/editor-ui/src/stores/testDefinition.store.ee.ts +++ b/packages/editor-ui/src/stores/testDefinition.store.ee.ts @@ -221,6 +221,14 @@ export const useTestDefinitionStore = defineStore( } }; + const fetchExampleEvaluationInput = async (testId: string, annotationTagId: string) => { + return await testDefinitionsApi.getExampleEvaluationInput( + rootStore.restApiContext, + testId, + annotationTagId, + ); + }; + /** * Creates a new test definition using the provided parameters. * @@ -457,6 +465,7 @@ export const useTestDefinitionStore = defineStore( fetchTestDefinition, fetchTestCaseExecutions, fetchAll, + fetchExampleEvaluationInput, create, update, deleteById, diff --git a/packages/editor-ui/src/views/TestDefinition/TestDefinitionEditView.vue b/packages/editor-ui/src/views/TestDefinition/TestDefinitionEditView.vue index 95a49ea2aa..d2699dedff 100644 --- a/packages/editor-ui/src/views/TestDefinition/TestDefinitionEditView.vue +++ b/packages/editor-ui/src/views/TestDefinition/TestDefinitionEditView.vue @@ -16,9 +16,8 @@ import { useTestDefinitionStore } from '@/stores/testDefinition.store.ee'; import ConfigSection from '@/components/TestDefinition/EditDefinition/sections/ConfigSection.vue'; import { useTelemetry } from '@/composables/useTelemetry'; import { useRootStore } from '@/stores/root.store'; -import { useExecutionsStore } from '@/stores/executions.store'; import { useWorkflowsStore } from '@/stores/workflows.store'; -import type { IPinData } from 'n8n-workflow'; +import type { IDataObject, IPinData } from 'n8n-workflow'; const props = defineProps<{ testId?: string; @@ -33,7 +32,6 @@ const testDefinitionStore = useTestDefinitionStore(); const tagsStore = useAnnotationTagsStore(); const uiStore = useUIStore(); const telemetry = useTelemetry(); -const executionsStore = useExecutionsStore(); const workflowStore = useWorkflowsStore(); const { @@ -169,22 +167,16 @@ function toggleConfig() { } async function getExamplePinnedDataForTags() { - const evaluationWorkflowExecutions = await executionsStore.fetchExecutions({ - workflowId: currentWorkflowId.value, - annotationTags: state.value.tags.value, - }); - if (evaluationWorkflowExecutions.count > 0) { - const firstExecution = evaluationWorkflowExecutions.results[0]; - const executionData = await executionsStore.fetchExecution(firstExecution.id); - const resultData = executionData?.data?.resultData.runData; + const exampleInput = await testDefinitionStore.fetchExampleEvaluationInput( + testId.value, + state.value.tags.value[0], + ); + if (exampleInput !== null) { examplePinnedData.value = { 'When called by a test run': [ { - json: { - originalExecution: resultData, - newExecution: resultData, - }, + json: exampleInput as IDataObject, }, ], }; @@ -197,7 +189,7 @@ watch( debounce(async () => await updateMetrics(testId.value), { debounceTime: 400 }), { deep: true }, ); -watch(() => state.value.tags, getExamplePinnedDataForTags); +watch(() => state.value.tags.value, getExamplePinnedDataForTags); watch( () => [ state.value.description,