diff --git a/.github/workflows/ci-postgres-mysql.yml b/.github/workflows/ci-postgres-mysql.yml index bd66409add0b3..19fa1cad4f41a 100644 --- a/.github/workflows/ci-postgres-mysql.yml +++ b/.github/workflows/ci-postgres-mysql.yml @@ -42,7 +42,7 @@ jobs: run: pnpm build:backend - name: Cache build artifacts - uses: actions/cache/save@v4.0.0 + uses: actions/cache/save@v4.2.0 with: path: ./packages/**/dist key: ${{ github.sha }}:db-tests @@ -73,7 +73,7 @@ jobs: uses: rharkor/caching-for-turbo@v1.5 - name: Restore cached build artifacts - uses: actions/cache/restore@v4.0.0 + uses: actions/cache/restore@v4.2.0 with: path: ./packages/**/dist key: ${{ github.sha }}:db-tests @@ -107,7 +107,7 @@ jobs: uses: rharkor/caching-for-turbo@v1.5 - name: Restore cached build artifacts - uses: actions/cache/restore@v4.0.0 + uses: actions/cache/restore@v4.2.0 with: path: ./packages/**/dist key: ${{ github.sha }}:db-tests @@ -149,7 +149,7 @@ jobs: uses: rharkor/caching-for-turbo@v1.5 - name: Restore cached build artifacts - uses: actions/cache/restore@v4.0.0 + uses: actions/cache/restore@v4.2.0 with: path: ./packages/**/dist key: ${{ github.sha }}:db-tests diff --git a/.github/workflows/e2e-reusable.yml b/.github/workflows/e2e-reusable.yml index 82318961457bc..4f394325a8876 100644 --- a/.github/workflows/e2e-reusable.yml +++ b/.github/workflows/e2e-reusable.yml @@ -99,7 +99,7 @@ jobs: run: pnpm cypress:install - name: Cache build artifacts - uses: actions/cache/save@v4.0.0 + uses: actions/cache/save@v4.2.0 with: path: | /github/home/.cache @@ -133,7 +133,7 @@ jobs: - uses: pnpm/action-setup@v4.0.0 - name: Restore cached pnpm modules - uses: actions/cache/restore@v4.0.0 + uses: actions/cache/restore@v4.2.0 with: path: | /github/home/.cache diff --git a/.github/workflows/release-publish.yml b/.github/workflows/release-publish.yml index e8f8a44ee799d..06bc4e446578a 100644 --- a/.github/workflows/release-publish.yml +++ b/.github/workflows/release-publish.yml @@ -43,7 +43,7 @@ jobs: run: pnpm build - name: Cache build artifacts - uses: actions/cache/save@v4.0.0 + uses: actions/cache/save@v4.2.0 with: path: ./packages/**/dist key: ${{ github.sha }}-release:build @@ -144,7 +144,7 @@ jobs: steps: - uses: actions/checkout@v4.1.1 - name: Restore cached build artifacts - uses: actions/cache/restore@v4.0.0 + uses: actions/cache/restore@v4.2.0 with: path: ./packages/**/dist key: ${{ github.sha }}-release:build diff --git a/.github/workflows/test-workflows.yml b/.github/workflows/test-workflows.yml index 661a0ffd9d209..37ec57480df66 100644 --- a/.github/workflows/test-workflows.yml +++ b/.github/workflows/test-workflows.yml @@ -41,7 +41,7 @@ jobs: run: pnpm build:backend - name: Cache build artifacts - uses: actions/cache/save@v4.0.0 + uses: actions/cache/save@v4.2.0 with: path: ./packages/**/dist key: ${{ github.sha }}:workflow-tests @@ -69,7 +69,7 @@ jobs: uses: rharkor/caching-for-turbo@v1.5 - name: Restore cached build artifacts - uses: actions/cache/restore@v4.0.0 + uses: actions/cache/restore@v4.2.0 with: path: ./packages/**/dist key: ${{ github.sha }}:workflow-tests diff --git a/cypress/e2e/233-AI-switch-to-logs-on-error.cy.ts b/cypress/e2e/233-AI-switch-to-logs-on-error.cy.ts index 79f33b841cd1e..9aa30bb58c8fd 100644 --- a/cypress/e2e/233-AI-switch-to-logs-on-error.cy.ts +++ b/cypress/e2e/233-AI-switch-to-logs-on-error.cy.ts @@ -1,4 +1,4 @@ -import type { ExecutionError } from 'n8n-workflow/src'; +import type { ExecutionError } from 'n8n-workflow'; import { closeManualChatModal, diff --git a/packages/@n8n/nodes-langchain/nodes/chains/InformationExtractor/test/InformationExtraction.node.test.ts b/packages/@n8n/nodes-langchain/nodes/chains/InformationExtractor/test/InformationExtraction.node.test.ts index 725af39a60460..0444ba3cefde0 100644 --- a/packages/@n8n/nodes-langchain/nodes/chains/InformationExtractor/test/InformationExtraction.node.test.ts +++ b/packages/@n8n/nodes-langchain/nodes/chains/InformationExtractor/test/InformationExtraction.node.test.ts @@ -1,7 +1,7 @@ import type { BaseLanguageModel } from '@langchain/core/language_models/base'; import { FakeLLM, FakeListChatModel } from '@langchain/core/utils/testing'; import get from 'lodash/get'; -import type { IDataObject, IExecuteFunctions } from 'n8n-workflow/src'; +import type { IDataObject, IExecuteFunctions } from 'n8n-workflow'; import { makeZodSchemaFromAttributes } from '../helpers'; import { InformationExtractor } from '../InformationExtractor.node'; diff --git a/packages/cli/src/commands/base-command.ts b/packages/cli/src/commands/base-command.ts index 5e07b8e350b0a..930acceb1c96f 100644 --- a/packages/cli/src/commands/base-command.ts +++ b/packages/cli/src/commands/base-command.ts @@ -19,6 +19,7 @@ import * as CrashJournal from '@/crash-journal'; import * as Db from '@/db'; import { getDataDeduplicationService } from '@/deduplication'; import { DeprecationService } from '@/deprecation/deprecation.service'; +import { TestRunnerService } from '@/evaluation.ee/test-runner/test-runner.service.ee'; import { MessageEventBus } from '@/eventbus/message-event-bus/message-event-bus'; import { TelemetryEventRelay } from '@/events/relays/telemetry.event-relay'; import { initExpressionEvaluator } from '@/expression-evaluator'; @@ -259,6 +260,10 @@ export abstract class BaseCommand extends Command { Container.get(WorkflowHistoryManager).init(); } + async cleanupTestRunner() { + await Container.get(TestRunnerService).cleanupIncompleteRuns(); + } + async finally(error: Error | undefined) { if (inTest || this.id === 'start') return; if (Db.connectionState.connected) { diff --git a/packages/cli/src/commands/start.ts b/packages/cli/src/commands/start.ts index a255a95bb905a..56c648b4902d4 100644 --- a/packages/cli/src/commands/start.ts +++ b/packages/cli/src/commands/start.ts @@ -222,6 +222,11 @@ export class Start extends BaseCommand { this.initWorkflowHistory(); this.logger.debug('Workflow history init complete'); + if (!isMultiMainEnabled) { + await this.cleanupTestRunner(); + this.logger.debug('Test runner cleanup complete'); + } + if (!this.globalConfig.endpoints.disableUi) { await this.generateStaticAssets(); } diff --git a/packages/cli/src/databases/entities/test-case-execution.ee.ts b/packages/cli/src/databases/entities/test-case-execution.ee.ts index dc15ae63b58e3..d9beadbf8e6ac 100644 --- a/packages/cli/src/databases/entities/test-case-execution.ee.ts +++ b/packages/cli/src/databases/entities/test-case-execution.ee.ts @@ -1,4 +1,5 @@ import { Column, Entity, ManyToOne, OneToOne } from '@n8n/typeorm'; +import type { IDataObject } from 'n8n-workflow'; import { datetimeColumnType, @@ -7,9 +8,19 @@ import { } from '@/databases/entities/abstract-entity'; import type { ExecutionEntity } from '@/databases/entities/execution-entity'; import { TestRun } from '@/databases/entities/test-run.ee'; +import type { TestCaseExecutionErrorCode } from '@/evaluation.ee/test-runner/errors.ee'; export type TestCaseRunMetrics = Record; +export type TestCaseExecutionStatus = + | 'new' // Test case execution was created and added to the test run, but has not been started yet + | 'running' // Workflow under test is running + | 'evaluation_running' // Evaluation workflow is running + | 'success' // Both workflows have completed successfully + | 'error' // An error occurred during the execution of workflow under test or evaluation workflow + | 'warning' // There were warnings during the execution of workflow under test or evaluation workflow. Used only to signal possible issues to user, not to indicate a failure. + | 'cancelled'; + /** * This entity represents the linking between the test runs and individual executions. * It stores status, links to past, new and evaluation executions, and metrics produced by individual evaluation wf executions @@ -49,7 +60,7 @@ export class TestCaseExecution extends WithStringId { evaluationExecutionId: string | null; @Column() - status: 'new' | 'running' | 'evaluation_running' | 'success' | 'error' | 'cancelled'; + status: TestCaseExecutionStatus; @Column({ type: datetimeColumnType, nullable: true }) runAt: Date | null; @@ -58,10 +69,10 @@ export class TestCaseExecution extends WithStringId { completedAt: Date | null; @Column('varchar', { nullable: true }) - errorCode: string | null; + errorCode: TestCaseExecutionErrorCode | null; @Column(jsonColumnType, { nullable: true }) - errorDetails: Record; + errorDetails: IDataObject | null; @Column(jsonColumnType, { nullable: true }) metrics: TestCaseRunMetrics; diff --git a/packages/cli/src/databases/entities/test-run.ee.ts b/packages/cli/src/databases/entities/test-run.ee.ts index 79c7bc9f07596..51549f763a886 100644 --- a/packages/cli/src/databases/entities/test-run.ee.ts +++ b/packages/cli/src/databases/entities/test-run.ee.ts @@ -1,13 +1,17 @@ -import { Column, Entity, Index, ManyToOne, RelationId } from '@n8n/typeorm'; +import { Column, Entity, Index, ManyToOne, OneToMany, RelationId } from '@n8n/typeorm'; +import type { IDataObject } from 'n8n-workflow'; import { datetimeColumnType, jsonColumnType, WithTimestampsAndStringId, } from '@/databases/entities/abstract-entity'; +import type { TestCaseExecution } from '@/databases/entities/test-case-execution.ee'; import { TestDefinition } from '@/databases/entities/test-definition.ee'; +import type { TestRunFinalResult } from '@/databases/repositories/test-run.repository.ee'; +import type { TestRunErrorCode } from '@/evaluation.ee/test-runner/errors.ee'; -type TestRunStatus = 'new' | 'running' | 'completed' | 'error' | 'cancelled'; +export type TestRunStatus = 'new' | 'running' | 'completed' | 'error' | 'cancelled'; export type AggregatedTestRunMetrics = Record; @@ -54,4 +58,26 @@ export class TestRun extends WithTimestampsAndStringId { */ @Column('integer', { nullable: true }) failedCases: number; + + /** + * This will contain the error code if the test run failed. + * This is used for test run level errors, not for individual test case errors. + */ + @Column('varchar', { nullable: true, length: 255 }) + errorCode: TestRunErrorCode | null; + + /** + * Optional details about the error that happened during the test run + */ + @Column(jsonColumnType, { nullable: true }) + errorDetails: IDataObject | null; + + @OneToMany('TestCaseExecution', 'testRun') + testCaseExecutions: TestCaseExecution[]; + + /** + * Calculated property to determine the final result of the test run + * depending on the statuses of test case executions + */ + finalResult?: TestRunFinalResult | null; } diff --git a/packages/cli/src/databases/migrations/common/1737715421462-AddErrorColumnsToTestRuns.ts b/packages/cli/src/databases/migrations/common/1737715421462-AddErrorColumnsToTestRuns.ts new file mode 100644 index 0000000000000..b3affe9ac456f --- /dev/null +++ b/packages/cli/src/databases/migrations/common/1737715421462-AddErrorColumnsToTestRuns.ts @@ -0,0 +1,24 @@ +import type { MigrationContext, ReversibleMigration } from '@/databases/types'; + +// We have to use raw query migration instead of schemaBuilder helpers, +// because the typeorm schema builder implements addColumns by a table recreate for sqlite +// which causes weird issues with the migration +export class AddErrorColumnsToTestRuns1737715421462 implements ReversibleMigration { + async up({ escape, runQuery }: MigrationContext) { + const tableName = escape.tableName('test_run'); + const errorCodeColumnName = escape.columnName('errorCode'); + const errorDetailsColumnName = escape.columnName('errorDetails'); + + await runQuery(`ALTER TABLE ${tableName} ADD COLUMN ${errorCodeColumnName} VARCHAR(255);`); + await runQuery(`ALTER TABLE ${tableName} ADD COLUMN ${errorDetailsColumnName} TEXT;`); + } + + async down({ escape, runQuery }: MigrationContext) { + const tableName = escape.tableName('test_run'); + const errorCodeColumnName = escape.columnName('errorCode'); + const errorDetailsColumnName = escape.columnName('errorDetails'); + + await runQuery(`ALTER TABLE ${tableName} DROP COLUMN ${errorCodeColumnName};`); + await runQuery(`ALTER TABLE ${tableName} DROP COLUMN ${errorDetailsColumnName};`); + } +} diff --git a/packages/cli/src/databases/migrations/mysqldb/index.ts b/packages/cli/src/databases/migrations/mysqldb/index.ts index 32b1dd9751608..8789f7ac213ea 100644 --- a/packages/cli/src/databases/migrations/mysqldb/index.ts +++ b/packages/cli/src/databases/migrations/mysqldb/index.ts @@ -78,6 +78,7 @@ import { AddMockedNodesColumnToTestDefinition1733133775640 } from '../common/173 import { AddManagedColumnToCredentialsTable1734479635324 } from '../common/1734479635324-AddManagedColumnToCredentialsTable'; import { AddStatsColumnsToTestRun1736172058779 } from '../common/1736172058779-AddStatsColumnsToTestRun'; import { CreateTestCaseExecutionTable1736947513045 } from '../common/1736947513045-CreateTestCaseExecutionTable'; +import { AddErrorColumnsToTestRuns1737715421462 } from '../common/1737715421462-AddErrorColumnsToTestRuns'; export const mysqlMigrations: Migration[] = [ InitialMigration1588157391238, @@ -158,4 +159,5 @@ export const mysqlMigrations: Migration[] = [ AddProjectIcons1729607673469, AddStatsColumnsToTestRun1736172058779, CreateTestCaseExecutionTable1736947513045, + AddErrorColumnsToTestRuns1737715421462, ]; diff --git a/packages/cli/src/databases/migrations/postgresdb/index.ts b/packages/cli/src/databases/migrations/postgresdb/index.ts index c5547271b7a44..40e78bd49ccdd 100644 --- a/packages/cli/src/databases/migrations/postgresdb/index.ts +++ b/packages/cli/src/databases/migrations/postgresdb/index.ts @@ -78,6 +78,7 @@ import { AddMockedNodesColumnToTestDefinition1733133775640 } from '../common/173 import { AddManagedColumnToCredentialsTable1734479635324 } from '../common/1734479635324-AddManagedColumnToCredentialsTable'; import { AddStatsColumnsToTestRun1736172058779 } from '../common/1736172058779-AddStatsColumnsToTestRun'; import { CreateTestCaseExecutionTable1736947513045 } from '../common/1736947513045-CreateTestCaseExecutionTable'; +import { AddErrorColumnsToTestRuns1737715421462 } from '../common/1737715421462-AddErrorColumnsToTestRuns'; export const postgresMigrations: Migration[] = [ InitialMigration1587669153312, @@ -158,4 +159,5 @@ export const postgresMigrations: Migration[] = [ AddProjectIcons1729607673469, AddStatsColumnsToTestRun1736172058779, CreateTestCaseExecutionTable1736947513045, + AddErrorColumnsToTestRuns1737715421462, ]; diff --git a/packages/cli/src/databases/migrations/sqlite/index.ts b/packages/cli/src/databases/migrations/sqlite/index.ts index b8b8e26d3d79a..4ee7549f94c95 100644 --- a/packages/cli/src/databases/migrations/sqlite/index.ts +++ b/packages/cli/src/databases/migrations/sqlite/index.ts @@ -75,6 +75,7 @@ import { AddMockedNodesColumnToTestDefinition1733133775640 } from '../common/173 import { AddManagedColumnToCredentialsTable1734479635324 } from '../common/1734479635324-AddManagedColumnToCredentialsTable'; import { AddStatsColumnsToTestRun1736172058779 } from '../common/1736172058779-AddStatsColumnsToTestRun'; import { CreateTestCaseExecutionTable1736947513045 } from '../common/1736947513045-CreateTestCaseExecutionTable'; +import { AddErrorColumnsToTestRuns1737715421462 } from '../common/1737715421462-AddErrorColumnsToTestRuns'; const sqliteMigrations: Migration[] = [ InitialMigration1588102412422, @@ -152,6 +153,7 @@ const sqliteMigrations: Migration[] = [ AddProjectIcons1729607673469, AddStatsColumnsToTestRun1736172058779, CreateTestCaseExecutionTable1736947513045, + AddErrorColumnsToTestRuns1737715421462, ]; export { sqliteMigrations }; diff --git a/packages/cli/src/databases/repositories/test-case-execution.repository.ee.ts b/packages/cli/src/databases/repositories/test-case-execution.repository.ee.ts index c9798c89419b9..08607c6e2450c 100644 --- a/packages/cli/src/databases/repositories/test-case-execution.repository.ee.ts +++ b/packages/cli/src/databases/repositories/test-case-execution.repository.ee.ts @@ -2,8 +2,35 @@ import { Service } from '@n8n/di'; import type { EntityManager } from '@n8n/typeorm'; import { DataSource, In, Not, Repository } from '@n8n/typeorm'; import type { DeepPartial } from '@n8n/typeorm/common/DeepPartial'; +import type { IDataObject } from 'n8n-workflow'; import { TestCaseExecution } from '@/databases/entities/test-case-execution.ee'; +import type { TestCaseExecutionErrorCode } from '@/evaluation.ee/test-runner/errors.ee'; + +type StatusUpdateOptions = { + testRunId: string; + pastExecutionId: string; + trx?: EntityManager; +}; + +type MarkAsFailedOptions = StatusUpdateOptions & { + errorCode?: TestCaseExecutionErrorCode; + errorDetails?: IDataObject; +}; + +type MarkAsWarningOptions = MarkAsFailedOptions; + +type MarkAsRunningOptions = StatusUpdateOptions & { + executionId: string; +}; + +type MarkAsEvaluationRunningOptions = StatusUpdateOptions & { + evaluationExecutionId: string; +}; + +type MarkAsCompletedOptions = StatusUpdateOptions & { + metrics: Record; +}; @Service() export class TestCaseExecutionRepository extends Repository { @@ -27,8 +54,11 @@ export class TestCaseExecutionRepository extends Repository { return await this.save(mappings); } - async markAsRunning(testRunId: string, pastExecutionId: string, executionId: string) { - return await this.update( + async markAsRunning({ testRunId, pastExecutionId, executionId, trx }: MarkAsRunningOptions) { + trx = trx ?? this.manager; + + return await trx.update( + TestCaseExecution, { testRun: { id: testRunId }, pastExecutionId }, { status: 'running', @@ -38,12 +68,16 @@ export class TestCaseExecutionRepository extends Repository { ); } - async markAsEvaluationRunning( - testRunId: string, - pastExecutionId: string, - evaluationExecutionId: string, - ) { - return await this.update( + async markAsEvaluationRunning({ + testRunId, + pastExecutionId, + evaluationExecutionId, + trx, + }: MarkAsEvaluationRunningOptions) { + trx = trx ?? this.manager; + + return await trx.update( + TestCaseExecution, { testRun: { id: testRunId }, pastExecutionId }, { status: 'evaluation_running', @@ -52,12 +86,7 @@ export class TestCaseExecutionRepository extends Repository { ); } - async markAsCompleted( - testRunId: string, - pastExecutionId: string, - metrics: Record, - trx?: EntityManager, - ) { + async markAsCompleted({ testRunId, pastExecutionId, metrics, trx }: MarkAsCompletedOptions) { trx = trx ?? this.manager; return await trx.update( @@ -84,7 +113,13 @@ export class TestCaseExecutionRepository extends Repository { ); } - async markAsFailed(testRunId: string, pastExecutionId: string, trx?: EntityManager) { + async markAsFailed({ + testRunId, + pastExecutionId, + errorCode, + errorDetails, + trx, + }: MarkAsFailedOptions) { trx = trx ?? this.manager; return await trx.update( @@ -93,6 +128,25 @@ export class TestCaseExecutionRepository extends Repository { { status: 'error', completedAt: new Date(), + errorCode, + errorDetails, + }, + ); + } + + async markAsWarning({ + testRunId, + pastExecutionId, + errorCode, + errorDetails, + }: MarkAsWarningOptions) { + return await this.update( + { testRun: { id: testRunId }, pastExecutionId }, + { + status: 'warning', + completedAt: new Date(), + errorCode, + errorDetails, }, ); } diff --git a/packages/cli/src/databases/repositories/test-run.repository.ee.ts b/packages/cli/src/databases/repositories/test-run.repository.ee.ts index 1f2039aceea36..c86a3b5842943 100644 --- a/packages/cli/src/databases/repositories/test-run.repository.ee.ts +++ b/packages/cli/src/databases/repositories/test-run.repository.ee.ts @@ -1,11 +1,21 @@ import { Service } from '@n8n/di'; import type { EntityManager, FindManyOptions } from '@n8n/typeorm'; -import { DataSource, Repository } from '@n8n/typeorm'; +import { DataSource, In, Repository } from '@n8n/typeorm'; +import type { IDataObject } from 'n8n-workflow'; import type { AggregatedTestRunMetrics } from '@/databases/entities/test-run.ee'; import { TestRun } from '@/databases/entities/test-run.ee'; +import { NotFoundError } from '@/errors/response-errors/not-found.error'; +import type { TestRunErrorCode } from '@/evaluation.ee/test-runner/errors.ee'; +import { getTestRunFinalResult } from '@/evaluation.ee/test-runner/utils.ee'; import type { ListQuery } from '@/requests'; +export type TestRunFinalResult = 'success' | 'error' | 'warning'; + +export type TestRunSummary = TestRun & { + finalResult: TestRunFinalResult | null; +}; + @Service() export class TestRunRepository extends Repository { constructor(dataSource: DataSource) { @@ -40,6 +50,21 @@ export class TestRunRepository extends Repository { return await trx.update(TestRun, id, { status: 'cancelled' }); } + async markAsError(id: string, errorCode: TestRunErrorCode, errorDetails?: IDataObject) { + return await this.update(id, { + status: 'error', + errorCode, + errorDetails, + }); + } + + async markAllIncompleteAsFailed() { + return await this.update( + { status: In(['new', 'running']) }, + { status: 'error', errorCode: 'INTERRUPTED' }, + ); + } + async incrementPassed(id: string, trx?: EntityManager) { trx = trx ?? this.manager; return await trx.increment(TestRun, { id }, 'passedCases', 1); @@ -51,9 +76,11 @@ export class TestRunRepository extends Repository { } async getMany(testDefinitionId: string, options: ListQuery.Options) { + // FIXME: optimize fetching final result of each test run const findManyOptions: FindManyOptions = { where: { testDefinition: { id: testDefinitionId } }, order: { createdAt: 'DESC' }, + relations: ['testCaseExecutions'], }; if (options?.take) { @@ -61,6 +88,37 @@ export class TestRunRepository extends Repository { findManyOptions.take = options.take; } - return await this.find(findManyOptions); + const testRuns = await this.find(findManyOptions); + + return testRuns.map(({ testCaseExecutions, ...testRun }) => { + const finalResult = + testRun.status === 'completed' ? getTestRunFinalResult(testCaseExecutions) : null; + return { ...testRun, finalResult }; + }); + } + + /** + * Test run summary is a TestRun with a final result. + * Final result is calculated based on the status of all test case executions. + * E.g. Test Run is considered successful if all test case executions are successful. + * Test Run is considered failed if at least one test case execution is failed. + */ + async getTestRunSummaryById( + testDefinitionId: string, + testRunId: string, + ): Promise { + const testRun = await this.findOne({ + where: { id: testRunId, testDefinition: { id: testDefinitionId } }, + relations: ['testCaseExecutions'], + }); + + if (!testRun) { + throw new NotFoundError('Test run not found'); + } + + testRun.finalResult = + testRun.status === 'completed' ? getTestRunFinalResult(testRun.testCaseExecutions) : null; + + return testRun as TestRunSummary; } } 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 ed393421b01f8..682223e3b2729 100644 --- a/packages/cli/src/evaluation.ee/test-definition.service.ee.ts +++ b/packages/cli/src/evaluation.ee/test-definition.service.ee.ts @@ -126,13 +126,10 @@ export class TestDefinitionService { ); const existingNodeIds = new Map(existingTestDefinition.workflow.nodes.map((n) => [n.id, n])); - attrs.mockedNodes.forEach((node) => { - 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})`, - ); - } - }); + // If some node was previously mocked and then removed from the workflow, it should be removed from the mocked nodes + attrs.mockedNodes = attrs.mockedNodes.filter( + (node) => existingNodeIds.has(node.id) || (node.name && existingNodeNames.has(node.name)), + ); // Update the node names OR node ids if they are not provided attrs.mockedNodes = attrs.mockedNodes.map((node) => { diff --git a/packages/cli/src/evaluation.ee/test-runner/__tests__/evaluation-metrics.ee.test.ts b/packages/cli/src/evaluation.ee/test-runner/__tests__/evaluation-metrics.ee.test.ts index 27daf3aa793fd..d7bb9ec910d55 100644 --- a/packages/cli/src/evaluation.ee/test-runner/__tests__/evaluation-metrics.ee.test.ts +++ b/packages/cli/src/evaluation.ee/test-runner/__tests__/evaluation-metrics.ee.test.ts @@ -13,29 +13,23 @@ describe('EvaluationMetrics', () => { expect(aggregatedMetrics).toEqual({ metric1: 0.75, metric2: 0.1 }); }); - test('should aggregate only numbers', () => { + test('should throw when metric value is not number', () => { const testMetricNames = new Set(['metric1', 'metric2']); const metrics = new EvaluationMetrics(testMetricNames); - metrics.addResults({ metric1: 1, metric2: 0 }); - metrics.addResults({ metric1: '0.5', metric2: 0.2 }); - metrics.addResults({ metric1: 'not a number', metric2: [1, 2, 3] }); - - const aggregatedUpMetrics = metrics.getAggregatedMetrics(); - - expect(aggregatedUpMetrics).toEqual({ metric1: 1, metric2: 0.1 }); + expect(() => metrics.addResults({ metric1: 1, metric2: 0 })).not.toThrow(); + expect(() => metrics.addResults({ metric1: '0.5', metric2: 0.2 })).toThrow('INVALID_METRICS'); + expect(() => metrics.addResults({ metric1: 'not a number', metric2: [1, 2, 3] })).toThrow( + 'INVALID_METRICS', + ); }); - test('should handle missing values', () => { + test('should throw when missing values', () => { const testMetricNames = new Set(['metric1', 'metric2']); const metrics = new EvaluationMetrics(testMetricNames); - metrics.addResults({ metric1: 1 }); - metrics.addResults({ metric2: 0.2 }); - - const aggregatedMetrics = metrics.getAggregatedMetrics(); - - expect(aggregatedMetrics).toEqual({ metric1: 1, metric2: 0.2 }); + expect(() => metrics.addResults({ metric1: 1 })).toThrow('METRICS_MISSING'); + expect(() => metrics.addResults({ metric2: 0.2 })).toThrow('METRICS_MISSING'); }); test('should handle empty metrics', () => { @@ -69,4 +63,19 @@ describe('EvaluationMetrics', () => { expect(aggregatedMetrics).toEqual({ metric1: 0.75 }); }); + + test('should report info on added metrics', () => { + const testMetricNames = new Set(['metric1']); + const metrics = new EvaluationMetrics(testMetricNames); + let info; + + expect(() => (info = metrics.addResults({ metric1: 1, metric2: 0 }))).not.toThrow(); + + expect(info).toBeDefined(); + expect(info).toHaveProperty('unknownMetrics'); + expect(info!.unknownMetrics).toEqual(new Set(['metric2'])); + + expect(info).toHaveProperty('addedMetrics'); + expect(info!.addedMetrics).toEqual({ metric1: 1 }); + }); }); diff --git a/packages/cli/src/evaluation.ee/test-runner/__tests__/get-test-run-final-result.ee.test.ts b/packages/cli/src/evaluation.ee/test-runner/__tests__/get-test-run-final-result.ee.test.ts new file mode 100644 index 0000000000000..b12232dca97df --- /dev/null +++ b/packages/cli/src/evaluation.ee/test-runner/__tests__/get-test-run-final-result.ee.test.ts @@ -0,0 +1,42 @@ +import { mock } from 'jest-mock-extended'; + +import type { TestCaseExecution } from '@/databases/entities/test-case-execution.ee'; +import { getTestRunFinalResult } from '@/evaluation.ee/test-runner/utils.ee'; + +function mockTestCaseExecutions(statuses: Array) { + return statuses.map((status) => mock({ status })); +} + +describe('getTestRunFinalResult', () => { + test('should return success if all test cases are successful', () => { + const result = getTestRunFinalResult( + mockTestCaseExecutions(['success', 'success', 'success', 'success', 'success']), + ); + + expect(result).toEqual('success'); + }); + + test('should return error if at least one test case is errored', () => { + const result = getTestRunFinalResult( + mockTestCaseExecutions(['success', 'error', 'success', 'success', 'success']), + ); + + expect(result).toEqual('error'); + }); + + test('should return warning if at least one test case is warned', () => { + const result = getTestRunFinalResult( + mockTestCaseExecutions(['success', 'warning', 'success', 'success', 'success']), + ); + + expect(result).toEqual('warning'); + }); + + test('should return error if there are errors and warnings', () => { + const result = getTestRunFinalResult( + mockTestCaseExecutions(['success', 'error', 'warning', 'success', 'success']), + ); + + expect(result).toEqual('error'); + }); +}); diff --git a/packages/cli/src/evaluation.ee/test-runner/__tests__/mock-data/execution-data-renamed-nodes.json b/packages/cli/src/evaluation.ee/test-runner/__tests__/mock-data/execution-data-renamed-nodes.json new file mode 100644 index 0000000000000..4a26fae35cffd --- /dev/null +++ b/packages/cli/src/evaluation.ee/test-runner/__tests__/mock-data/execution-data-renamed-nodes.json @@ -0,0 +1,171 @@ +{ + "startData": {}, + "resultData": { + "runData": { + "Manual Run": [ + { + "hints": [], + "startTime": 1731079118048, + "executionTime": 0, + "source": [], + "executionStatus": "success", + "data": { + "main": [ + [ + { + "json": { + "query": "First item" + }, + "pairedItem": { + "item": 0 + } + }, + { + "json": { + "query": "Second item" + }, + "pairedItem": { + "item": 0 + } + }, + { + "json": { + "query": "Third item" + }, + "pairedItem": { + "item": 0 + } + } + ] + ] + } + } + ], + "Set Attribute": [ + { + "hints": [], + "startTime": 1731079118049, + "executionTime": 0, + "source": [ + { + "previousNode": "Manual Run" + } + ], + "executionStatus": "success", + "data": { + "main": [ + [ + { + "json": { + "foo": "bar" + }, + "pairedItem": { + "item": 0 + } + }, + { + "json": { + "foo": "bar" + }, + "pairedItem": { + "item": 1 + } + }, + { + "json": { + "foo": "bar" + }, + "pairedItem": { + "item": 2 + } + } + ] + ] + } + } + ], + "Code": [ + { + "hints": [], + "startTime": 1731079118049, + "executionTime": 3, + "source": [ + { + "previousNode": "Set Attribute" + } + ], + "executionStatus": "success", + "data": { + "main": [ + [ + { + "json": { + "foo": "bar", + "random": 0.6315509336851373 + }, + "pairedItem": { + "item": 0 + } + }, + { + "json": { + "foo": "bar", + "random": 0.3336315687359024 + }, + "pairedItem": { + "item": 1 + } + }, + { + "json": { + "foo": "bar", + "random": 0.4241870158917733 + }, + "pairedItem": { + "item": 2 + } + } + ] + ] + } + } + ] + }, + "pinData": { + "Manual Run": [ + { + "json": { + "query": "First item" + }, + "pairedItem": { + "item": 0 + } + }, + { + "json": { + "query": "Second item" + }, + "pairedItem": { + "item": 0 + } + }, + { + "json": { + "query": "Third item" + }, + "pairedItem": { + "item": 0 + } + } + ] + }, + "lastNodeExecuted": "Code" + }, + "executionData": { + "contextData": {}, + "nodeExecutionStack": [], + "metadata": {}, + "waitingExecution": {}, + "waitingExecutionSource": {} + } +} 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 7cfe7b3705368..30f165983697f 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 @@ -4,6 +4,7 @@ import { readFileSync } from 'fs'; import { mock, mockDeep } from 'jest-mock-extended'; import type { ErrorReporter } from 'n8n-core'; import type { ExecutionError, GenericValue, IRun } from 'n8n-workflow'; +import type { ITaskData } from 'n8n-workflow'; import path from 'path'; import type { ActiveExecutions } from '@/active-executions'; @@ -54,6 +55,12 @@ const executionDataJson = JSON.parse( readFileSync(path.join(__dirname, './mock-data/execution-data.json'), { encoding: 'utf-8' }), ); +const executionDataRenamedNodesJson = JSON.parse( + readFileSync(path.join(__dirname, './mock-data/execution-data-renamed-nodes.json'), { + encoding: 'utf-8', + }), +); + const executionDataMultipleTriggersJson = JSON.parse( readFileSync(path.join(__dirname, './mock-data/execution-data.multiple-triggers.json'), { encoding: 'utf-8', @@ -81,7 +88,7 @@ const executionMocks = [ workflowId: 'workflow-under-test-id', status: 'success', executionData: { - data: stringify(executionDataJson), + data: stringify(executionDataRenamedNodesJson), workflowData: wfUnderTestRenamedNodesJson, }, }), @@ -91,7 +98,12 @@ function mockExecutionData() { return mock({ data: { resultData: { - runData: {}, + runData: { + 'When clicking ‘Test workflow’': mock(), + }, + // error is an optional prop, but jest-mock-extended will mock it by default, + // which affects the code logic. So, we need to explicitly set it to undefined. + error: undefined, }, }, }); @@ -292,7 +304,7 @@ describe('TestRunnerService', () => { activeExecutions.getPostExecutePromise .calledWith('some-execution-id-4') - .mockResolvedValue(mockEvaluationExecutionData({ metric1: 0.5 })); + .mockResolvedValue(mockEvaluationExecutionData({ metric1: 0.5, metric2: 100 })); await testRunnerService.runTest( mock(), @@ -322,7 +334,7 @@ describe('TestRunnerService', () => { // Check evaluation workflow was executed expect(workflowRunner.run).toHaveBeenCalledWith( expect.objectContaining({ - executionMode: 'evaluation', + executionMode: 'integrated', executionData: expect.objectContaining({ executionData: expect.objectContaining({ nodeExecutionStack: expect.arrayContaining([ @@ -343,7 +355,7 @@ describe('TestRunnerService', () => { expect(testRunRepository.markAsCompleted).toHaveBeenCalledTimes(1); expect(testRunRepository.markAsCompleted).toHaveBeenCalledWith('test-run-id', { metric1: 0.75, - metric2: 0, + metric2: 50, }); expect(testRunRepository.incrementPassed).toHaveBeenCalledTimes(2); @@ -624,6 +636,7 @@ describe('TestRunnerService', () => { const startNodesData = (testRunnerService as any).getStartNodesData( wfMultipleTriggersJson, executionDataMultipleTriggersJson, + wfMultipleTriggersJson, // Test case where workflow didn't change ); expect(startNodesData).toEqual({ @@ -652,6 +665,7 @@ describe('TestRunnerService', () => { const startNodesData = (testRunnerService as any).getStartNodesData( wfMultipleTriggersJson, executionDataMultipleTriggersJson2, + wfMultipleTriggersJson, // Test case where workflow didn't change ); expect(startNodesData).toEqual({ @@ -662,6 +676,35 @@ describe('TestRunnerService', () => { }); }); + test('should properly choose trigger when it was renamed', async () => { + const testRunnerService = new TestRunnerService( + logger, + telemetry, + workflowRepository, + workflowRunner, + executionRepository, + activeExecutions, + testRunRepository, + testCaseExecutionRepository, + testMetricRepository, + mockNodeTypes, + errorReporter, + ); + + const startNodesData = (testRunnerService as any).getStartNodesData( + wfUnderTestRenamedNodesJson, // Test case where workflow didn't change + executionDataJson, + wfUnderTestJson, + ); + + expect(startNodesData).toEqual({ + startNodes: expect.arrayContaining([expect.objectContaining({ name: 'Set attribute' })]), + triggerToStartFrom: expect.objectContaining({ + name: 'Manual Run', + }), + }); + }); + describe('Test Run cancellation', () => { beforeAll(() => { jest.useFakeTimers(); diff --git a/packages/cli/src/evaluation.ee/test-runner/errors.ee.ts b/packages/cli/src/evaluation.ee/test-runner/errors.ee.ts new file mode 100644 index 0000000000000..05b88903fbb64 --- /dev/null +++ b/packages/cli/src/evaluation.ee/test-runner/errors.ee.ts @@ -0,0 +1,39 @@ +import { ApplicationError } from 'n8n-workflow'; + +export type TestCaseExecutionErrorCode = + | 'MOCKED_NODE_DOES_NOT_EXIST' + | 'TRIGGER_NO_LONGER_EXISTS' + | 'FAILED_TO_EXECUTE_WORKFLOW' + | 'EVALUATION_WORKFLOW_DOES_NOT_EXIST' + | 'FAILED_TO_EXECUTE_EVALUATION_WORKFLOW' + | 'METRICS_MISSING' + | 'UNKNOWN_METRICS' + | 'INVALID_METRICS' + | 'PAYLOAD_LIMIT_EXCEEDED' + | 'UNKNOWN_ERROR'; + +export class TestCaseExecutionError extends ApplicationError { + readonly code: TestCaseExecutionErrorCode; + + constructor(code: TestCaseExecutionErrorCode, extra: Record = {}) { + super('Test Case execution failed with code ' + code, { extra }); + + this.code = code; + } +} + +export type TestRunErrorCode = + | 'PAST_EXECUTIONS_NOT_FOUND' + | 'EVALUATION_WORKFLOW_NOT_FOUND' + | 'INTERRUPTED' + | 'UNKNOWN_ERROR'; + +export class TestRunError extends ApplicationError { + readonly code: TestRunErrorCode; + + constructor(code: TestRunErrorCode, extra: Record = {}) { + super('Test Run failed with code ' + code, { extra }); + + this.code = code; + } +} diff --git a/packages/cli/src/evaluation.ee/test-runner/evaluation-metrics.ee.ts b/packages/cli/src/evaluation.ee/test-runner/evaluation-metrics.ee.ts index cd320bd8dab50..b2422f4b6b7c6 100644 --- a/packages/cli/src/evaluation.ee/test-runner/evaluation-metrics.ee.ts +++ b/packages/cli/src/evaluation.ee/test-runner/evaluation-metrics.ee.ts @@ -1,5 +1,15 @@ +import difference from 'lodash/difference'; import type { IDataObject } from 'n8n-workflow'; +import { TestCaseExecutionError } from '@/evaluation.ee/test-runner/errors.ee'; + +export interface EvaluationMetricsAddResultsInfo { + addedMetrics: Record; + missingMetrics: Set; + unknownMetrics: Set; + incorrectTypeMetrics: Set; +} + export class EvaluationMetrics { private readonly rawMetricsByName = new Map(); @@ -9,17 +19,41 @@ export class EvaluationMetrics { } } - addResults(result: IDataObject): Record { - const addedMetrics: Record = {}; + addResults(result: IDataObject): EvaluationMetricsAddResultsInfo { + const addResultsInfo: EvaluationMetricsAddResultsInfo = { + addedMetrics: {}, + missingMetrics: new Set(), + unknownMetrics: new Set(), + incorrectTypeMetrics: new Set(), + }; for (const [metricName, metricValue] of Object.entries(result)) { - if (typeof metricValue === 'number' && this.metricNames.has(metricName)) { - addedMetrics[metricName] = metricValue; - this.rawMetricsByName.get(metricName)!.push(metricValue); + if (this.metricNames.has(metricName)) { + if (typeof metricValue === 'number') { + addResultsInfo.addedMetrics[metricName] = metricValue; + this.rawMetricsByName.get(metricName)!.push(metricValue); + } else { + throw new TestCaseExecutionError('INVALID_METRICS', { + metricName, + metricValue, + }); + } + } else { + addResultsInfo.unknownMetrics.add(metricName); } } - return addedMetrics; + // Check that result contains all expected metrics + if ( + difference(Array.from(this.metricNames), Object.keys(addResultsInfo.addedMetrics)).length > 0 + ) { + throw new TestCaseExecutionError('METRICS_MISSING', { + expectedMetrics: Array.from(this.metricNames).sort(), + receivedMetrics: Object.keys(addResultsInfo.addedMetrics).sort(), + }); + } + + return addResultsInfo; } getAggregatedMetrics() { 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 09c4fe4ec65aa..bb0ffbb29581d 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 @@ -24,6 +24,7 @@ import { TestMetricRepository } from '@/databases/repositories/test-metric.repos import { TestRunRepository } from '@/databases/repositories/test-run.repository.ee'; import { WorkflowRepository } from '@/databases/repositories/workflow.repository'; import * as Db from '@/db'; +import { TestCaseExecutionError, TestRunError } from '@/evaluation.ee/test-runner/errors.ee'; import { NodeTypes } from '@/node-types'; import { Telemetry } from '@/telemetry'; import { getRunData } from '@/workflow-execute-additional-data'; @@ -67,12 +68,21 @@ export class TestRunnerService { private readonly errorReporter: ErrorReporter, ) {} + /** + * As Test Runner does not have a recovery mechanism, it can not resume Test Runs interrupted by the server restart. + * All Test Runs in incomplete state will be marked as cancelled. + */ + async cleanupIncompleteRuns() { + await this.testRunRepository.markAllIncompleteAsFailed(); + } + /** * Prepares the start nodes and trigger node data props for the `workflowRunner.run` method input. */ private getStartNodesData( workflow: WorkflowEntity, pastExecutionData: IRunExecutionData, + pastExecutionWorkflowData: IWorkflowBase, ): Pick { // Create a new workflow instance to use the helper functions (getChildNodes) const workflowInstance = new Workflow({ @@ -82,21 +92,38 @@ export class TestRunnerService { nodeTypes: this.nodeTypes, }); + // Create a map between node IDs and node names for the past workflow + const pastWorkflowNodeIdByName = new Map( + pastExecutionWorkflowData.nodes.map((node) => [node.name, node.id]), + ); + + // Create a map between node names and IDs for the up-to-date workflow + const workflowNodeNameById = new Map(workflow.nodes.map((node) => [node.id, node.name])); + // Determine the trigger node of the past execution const pastExecutionTriggerNode = getPastExecutionTriggerNode(pastExecutionData); assert(pastExecutionTriggerNode, 'Could not find the trigger node of the past execution'); + const pastExecutionTriggerNodeId = pastWorkflowNodeIdByName.get(pastExecutionTriggerNode); + assert(pastExecutionTriggerNodeId, 'Could not find the trigger node ID of the past execution'); + + // Check the trigger is still present in the workflow + const triggerNode = workflowNodeNameById.get(pastExecutionTriggerNodeId); + if (!triggerNode) { + throw new TestCaseExecutionError('TRIGGER_NO_LONGER_EXISTS'); + } + const triggerNodeData = pastExecutionData.resultData.runData[pastExecutionTriggerNode][0]; assert(triggerNodeData, 'Trigger node data not found'); const triggerToStartFrom = { - name: pastExecutionTriggerNode, + name: triggerNode, data: triggerNodeData, }; // Start nodes are the nodes that are connected to the trigger node const startNodes = workflowInstance - .getChildNodes(pastExecutionTriggerNode, NodeConnectionType.Main, 1) + .getChildNodes(triggerNode, NodeConnectionType.Main, 1) .map((nodeName) => ({ name: nodeName, sourceData: { previousNode: pastExecutionTriggerNode }, @@ -135,7 +162,7 @@ export class TestRunnerService { // Prepare the data to run the workflow const data: IWorkflowExecutionDataProcess = { - ...this.getStartNodesData(workflow, pastExecutionData), + ...this.getStartNodesData(workflow, pastExecutionData, pastExecutionWorkflowData), executionMode: 'evaluation', runData: {}, pinData, @@ -154,11 +181,11 @@ export class TestRunnerService { }); // Update status of the test run execution mapping - await this.testCaseExecutionRepository.markAsRunning( - metadata.testRunId, - metadata.pastExecutionId, + await this.testCaseExecutionRepository.markAsRunning({ + testRunId: metadata.testRunId, + pastExecutionId: metadata.pastExecutionId, executionId, - ); + }); // Wait for the execution to finish const executePromise = this.activeExecutions.getPostExecutePromise(executionId); @@ -192,7 +219,7 @@ export class TestRunnerService { // Prepare the data to run the evaluation workflow const data = await getRunData(evaluationWorkflow, [evaluationInputData]); - data.executionMode = 'evaluation'; + data.executionMode = 'integrated'; // Trigger the evaluation workflow const executionId = await this.workflowRunner.run(data); @@ -204,11 +231,11 @@ export class TestRunnerService { }); // Update status of the test run execution mapping - await this.testCaseExecutionRepository.markAsEvaluationRunning( - metadata.testRunId, - metadata.pastExecutionId, - executionId, - ); + await this.testCaseExecutionRepository.markAsEvaluationRunning({ + testRunId: metadata.testRunId, + pastExecutionId: metadata.pastExecutionId, + evaluationExecutionId: executionId, + }); // Wait for the execution to finish const executePromise = this.activeExecutions.getPostExecutePromise(executionId); @@ -256,9 +283,6 @@ export class TestRunnerService { const workflow = await this.workflowRepository.findById(test.workflowId); assert(workflow, 'Workflow not found'); - const evaluationWorkflow = await this.workflowRepository.findById(test.evaluationWorkflowId); - assert(evaluationWorkflow, 'Evaluation workflow not found'); - // 0. Create new Test Run const testRun = await this.testRunRepository.createTestRun(test.id); assert(testRun, 'Unable to create a test run'); @@ -276,6 +300,12 @@ export class TestRunnerService { const abortSignal = abortController.signal; try { + // Get the evaluation workflow + const evaluationWorkflow = await this.workflowRepository.findById(test.evaluationWorkflowId); + if (!evaluationWorkflow) { + throw new TestRunError('EVALUATION_WORKFLOW_NOT_FOUND'); + } + /// // 1. Make test cases from previous executions /// @@ -294,6 +324,10 @@ export class TestRunnerService { this.logger.debug('Found past executions', { count: pastExecutions.length }); + if (pastExecutions.length === 0) { + throw new TestRunError('PAST_EXECUTIONS_NOT_FOUND'); + } + // Add all past executions mappings to the test run. // This will be used to track the status of each test case and keep the connection between test run and all related executions (past, current, and evaluation). await this.testCaseExecutionRepository.createBatch( @@ -365,20 +399,20 @@ export class TestRunnerService { this.logger.debug('Test case execution finished', { pastExecutionId }); // In case of a permission check issue, the test case execution will be undefined. - // Skip them, increment the failed count and continue with the next test case - if (!testCaseExecution) { + // If that happens, or if the test case execution produced an error, mark the test case as failed. + if (!testCaseExecution || testCaseExecution.data.resultData.error) { await Db.transaction(async (trx) => { await this.testRunRepository.incrementFailed(testRun.id, trx); - await this.testCaseExecutionRepository.markAsFailed(testRun.id, pastExecutionId, trx); + await this.testCaseExecutionRepository.markAsFailed({ + testRunId: testRun.id, + pastExecutionId, + errorCode: 'FAILED_TO_EXECUTE_WORKFLOW', + trx, + }); }); continue; } - // Update status of the test case execution mapping entry in case of an error - if (testCaseExecution.data.resultData.error) { - await this.testCaseExecutionRepository.markAsFailed(testRun.id, pastExecutionId); - } - // Collect the results of the test case execution const testCaseRunData = testCaseExecution.data.resultData.runData; @@ -398,32 +432,67 @@ export class TestRunnerService { this.logger.debug('Evaluation execution finished', { pastExecutionId }); // Extract the output of the last node executed in the evaluation workflow - const addedMetrics = metrics.addResults(this.extractEvaluationResult(evalExecution)); + const { addedMetrics, unknownMetrics } = metrics.addResults( + this.extractEvaluationResult(evalExecution), + ); if (evalExecution.data.resultData.error) { await Db.transaction(async (trx) => { await this.testRunRepository.incrementFailed(testRun.id, trx); - await this.testCaseExecutionRepository.markAsFailed(testRun.id, pastExecutionId, trx); + await this.testCaseExecutionRepository.markAsFailed({ + testRunId: testRun.id, + pastExecutionId, + errorCode: 'FAILED_TO_EXECUTE_EVALUATION_WORKFLOW', + trx, + }); }); } else { await Db.transaction(async (trx) => { await this.testRunRepository.incrementPassed(testRun.id, trx); - await this.testCaseExecutionRepository.markAsCompleted( - testRun.id, - pastExecutionId, - addedMetrics, - trx, - ); + + // Add warning if the evaluation workflow produced an unknown metric + if (unknownMetrics.size > 0) { + await this.testCaseExecutionRepository.markAsWarning({ + testRunId: testRun.id, + pastExecutionId, + errorCode: 'UNKNOWN_METRICS', + errorDetails: { unknownMetrics: Array.from(unknownMetrics) }, + }); + } else { + await this.testCaseExecutionRepository.markAsCompleted({ + testRunId: testRun.id, + pastExecutionId, + metrics: addedMetrics, + trx, + }); + } }); } } catch (e) { // In case of an unexpected error, increment the failed count and continue with the next test case await Db.transaction(async (trx) => { await this.testRunRepository.incrementFailed(testRun.id, trx); - await this.testCaseExecutionRepository.markAsFailed(testRun.id, pastExecutionId, trx); - }); - this.errorReporter.error(e); + if (e instanceof TestCaseExecutionError) { + await this.testCaseExecutionRepository.markAsFailed({ + testRunId: testRun.id, + pastExecutionId, + errorCode: e.code, + errorDetails: e.extra as IDataObject, + trx, + }); + } else { + await this.testCaseExecutionRepository.markAsFailed({ + testRunId: testRun.id, + pastExecutionId, + errorCode: 'UNKNOWN_ERROR', + trx, + }); + + // Report unexpected errors + this.errorReporter.error(e); + } + }); } } @@ -435,6 +504,7 @@ export class TestRunnerService { }); } else { const aggregatedMetrics = metrics.getAggregatedMetrics(); + await this.testRunRepository.markAsCompleted(testRun.id, aggregatedMetrics); this.logger.debug('Test run finished', { testId: test.id }); @@ -450,7 +520,10 @@ export class TestRunnerService { await this.testRunRepository.markAsCancelled(testRun.id, trx); await this.testCaseExecutionRepository.markAllPendingAsCancelled(testRun.id, trx); }); + } else if (e instanceof TestRunError) { + await this.testRunRepository.markAsError(testRun.id, e.code, e.extra as IDataObject); } else { + await this.testRunRepository.markAsError(testRun.id, 'UNKNOWN_ERROR'); throw e; } } finally { 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 be40b7ecfbbe8..3326c0dbe6741 100644 --- a/packages/cli/src/evaluation.ee/test-runner/utils.ee.ts +++ b/packages/cli/src/evaluation.ee/test-runner/utils.ee.ts @@ -1,8 +1,11 @@ import assert from 'assert'; import type { IRunExecutionData, IPinData, IWorkflowBase } from 'n8n-workflow'; +import type { TestCaseExecution } from '@/databases/entities/test-case-execution.ee'; import type { MockedNodeItem } from '@/databases/entities/test-definition.ee'; import type { WorkflowEntity } from '@/databases/entities/workflow-entity'; +import type { TestRunFinalResult } from '@/databases/repositories/test-run.repository.ee'; +import { TestCaseExecutionError } from '@/evaluation.ee/test-runner/errors.ee'; /** * Extracts the execution data from the past execution @@ -41,6 +44,8 @@ export function createPinData( if (nodeData?.[0]?.data?.main?.[0]) { pinData[nodeName] = nodeData[0]?.data?.main?.[0]; + } else { + throw new TestCaseExecutionError('MOCKED_NODE_DOES_NOT_EXIST'); } } } @@ -58,3 +63,31 @@ export function getPastExecutionTriggerNode(executionData: IRunExecutionData) { return !data[0].source || data[0].source.length === 0 || data[0].source[0] === null; }); } + +/** + * Returns the final result of the test run based on the test case executions. + * The final result is the most severe status among all test case executions' statuses. + */ +export function getTestRunFinalResult(testCaseExecutions: TestCaseExecution[]): TestRunFinalResult { + // Priority of statuses: error > warning > success + const severityMap: Record = { + error: 3, + warning: 2, + success: 1, + }; + + let finalResult: TestRunFinalResult = 'success'; + + for (const testCaseExecution of testCaseExecutions) { + if (['error', 'warning'].includes(testCaseExecution.status)) { + if ( + testCaseExecution.status in severityMap && + severityMap[testCaseExecution.status as TestRunFinalResult] > severityMap[finalResult] + ) { + finalResult = testCaseExecution.status as TestRunFinalResult; + } + } + } + + return finalResult; +} diff --git a/packages/cli/src/evaluation.ee/test-runs.controller.ee.ts b/packages/cli/src/evaluation.ee/test-runs.controller.ee.ts index 61853e6e4288b..00f4ead191a1a 100644 --- a/packages/cli/src/evaluation.ee/test-runs.controller.ee.ts +++ b/packages/cli/src/evaluation.ee/test-runs.controller.ee.ts @@ -73,9 +73,11 @@ export class TestRunsController { @Get('/:testDefinitionId/runs/:id') async getOne(req: TestRunsRequest.GetOne) { + const { testDefinitionId, id } = req.params; + await this.getTestDefinition(req); - return await this.getTestRun(req); + return await this.testRunRepository.getTestRunSummaryById(testDefinitionId, id); } @Get('/:testDefinitionId/runs/:id/cases') diff --git a/packages/editor-ui/src/api/testDefinition.ee.ts b/packages/editor-ui/src/api/testDefinition.ee.ts index 52e344d6081ae..4225a93559b29 100644 --- a/packages/editor-ui/src/api/testDefinition.ee.ts +++ b/packages/editor-ui/src/api/testDefinition.ee.ts @@ -43,12 +43,15 @@ export interface UpdateTestResponse { export interface TestRunRecord { id: string; testDefinitionId: string; - status: 'new' | 'running' | 'completed' | 'error' | 'cancelled'; + status: 'new' | 'running' | 'completed' | 'error' | 'cancelled' | 'warning' | 'success'; metrics?: Record; createdAt: string; updatedAt: string; runAt: string; completedAt: string; + errorCode?: string; + errorDetails?: Record; + finalResult?: 'success' | 'error' | 'warning'; } interface GetTestRunParams { diff --git a/packages/editor-ui/src/components/TestDefinition/ListRuns/TestRunsTable.vue b/packages/editor-ui/src/components/TestDefinition/ListRuns/TestRunsTable.vue index f5222dccbb861..aa8726fce19d9 100644 --- a/packages/editor-ui/src/components/TestDefinition/ListRuns/TestRunsTable.vue +++ b/packages/editor-ui/src/components/TestDefinition/ListRuns/TestRunsTable.vue @@ -22,6 +22,17 @@ const locale = useI18n(); const navigateToRunDetail = (run: TestRunRecord) => emit('getRunDetail', run); const selectedRows = ref([]); +// Combine test run statuses and finalResult to get the final status +const runSummaries = computed(() => { + return props.runs.map(({ status, finalResult, ...run }) => { + if (status === 'completed' && finalResult) { + return { ...run, status: finalResult }; + } + + return { ...run, status }; + }); +}); + const metrics = computed(() => { return props.runs.reduce((acc, run) => { const metricKeys = Object.keys(run.metrics ?? {}); @@ -29,6 +40,19 @@ const metrics = computed(() => { }, [] as string[]); }); +const getErrorTooltipLinkRoute = (row: TestRunRecord) => { + if (row.errorCode === 'EVALUATION_WORKFLOW_NOT_FOUND') { + return { + name: VIEWS.TEST_DEFINITION_EDIT, + params: { + testId: row.testDefinitionId, + }, + }; + } + + return undefined; +}; + const columns = computed((): Array> => { return [ { @@ -51,15 +75,17 @@ const columns = computed((): Array> => { { text: locale.baseText('testDefinition.listRuns.status.error'), value: 'error' }, { text: locale.baseText('testDefinition.listRuns.status.cancelled'), value: 'cancelled' }, ], + errorRoute: getErrorTooltipLinkRoute, filterMethod: (value: string, row: TestRunRecord) => row.status === value, }, { prop: 'date', label: locale.baseText('testDefinition.listRuns.runDate'), sortable: true, - formatter: (row: TestRunRecord) => convertToDisplayDate(new Date(row.runAt).getTime()), + formatter: (row: TestRunRecord) => + convertToDisplayDate(new Date(row.runAt ?? row.createdAt).getTime()), sortMethod: (a: TestRunRecord, b: TestRunRecord) => - new Date(a.runAt).getTime() - new Date(b.runAt).getTime(), + new Date(a.runAt ?? a.createdAt).getTime() - new Date(b.runAt ?? b.createdAt).getTime(), }, ...metrics.value.map((metric) => ({ @@ -104,7 +130,7 @@ async function deleteRuns() { -import { useI18n } from '@/composables/useI18n'; import type { TestTableColumn } from './TestTableBase.vue'; import { useRouter } from 'vue-router'; @@ -12,35 +11,7 @@ defineEmits<{ click: []; }>(); -const locale = useI18n(); const router = useRouter(); -interface WithStatus { - status: string; -} - -function hasStatus(row: unknown): row is WithStatus { - return typeof row === 'object' && row !== null && 'status' in row; -} - -const statusThemeMap: Record = { - new: 'default', - running: 'warning', - evaluation_running: 'warning', - completed: 'success', - error: 'danger', - success: 'success', - cancelled: 'default', -}; - -const statusLabelMap: Record = { - new: locale.baseText('testDefinition.listRuns.status.new'), - running: locale.baseText('testDefinition.listRuns.status.running'), - evaluation_running: locale.baseText('testDefinition.listRuns.status.evaluating'), - completed: locale.baseText('testDefinition.listRuns.status.completed'), - error: locale.baseText('testDefinition.listRuns.status.error'), - success: locale.baseText('testDefinition.listRuns.status.success'), - cancelled: locale.baseText('testDefinition.listRuns.status.cancelled'), -}; function hasProperty(row: unknown, prop: string): row is Record { return typeof row === 'object' && row !== null && prop in row; @@ -64,14 +35,6 @@ const getCellContent = (column: TestTableColumn, row: T) => { - - {{ statusLabelMap[row.status] }} - -
{{ getCellContent(column, row) }}
diff --git a/packages/editor-ui/src/components/TestDefinition/shared/TableStatusCell.vue b/packages/editor-ui/src/components/TestDefinition/shared/TableStatusCell.vue new file mode 100644 index 0000000000000..9ae7ebd9c4e2b --- /dev/null +++ b/packages/editor-ui/src/components/TestDefinition/shared/TableStatusCell.vue @@ -0,0 +1,104 @@ + + + + + diff --git a/packages/editor-ui/src/components/TestDefinition/shared/TestTableBase.vue b/packages/editor-ui/src/components/TestDefinition/shared/TestTableBase.vue index ae0d9eea11b3a..91590422b6898 100644 --- a/packages/editor-ui/src/components/TestDefinition/shared/TestTableBase.vue +++ b/packages/editor-ui/src/components/TestDefinition/shared/TestTableBase.vue @@ -1,6 +1,7 @@