From 877511c0e16de9946495f1e8209d0da52b23b449 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Thu, 30 Jan 2025 15:12:26 +0100 Subject: [PATCH] additional tests for saveExecutionProgress --- .../__tests__/save-execution-progress.test.ts | 167 +++++++++++++----- .../save-execution-progress.ts | 63 +++---- 2 files changed, 147 insertions(+), 83 deletions(-) diff --git a/packages/cli/src/execution-lifecycle/__tests__/save-execution-progress.test.ts b/packages/cli/src/execution-lifecycle/__tests__/save-execution-progress.test.ts index 14692ee4ae..62b735bc0a 100644 --- a/packages/cli/src/execution-lifecycle/__tests__/save-execution-progress.test.ts +++ b/packages/cli/src/execution-lifecycle/__tests__/save-execution-progress.test.ts @@ -8,63 +8,142 @@ import { mockInstance } from '@test/mocking'; import { saveExecutionProgress } from '../save-execution-progress'; -mockInstance(Logger); -const errorReporter = mockInstance(ErrorReporter); -const executionRepository = mockInstance(ExecutionRepository); +describe('saveExecutionProgress', () => { + mockInstance(Logger); + const errorReporter = mockInstance(ErrorReporter); + const executionRepository = mockInstance(ExecutionRepository); -afterEach(() => { - jest.clearAllMocks(); -}); + afterEach(() => { + jest.resetAllMocks(); + }); -const commonArgs: [string, string, string, ITaskData, IRunExecutionData] = [ - 'some-workflow-id', - 'some-execution-id', - 'My Node', - {} as ITaskData, - {} as IRunExecutionData, -]; + const commonArgs: [string, string, string, ITaskData, IRunExecutionData] = [ + 'some-workflow-id', + 'some-execution-id', + 'My Node', + {} as ITaskData, + {} as IRunExecutionData, + ]; -test('should ignore on leftover async call', async () => { - executionRepository.findSingleExecution.mockResolvedValue({ - finished: true, - } as IExecutionResponse); + test('should not try to update non-existent executions', async () => { + executionRepository.findSingleExecution.mockResolvedValue(undefined); - await saveExecutionProgress(...commonArgs); + await saveExecutionProgress(...commonArgs); + expect(executionRepository.updateExistingExecution).not.toHaveBeenCalled(); + }); - expect(executionRepository.updateExistingExecution).not.toHaveBeenCalled(); -}); + test('should handle DB errors on execution lookup', async () => { + const error = new Error('Something went wrong'); + executionRepository.findSingleExecution.mockImplementation(() => { + throw error; + }); -test('should update execution when saving progress is enabled', async () => { - executionRepository.findSingleExecution.mockResolvedValue({} as IExecutionResponse); + await saveExecutionProgress(...commonArgs); - await saveExecutionProgress(...commonArgs); + expect(executionRepository.updateExistingExecution).not.toHaveBeenCalled(); + expect(errorReporter.error).toHaveBeenCalledWith(error); + }); - expect(executionRepository.updateExistingExecution).toHaveBeenCalledWith('some-execution-id', { - data: { - executionData: undefined, - resultData: { - lastNodeExecuted: 'My Node', - runData: { - 'My Node': [{}], + test('should handle DB errors when updating the execution', async () => { + const error = new Error('Something went wrong'); + executionRepository.findSingleExecution.mockResolvedValue({} as IExecutionResponse); + executionRepository.updateExistingExecution.mockImplementation(() => { + throw error; + }); + + await saveExecutionProgress(...commonArgs); + + expect(executionRepository.findSingleExecution).toHaveBeenCalled(); + expect(executionRepository.updateExistingExecution).toHaveBeenCalled(); + expect(errorReporter.error).toHaveBeenCalledWith(error); + }); + + test('should not try to update finished executions', async () => { + executionRepository.findSingleExecution.mockResolvedValue({ + finished: true, + } as IExecutionResponse); + + await saveExecutionProgress(...commonArgs); + + expect(executionRepository.updateExistingExecution).not.toHaveBeenCalled(); + }); + + test('should populate `.data` when it is missing', async () => { + const fullExecutionData = {} as IExecutionResponse; + executionRepository.findSingleExecution.mockResolvedValue(fullExecutionData); + + await saveExecutionProgress(...commonArgs); + + expect(fullExecutionData).toEqual({ + data: { + executionData: undefined, + resultData: { + lastNodeExecuted: 'My Node', + runData: { + 'My Node': [{}], + }, + }, + startData: {}, + }, + status: 'running', + }); + + expect(executionRepository.updateExistingExecution).toHaveBeenCalledWith( + 'some-execution-id', + fullExecutionData, + ); + + expect(errorReporter.error).not.toHaveBeenCalled(); + }); + + test('should augment `.data` if it already exists', async () => { + const fullExecutionData = { + data: { + startData: {}, + resultData: { + runData: { + 'My Node': [{}], + }, }, }, - startData: {}, - }, - status: 'running', + } as unknown as IExecutionResponse; + executionRepository.findSingleExecution.mockResolvedValue(fullExecutionData); + + await saveExecutionProgress(...commonArgs); + + expect(fullExecutionData).toEqual({ + data: { + executionData: undefined, + resultData: { + lastNodeExecuted: 'My Node', + runData: { + 'My Node': [{}, {}], + }, + }, + startData: {}, + }, + status: 'running', + }); + + expect(executionRepository.updateExistingExecution).toHaveBeenCalledWith( + 'some-execution-id', + fullExecutionData, + ); }); - expect(errorReporter.error).not.toHaveBeenCalled(); -}); + test('should set last executed node correctly', async () => { + const fullExecutionData = { + data: { + resultData: { + lastNodeExecuted: 'Another Node', + runData: {}, + }, + }, + } as unknown as IExecutionResponse; + executionRepository.findSingleExecution.mockResolvedValue(fullExecutionData); -test('should report error on failure', async () => { - const error = new Error('Something went wrong'); + await saveExecutionProgress(...commonArgs); - executionRepository.findSingleExecution.mockImplementation(() => { - throw error; + expect(fullExecutionData.data.resultData.lastNodeExecuted).toEqual('My Node'); }); - - await saveExecutionProgress(...commonArgs); - - expect(executionRepository.updateExistingExecution).not.toHaveBeenCalled(); - expect(errorReporter.error).toHaveBeenCalledWith(error); }); diff --git a/packages/cli/src/execution-lifecycle/save-execution-progress.ts b/packages/cli/src/execution-lifecycle/save-execution-progress.ts index f70cf28430..571cc7c96f 100644 --- a/packages/cli/src/execution-lifecycle/save-execution-progress.ts +++ b/packages/cli/src/execution-lifecycle/save-execution-progress.ts @@ -12,6 +12,8 @@ export async function saveExecutionProgress( executionData: IRunExecutionData, ) { const logger = Container.get(Logger); + const executionRepository = Container.get(ExecutionRepository); + const errorReporter = Container.get(ErrorReporter); try { logger.debug(`Save execution progress to database for execution ID ${executionId} `, { @@ -19,13 +21,10 @@ export async function saveExecutionProgress( nodeName, }); - const fullExecutionData = await Container.get(ExecutionRepository).findSingleExecution( - executionId, - { - includeData: true, - unflattenData: true, - }, - ); + const fullExecutionData = await executionRepository.findSingleExecution(executionId, { + includeData: true, + unflattenData: true, + }); if (!fullExecutionData) { // Something went badly wrong if this happens. @@ -40,29 +39,22 @@ export async function saveExecutionProgress( return; } - if (fullExecutionData.data === undefined) { - fullExecutionData.data = { - startData: {}, - resultData: { - runData: {}, - }, - executionData: { - contextData: {}, - metadata: {}, - nodeExecutionStack: [], - waitingExecution: {}, - waitingExecutionSource: {}, - }, - }; - } + fullExecutionData.data ??= { + startData: {}, + resultData: { + runData: {}, + }, + executionData: { + contextData: {}, + metadata: {}, + nodeExecutionStack: [], + waitingExecution: {}, + waitingExecutionSource: {}, + }, + }; - if (Array.isArray(fullExecutionData.data.resultData.runData[nodeName])) { - // Append data if array exists - fullExecutionData.data.resultData.runData[nodeName].push(data); - } else { - // Initialize array and save data - fullExecutionData.data.resultData.runData[nodeName] = [data]; - } + const { runData } = fullExecutionData.data.resultData; + (runData[nodeName] ??= []).push(data); fullExecutionData.data.executionData = executionData.executionData; @@ -71,14 +63,11 @@ export async function saveExecutionProgress( fullExecutionData.status = 'running'; - await Container.get(ExecutionRepository).updateExistingExecution( - executionId, - fullExecutionData, - ); + await executionRepository.updateExistingExecution(executionId, fullExecutionData); } catch (e) { const error = e instanceof Error ? e : new Error(`${e}`); - Container.get(ErrorReporter).error(error); + errorReporter.error(error); // TODO: Improve in the future! // Errors here might happen because of database access // For busy machines, we may get "Database is locked" errors. @@ -86,11 +75,7 @@ export async function saveExecutionProgress( // We do this to prevent crashes and executions ending in `unknown` state. logger.error( `Failed saving execution progress to database for execution ID ${executionId} (hookFunctionsPreExecute, nodeExecuteAfter)`, - { - ...error, - executionId, - workflowId, - }, + { error, executionId, workflowId }, ); } }