From a2e2ec5442ccce59217d22a23abcd3b56d449c29 Mon Sep 17 00:00:00 2001 From: Omar Ajoue Date: Fri, 10 Feb 2023 15:24:20 +0100 Subject: [PATCH] test: Add tests for ActiveWorkflowRunner class (#5278) --- packages/cli/src/ActiveWorkflowRunner.ts | 40 +-- .../test/unit/ActiveWorkflowRunner.test.ts | 265 ++++++++++++++++++ packages/cli/test/unit/Helpers.ts | 67 +++++ .../cli/test/unit/PermissionChecker.test.ts | 24 +- 4 files changed, 357 insertions(+), 39 deletions(-) create mode 100644 packages/cli/test/unit/ActiveWorkflowRunner.test.ts diff --git a/packages/cli/src/ActiveWorkflowRunner.ts b/packages/cli/src/ActiveWorkflowRunner.ts index 376f6c15e4264..4eddb668d278f 100644 --- a/packages/cli/src/ActiveWorkflowRunner.ts +++ b/packages/cli/src/ActiveWorkflowRunner.ts @@ -1,7 +1,6 @@ /* eslint-disable prefer-spread */ /* eslint-disable @typescript-eslint/no-non-null-assertion */ /* eslint-disable no-param-reassign */ -/* eslint-disable no-console */ /* eslint-disable no-await-in-loop */ /* eslint-disable no-restricted-syntax */ /* eslint-disable @typescript-eslint/no-floating-promises */ @@ -93,7 +92,7 @@ export class ActiveWorkflowRunner { // so instead of pulling all the active webhooks just pull the actives that have a trigger const workflowsData: IWorkflowDb[] = (await Db.collections.Workflow.find({ where: { active: true }, - relations: ['shared', 'shared.user', 'shared.user.globalRole'], + relations: ['shared', 'shared.user', 'shared.user.globalRole', 'shared.role'], })) as IWorkflowDb[]; if (!config.getEnv('endpoints.skipWebhooksDeregistrationOnShutdown')) { @@ -109,12 +108,12 @@ export class ActiveWorkflowRunner { } if (workflowsData.length !== 0) { - console.info(' ================================'); - console.info(' Start Active Workflows:'); - console.info(' ================================'); + Logger.info(' ================================'); + Logger.info(' Start Active Workflows:'); + Logger.info(' ================================'); for (const workflowData of workflowsData) { - console.log(` - ${workflowData.name} (ID: ${workflowData.id})`); + Logger.info(` - ${workflowData.name} (ID: ${workflowData.id})`); Logger.debug(`Initializing active workflow "${workflowData.name}" (startup)`, { workflowName: workflowData.name, workflowId: workflowData.id, @@ -125,14 +124,14 @@ export class ActiveWorkflowRunner { workflowName: workflowData.name, workflowId: workflowData.id, }); - console.log(' => Started'); + Logger.info(' => Started'); } catch (error) { ErrorReporter.error(error); - console.log( + Logger.info( ' => ERROR: Workflow could not be activated on first try, keep on trying', ); // eslint-disable-next-line @typescript-eslint/restrict-template-expressions - console.log(` ${error.message}`); + Logger.info(` ${error.message}`); Logger.error( `Issue on intital workflow activation try "${workflowData.name}" (startup)`, { @@ -166,7 +165,10 @@ export class ActiveWorkflowRunner { } const activeWorkflows = await this.getActiveWorkflows(); - activeWorkflowIds = [...activeWorkflowIds, ...activeWorkflows.map((workflow) => workflow.id)]; + activeWorkflowIds = [ + ...activeWorkflowIds, + ...activeWorkflows.map((workflow) => workflow.id.toString()), + ]; // Make sure IDs are unique activeWorkflowIds = Array.from(new Set(activeWorkflowIds)); @@ -480,7 +482,7 @@ export class ActiveWorkflowRunner { try { await this.removeWorkflowWebhooks(workflow.id as string); } catch (error) { - console.error( + Logger.error( // eslint-disable-next-line @typescript-eslint/restrict-template-expressions `Could not remove webhooks of workflow "${workflow.id}" because of error: "${error.message}"`, ); @@ -649,7 +651,7 @@ export class ActiveWorkflowRunner { .catch(donePromise.reject); }); } else { - executePromise.catch(console.error); + executePromise.catch(Logger.error); } }; @@ -706,7 +708,7 @@ export class ActiveWorkflowRunner { .catch(donePromise.reject); }); } else { - executePromise.catch(console.error); + executePromise.catch(Logger.error); } }; returnFunctions.emitError = async (error: Error): Promise => { @@ -783,7 +785,7 @@ export class ActiveWorkflowRunner { if (workflowData === undefined) { workflowData = (await Db.collections.Workflow.findOne({ where: { id: workflowId }, - relations: ['shared', 'shared.user', 'shared.user.globalRole'], + relations: ['shared', 'shared.user', 'shared.user.globalRole', 'shared.role'], })) as IWorkflowDb; } @@ -811,9 +813,13 @@ export class ActiveWorkflowRunner { } const mode = 'trigger'; - const additionalData = await WorkflowExecuteAdditionalData.getBase( - (workflowData as WorkflowEntity).shared[0].user.id, + const workflowOwner = (workflowData as WorkflowEntity).shared.find( + (shared) => shared.role.name === 'owner', ); + if (!workflowOwner) { + throw new Error('Workflow cannot be activated because it has no owner'); + } + const additionalData = await WorkflowExecuteAdditionalData.getBase(workflowOwner.user.id); const getTriggerFunctions = this.getExecuteTriggerFunctions( workflowData, additionalData, @@ -977,7 +983,7 @@ export class ActiveWorkflowRunner { await this.removeWorkflowWebhooks(workflowId); } catch (error) { ErrorReporter.error(error); - console.error( + Logger.error( // eslint-disable-next-line @typescript-eslint/restrict-template-expressions `Could not remove webhooks of workflow "${workflowId}" because of error: "${error.message}"`, ); diff --git a/packages/cli/test/unit/ActiveWorkflowRunner.test.ts b/packages/cli/test/unit/ActiveWorkflowRunner.test.ts new file mode 100644 index 0000000000000..e45dd334cb0ce --- /dev/null +++ b/packages/cli/test/unit/ActiveWorkflowRunner.test.ts @@ -0,0 +1,265 @@ +import { v4 as uuid } from 'uuid'; +import { mocked } from 'jest-mock'; + +import { ICredentialTypes, LoggerProxy, NodeOperationError, Workflow } from 'n8n-workflow'; + +import { ActiveWorkflowRunner } from '@/ActiveWorkflowRunner'; +import * as Db from '@/Db'; +import { WorkflowEntity } from '@/databases/entities/WorkflowEntity'; +import { SharedWorkflow } from '@/databases/entities/SharedWorkflow'; +import { Role } from '@/databases/entities/Role'; +import { User } from '@/databases/entities/User'; +import { getLogger } from '@/Logger'; +import { NodeTypes } from '@/NodeTypes'; +import { CredentialTypes } from '@/CredentialTypes'; +import { randomEmail, randomName } from '../integration/shared/random'; +import * as Helpers from './Helpers'; +import { WorkflowExecuteAdditionalData } from '@/index'; +import { WorkflowRunner } from '@/WorkflowRunner'; + +/** + * TODO: + * - test workflow webhooks activation (that trigger `executeWebhook`and other webhook methods) + * - test activation error catching and getters such as `getActivationError` (requires building a workflow that fails to activate) + * - test queued workflow activation functions (might need to create a non-working workflow to test this) + */ + +let databaseActiveWorkflowsCount = 0; +let databaseActiveWorkflowsList: WorkflowEntity[] = []; + +const generateWorkflows = (count: number): WorkflowEntity[] => { + const workflows: WorkflowEntity[] = []; + const ownerRole = new Role(); + ownerRole.scope = 'workflow'; + ownerRole.name = 'owner'; + ownerRole.id = '1'; + + const owner = new User(); + owner.id = uuid(); + owner.firstName = randomName(); + owner.lastName = randomName(); + owner.email = randomEmail(); + + for (let i = 0; i < count; i++) { + const workflow = new WorkflowEntity(); + Object.assign(workflow, { + id: i + 1, + name: randomName(), + active: true, + createdAt: new Date(), + updatedAt: new Date(), + nodes: [ + { + parameters: { + rule: { + interval: [{}], + }, + }, + id: uuid(), + name: 'Schedule Trigger', + type: 'n8n-nodes-base.scheduleTrigger', + typeVersion: 1, + position: [900, 460], + }, + ], + connections: {}, + tags: [], + }); + const sharedWorkflow = new SharedWorkflow(); + sharedWorkflow.workflowId = workflow.id; + sharedWorkflow.role = ownerRole; + sharedWorkflow.user = owner; + + workflow.shared = [sharedWorkflow]; + + workflows.push(workflow); + } + databaseActiveWorkflowsList = workflows; + return workflows; +}; + +const MOCK_NODE_TYPES_DATA = Helpers.mockNodeTypesData(['scheduleTrigger'], { + addTrigger: true, +}); + +jest.mock('@/Db', () => { + return { + collections: { + Workflow: { + find: jest.fn(async () => Promise.resolve(generateWorkflows(databaseActiveWorkflowsCount))), + findOne: jest.fn(async (searchParams) => { + const foundWorkflow = databaseActiveWorkflowsList.find( + (workflow) => workflow.id.toString() === searchParams.where.id.toString(), + ); + return Promise.resolve(foundWorkflow); + }), + update: jest.fn(), + createQueryBuilder: jest.fn(() => { + const fakeQueryBuilder = { + update: () => fakeQueryBuilder, + set: () => fakeQueryBuilder, + where: () => fakeQueryBuilder, + execute: () => Promise.resolve(), + }; + return fakeQueryBuilder; + }), + }, + Webhook: { + clear: jest.fn(), + delete: jest.fn(), + }, + }, + }; +}); + +const mockExternalHooksRunFunction = jest.fn(); + +jest.mock('@/ExternalHooks', () => { + return { + ExternalHooks: () => { + return { + run: () => mockExternalHooksRunFunction(), + init: () => Promise.resolve(), + }; + }, + }; +}); + +const workflowCheckIfCanBeActivated = jest.fn(() => true); + +jest + .spyOn(Workflow.prototype, 'checkIfWorkflowCanBeActivated') + .mockImplementation(workflowCheckIfCanBeActivated); + +const removeFunction = jest.spyOn(ActiveWorkflowRunner.prototype, 'remove'); +const removeWebhooksFunction = jest.spyOn(ActiveWorkflowRunner.prototype, 'removeWorkflowWebhooks'); +const workflowRunnerRun = jest.spyOn(WorkflowRunner.prototype, 'run'); +const workflowExecuteAdditionalDataExecuteErrorWorkflowSpy = jest.spyOn( + WorkflowExecuteAdditionalData, + 'executeErrorWorkflow', +); + +describe('ActiveWorkflowRunner', () => { + let activeWorkflowRunner: ActiveWorkflowRunner; + + beforeAll(async () => { + LoggerProxy.init(getLogger()); + NodeTypes({ + loaded: { + nodes: MOCK_NODE_TYPES_DATA, + credentials: {}, + }, + known: { nodes: {}, credentials: {} }, + credentialTypes: {} as ICredentialTypes, + }); + CredentialTypes({ + loaded: { + nodes: MOCK_NODE_TYPES_DATA, + credentials: {}, + }, + known: { nodes: {}, credentials: {} }, + credentialTypes: {} as ICredentialTypes, + }); + }); + + beforeEach(() => { + activeWorkflowRunner = new ActiveWorkflowRunner(); + }); + + afterEach(async () => { + await activeWorkflowRunner.removeAll(); + databaseActiveWorkflowsCount = 0; + jest.clearAllMocks(); + }); + + test('Should initialize activeWorkflowRunner with empty list of active workflows and call External Hooks', async () => { + void (await activeWorkflowRunner.init()); + expect(await activeWorkflowRunner.getActiveWorkflows()).toHaveLength(0); + expect(mocked(Db.collections.Workflow.find)).toHaveBeenCalled(); + expect(mocked(Db.collections.Webhook.clear)).toHaveBeenCalled(); + expect(mockExternalHooksRunFunction).toHaveBeenCalledTimes(1); + }); + + test('Should initialize activeWorkflowRunner with one active workflow', async () => { + databaseActiveWorkflowsCount = 1; + void (await activeWorkflowRunner.init()); + expect(await activeWorkflowRunner.getActiveWorkflows()).toHaveLength( + databaseActiveWorkflowsCount, + ); + expect(mocked(Db.collections.Workflow.find)).toHaveBeenCalled(); + expect(mocked(Db.collections.Webhook.clear)).toHaveBeenCalled(); + expect(mockExternalHooksRunFunction).toHaveBeenCalled(); + }); + + test('Should make sure function checkIfWorkflowCanBeActivated was called for every workflow', async () => { + databaseActiveWorkflowsCount = 2; + void (await activeWorkflowRunner.init()); + expect(workflowCheckIfCanBeActivated).toHaveBeenCalledTimes(databaseActiveWorkflowsCount); + }); + + test('Call to removeAll should remove every workflow', async () => { + databaseActiveWorkflowsCount = 2; + void (await activeWorkflowRunner.init()); + expect(await activeWorkflowRunner.getActiveWorkflows()).toHaveLength( + databaseActiveWorkflowsCount, + ); + void (await activeWorkflowRunner.removeAll()); + expect(removeFunction).toHaveBeenCalledTimes(databaseActiveWorkflowsCount); + }); + + test('Call to remove should also call removeWorkflowWebhooks', async () => { + databaseActiveWorkflowsCount = 1; + void (await activeWorkflowRunner.init()); + expect(await activeWorkflowRunner.getActiveWorkflows()).toHaveLength( + databaseActiveWorkflowsCount, + ); + void (await activeWorkflowRunner.remove('1')); + expect(removeWebhooksFunction).toHaveBeenCalledTimes(1); + }); + + test('Call to isActive should return true for valid workflow', async () => { + databaseActiveWorkflowsCount = 1; + void (await activeWorkflowRunner.init()); + expect(await activeWorkflowRunner.isActive('1')).toBe(true); + }); + + test('Call to isActive should return false for invalid workflow', async () => { + databaseActiveWorkflowsCount = 1; + void (await activeWorkflowRunner.init()); + expect(await activeWorkflowRunner.isActive('2')).toBe(false); + }); + + test('Calling add should call checkIfWorkflowCanBeActivated', async () => { + // Initialize with default (0) workflows + void (await activeWorkflowRunner.init()); + generateWorkflows(1); + void (await activeWorkflowRunner.add('1', 'activate')); + expect(workflowCheckIfCanBeActivated).toHaveBeenCalledTimes(1); + }); + + test('runWorkflow should call run method in WorkflowRunner', async () => { + void (await activeWorkflowRunner.init()); + const workflow = generateWorkflows(1); + const additionalData = await WorkflowExecuteAdditionalData.getBase('fake-user-id'); + + workflowRunnerRun.mockImplementationOnce(() => Promise.resolve('invalid-execution-id')); + + void (await activeWorkflowRunner.runWorkflow( + workflow[0], + workflow[0].nodes[0], + [[]], + additionalData, + 'trigger', + )); + + expect(workflowRunnerRun).toHaveBeenCalledTimes(1); + }); + + test('executeErrorWorkflow should call function with same name in WorkflowExecuteAdditionalData', async () => { + const workflowData = generateWorkflows(1)[0]; + const error = new NodeOperationError(workflowData.nodes[0], 'Fake error message'); + void (await activeWorkflowRunner.init()); + activeWorkflowRunner.executeErrorWorkflow(error, workflowData, 'trigger'); + expect(workflowExecuteAdditionalDataExecuteErrorWorkflowSpy).toHaveBeenCalledTimes(1); + }); +}); diff --git a/packages/cli/test/unit/Helpers.ts b/packages/cli/test/unit/Helpers.ts index 079b606f415ea..afe90f68704a1 100644 --- a/packages/cli/test/unit/Helpers.ts +++ b/packages/cli/test/unit/Helpers.ts @@ -3,6 +3,8 @@ import { INodeType, INodeTypeData, INodeTypes, + ITriggerFunctions, + ITriggerResponse, IVersionedNodeType, NodeHelpers, } from 'n8n-workflow'; @@ -41,6 +43,41 @@ class NodeTypesClass implements INodeTypes { }, }, }, + 'fake-scheduler': { + sourcePath: '', + type: { + description: { + displayName: 'Schedule', + name: 'set', + group: ['input'], + version: 1, + description: 'Schedules execuitons', + defaults: { + name: 'Set', + color: '#0000FF', + }, + inputs: ['main'], + outputs: ['main'], + properties: [ + { + displayName: 'Value1', + name: 'value1', + type: 'string', + default: 'default-value1', + }, + { + displayName: 'Value2', + name: 'value2', + type: 'string', + default: 'default-value2', + }, + ], + }, + trigger: () => { + return Promise.resolve(undefined); + }, + }, + }, }; constructor(nodesAndCredentials?: INodesAndCredentials) { @@ -74,3 +111,33 @@ export function NodeTypes(nodesAndCredentials?: INodesAndCredentials): NodeTypes * after all promises in the microtask queue have settled first. */ export const flushPromises = async () => new Promise(setImmediate); + +export function mockNodeTypesData( + nodeNames: string[], + options?: { + addTrigger?: boolean; + }, +) { + return nodeNames.reduce((acc, nodeName) => { + return ( + (acc[`n8n-nodes-base.${nodeName}`] = { + sourcePath: '', + type: { + description: { + displayName: nodeName, + name: nodeName, + group: [], + description: '', + version: 1, + defaults: {}, + inputs: [], + outputs: [], + properties: [], + }, + trigger: options?.addTrigger ? () => Promise.resolve(undefined) : undefined, + }, + }), + acc + ); + }, {}); +} diff --git a/packages/cli/test/unit/PermissionChecker.test.ts b/packages/cli/test/unit/PermissionChecker.test.ts index 3514e5bf064c3..7fbe35535131d 100644 --- a/packages/cli/test/unit/PermissionChecker.test.ts +++ b/packages/cli/test/unit/PermissionChecker.test.ts @@ -10,7 +10,7 @@ import { import config from '@/config'; import * as Db from '@/Db'; import * as testDb from '../integration/shared/testDb'; -import { NodeTypes as MockNodeTypes } from './Helpers'; +import { mockNodeTypesData, NodeTypes as MockNodeTypes } from './Helpers'; import { UserService } from '@/user/user.service'; import { PermissionChecker } from '@/UserManagement/PermissionChecker'; import * as UserManagementHelper from '@/UserManagement/UserManagementHelper'; @@ -388,24 +388,4 @@ describe('PermissionChecker.checkSubworkflowExecutePolicy', () => { }); }); -const MOCK_NODE_TYPES_DATA = ['start', 'actionNetwork'].reduce((acc, nodeName) => { - return ( - (acc[`n8n-nodes-base.${nodeName}`] = { - sourcePath: '', - type: { - description: { - displayName: nodeName, - name: nodeName, - group: [], - description: '', - version: 1, - defaults: {}, - inputs: [], - outputs: [], - properties: [], - }, - }, - }), - acc - ); -}, {}); +const MOCK_NODE_TYPES_DATA = mockNodeTypesData(['start', 'actionNetwork']);