refactor(core): Separate listeners in scaling service (no-changelog) (#10487)

packages/cli/src/scaling/__tests__/scaling.service.test.ts

@@ -3,7 +3,7 @@ import { ScalingService } from '../scaling.service';
 import { JOB_TYPE_NAME, QUEUE_NAME } from '../constants';
 import config from '@/config';
 import * as BullModule from 'bull';
-import type { Job, JobData, JobOptions, JobQueue } from '../types';
+import type { Job, JobData, JobOptions, JobQueue } from '../scaling.types';
 import { ApplicationError } from 'n8n-workflow';
 import { mockInstance } from '@test/mocking';
 import { GlobalConfig } from '@n8n/config';
@@ -22,6 +22,8 @@ jest.mock('bull', () => ({
 }));
 
 describe('ScalingService', () => {
+	const Bull = jest.mocked(BullModule.default);
+
 	const globalConfig = mockInstance(GlobalConfig, {
 		queue: {
 			bull: {
@@ -40,11 +42,29 @@ describe('ScalingService', () => {
 	const instanceSettings = Container.get(InstanceSettings);
 	const orchestrationService = mock<OrchestrationService>({ isMultiMainSetupEnabled: false });
 	const jobProcessor = mock<JobProcessor>();
+
 	let scalingService: ScalingService;
 
+	let registerMainListenersSpy: jest.SpyInstance;
+	let registerWorkerListenersSpy: jest.SpyInstance;
+	let scheduleQueueRecoverySpy: jest.SpyInstance;
+	let stopQueueRecoverySpy: jest.SpyInstance;
+	let getRunningJobsCountSpy: jest.SpyInstance;
+
+	const bullConstructorArgs = [
+		QUEUE_NAME,
+		{
+			prefix: globalConfig.queue.bull.prefix,
+			settings: globalConfig.queue.bull.settings,
+			createClient: expect.any(Function),
+		},
+	];
+
 	beforeEach(() => {
 		jest.clearAllMocks();
 		config.set('generic.instanceType', 'main');
+		instanceSettings.markAsLeader();
+
 		scalingService = new ScalingService(
 			mock(),
 			mock(),
@@ -54,98 +74,93 @@ describe('ScalingService', () => {
 			instanceSettings,
 			orchestrationService,
 		);
-	});
 
-	afterEach(() => {
-		scalingService.stopQueueRecovery();
+		getRunningJobsCountSpy = jest.spyOn(scalingService, 'getRunningJobsCount');
+
+		// @ts-expect-error Private method
+		ScalingService.prototype.scheduleQueueRecovery = jest.fn();
+		// @ts-expect-error Private method
+		registerMainListenersSpy = jest.spyOn(scalingService, 'registerMainListeners');
+		// @ts-expect-error Private method
+		registerWorkerListenersSpy = jest.spyOn(scalingService, 'registerWorkerListeners');
+		// @ts-expect-error Private method
+		scheduleQueueRecoverySpy = jest.spyOn(scalingService, 'scheduleQueueRecovery');
+		// @ts-expect-error Private method
+		stopQueueRecoverySpy = jest.spyOn(scalingService, 'stopQueueRecovery');
 	});
 
 	describe('setupQueue', () => {
-		it('should set up the queue', async () => {
-			/**
-			 * Arrange
-			 */
-			const { prefix, settings } = globalConfig.queue.bull;
-			const Bull = jest.mocked(BullModule.default);
+		describe('if leader main', () => {
+			it('should set up queue + listeners + queue recovery', async () => {
+				await scalingService.setupQueue();
 
-			/**
-			 * Act
-			 */
-			await scalingService.setupQueue();
-
-			/**
-			 * Assert
-			 */
-			expect(Bull).toHaveBeenCalledWith(QUEUE_NAME, {
-				prefix,
-				settings,
-				createClient: expect.any(Function),
+				expect(Bull).toHaveBeenCalledWith(...bullConstructorArgs);
+				expect(registerMainListenersSpy).toHaveBeenCalled();
+				expect(registerWorkerListenersSpy).not.toHaveBeenCalled();
+				expect(scheduleQueueRecoverySpy).toHaveBeenCalled();
+			});
+		});
+
+		describe('if follower main', () => {
+			it('should set up queue + listeners', async () => {
+				instanceSettings.markAsFollower();
+
+				await scalingService.setupQueue();
+
+				expect(Bull).toHaveBeenCalledWith(...bullConstructorArgs);
+				expect(registerMainListenersSpy).toHaveBeenCalled();
+				expect(registerWorkerListenersSpy).not.toHaveBeenCalled();
+				expect(scheduleQueueRecoverySpy).not.toHaveBeenCalled();
+			});
+		});
+
+		describe('if worker', () => {
+			it('should set up queue + listeners', async () => {
+				// @ts-expect-error Private field
+				scalingService.instanceType = 'worker';
+
+				await scalingService.setupQueue();
+
+				expect(Bull).toHaveBeenCalledWith(...bullConstructorArgs);
+				expect(registerWorkerListenersSpy).toHaveBeenCalled();
+				expect(registerMainListenersSpy).not.toHaveBeenCalled();
 			});
-			expect(queue.on).toHaveBeenCalledWith('global:progress', expect.any(Function));
-			expect(queue.on).toHaveBeenCalledWith('error', expect.any(Function));
 		});
 	});
 
 	describe('setupWorker', () => {
 		it('should set up a worker with concurrency', async () => {
-			/**
-			 * Arrange
-			 */
-			config.set('generic.instanceType', 'worker');
-			const scalingService = new ScalingService(
-				mock(),
-				mock(),
-				mock(),
-				globalConfig,
-				mock(),
-				instanceSettings,
-				orchestrationService,
-			);
+			// @ts-expect-error Private field
+			scalingService.instanceType = 'worker';
 			await scalingService.setupQueue();
 			const concurrency = 5;
 
-			/**
-			 * Act
-			 */
 			scalingService.setupWorker(concurrency);
 
-			/**
-			 * Assert
-			 */
 			expect(queue.process).toHaveBeenCalledWith(JOB_TYPE_NAME, concurrency, expect.any(Function));
 		});
 
 		it('should throw if called on a non-worker instance', async () => {
-			/**
-			 * Arrange
-			 */
 			await scalingService.setupQueue();
 
-			/**
-			 * Act and Assert
-			 */
+			expect(() => scalingService.setupWorker(5)).toThrow();
+		});
+
+		it('should throw if called before queue is ready', async () => {
+			// @ts-expect-error Private field
+			scalingService.instanceType = 'worker';
+
 			expect(() => scalingService.setupWorker(5)).toThrow();
 		});
 	});
 
 	describe('stop', () => {
-		it('should pause the queue, check for running jobs, and stop queue recovery', async () => {
-			/**
-			 * Arrange
-			 */
+		it('should pause queue, wait for running jobs, stop queue recovery', async () => {
 			await scalingService.setupQueue();
 			jobProcessor.getRunningJobIds.mockReturnValue([]);
-			const stopQueueRecoverySpy = jest.spyOn(scalingService, 'stopQueueRecovery');
-			const getRunningJobsCountSpy = jest.spyOn(scalingService, 'getRunningJobsCount');
 
-			/**
-			 * Act
-			 */
 			await scalingService.stop();
 
-			/**
-			 * Assert
-			 */
 			expect(queue.pause).toHaveBeenCalledWith(true, true);
 			expect(stopQueueRecoverySpy).toHaveBeenCalled();
 			expect(getRunningJobsCountSpy).toHaveBeenCalled();
@@ -154,62 +169,35 @@ describe('ScalingService', () => {
 
 	describe('pingQueue', () => {
 		it('should ping the queue', async () => {
-			/**
-			 * Arrange
-			 */
 			await scalingService.setupQueue();
 
-			/**
-			 * Act
-			 */
 			await scalingService.pingQueue();
 
-			/**
-			 * Assert
-			 */
 			expect(queue.client.ping).toHaveBeenCalled();
 		});
 	});
 
 	describe('addJob', () => {
 		it('should add a job', async () => {
-			/**
-			 * Arrange
-			 */
 			await scalingService.setupQueue();
 			queue.add.mockResolvedValue(mock<Job>({ id: '456' }));
 
-			/**
-			 * Act
-			 */
 			const jobData = mock<JobData>({ executionId: '123' });
 			const jobOptions = mock<JobOptions>();
 			await scalingService.addJob(jobData, jobOptions);
 
-			/**
-			 * Assert
-			 */
 			expect(queue.add).toHaveBeenCalledWith(JOB_TYPE_NAME, jobData, jobOptions);
 		});
 	});
 
 	describe('getJob', () => {
 		it('should get a job', async () => {
-			/**
-			 * Arrange
-			 */
 			await scalingService.setupQueue();
 			const jobId = '123';
 			queue.getJob.mockResolvedValue(mock<Job>({ id: jobId }));
 
-			/**
-			 * Act
-			 */
 			const job = await scalingService.getJob(jobId);
 
-			/**
-			 * Assert
-			 */
 			expect(queue.getJob).toHaveBeenCalledWith(jobId);
 			expect(job?.id).toBe(jobId);
 		});
@@ -217,88 +205,49 @@ describe('ScalingService', () => {
 
 	describe('findJobsByStatus', () => {
 		it('should find jobs by status', async () => {
-			/**
-			 * Arrange
-			 */
 			await scalingService.setupQueue();
 			queue.getJobs.mockResolvedValue([mock<Job>({ id: '123' })]);
 
-			/**
-			 * Act
-			 */
 			const jobs = await scalingService.findJobsByStatus(['active']);
 
-			/**
-			 * Assert
-			 */
 			expect(queue.getJobs).toHaveBeenCalledWith(['active']);
 			expect(jobs).toHaveLength(1);
 			expect(jobs.at(0)?.id).toBe('123');
 		});
 
 		it('should filter out `null` in Redis response', async () => {
-			/**
-			 * Arrange
-			 */
 			await scalingService.setupQueue();
 			// @ts-expect-error - Untyped but possible Redis response
 			queue.getJobs.mockResolvedValue([mock<Job>(), null]);
 
-			/**
-			 * Act
-			 */
 			const jobs = await scalingService.findJobsByStatus(['waiting']);
 
-			/**
-			 * Assert
-			 */
 			expect(jobs).toHaveLength(1);
 		});
 	});
 
 	describe('stopJob', () => {
 		it('should stop an active job', async () => {
-			/**
-			 * Arrange
-			 */
 			await scalingService.setupQueue();
 			const job = mock<Job>({ isActive: jest.fn().mockResolvedValue(true) });
 
-			/**
-			 * Act
-			 */
 			const result = await scalingService.stopJob(job);
 
-			/**
-			 * Assert
-			 */
 			expect(job.progress).toHaveBeenCalledWith({ kind: 'abort-job' });
 			expect(result).toBe(true);
 		});
 
 		it('should stop an inactive job', async () => {
-			/**
-			 * Arrange
-			 */
 			await scalingService.setupQueue();
 			const job = mock<Job>({ isActive: jest.fn().mockResolvedValue(false) });
 
-			/**
-			 * Act
-			 */
 			const result = await scalingService.stopJob(job);
 
-			/**
-			 * Assert
-			 */
 			expect(job.remove).toHaveBeenCalled();
 			expect(result).toBe(true);
 		});
 
 		it('should report failure to stop a job', async () => {
-			/**
-			 * Arrange
-			 */
 			await scalingService.setupQueue();
 			const job = mock<Job>({
 				isActive: jest.fn().mockImplementation(() => {
@@ -306,53 +255,9 @@ describe('ScalingService', () => {
 				}),
 			});
 
-			/**
-			 * Act
-			 */
 			const result = await scalingService.stopJob(job);
 
-			/**
-			 * Assert
-			 */
 			expect(result).toBe(false);
 		});
 	});
-
-	describe('scheduleQueueRecovery', () => {
-		it('if leader, should schedule queue recovery', async () => {
-			/**
-			 * Arrange
-			 */
-			const scheduleSpy = jest.spyOn(scalingService, 'scheduleQueueRecovery');
-			instanceSettings.markAsLeader();
-
-			/**
-			 * Act
-			 */
-			await scalingService.setupQueue();
-
-			/**
-			 * Assert
-			 */
-			expect(scheduleSpy).toHaveBeenCalled();
-		});
-
-		it('if follower, should not schedule queue recovery', async () => {
-			/**
-			 * Arrange
-			 */
-			const scheduleSpy = jest.spyOn(scalingService, 'scheduleQueueRecovery');
-			instanceSettings.markAsFollower();
-
-			/**
-			 * Act
-			 */
-			await scalingService.setupQueue();
-
-			/**
-			 * Assert
-			 */
-			expect(scheduleSpy).not.toHaveBeenCalled();
-		});
-	});
 });