From 5636a4879da22a6977b23ed335e46ae7da194300 Mon Sep 17 00:00:00 2001 From: jer3k <99355997+jer3k@users.noreply.github.com> Date: Thu, 24 Oct 2024 14:13:31 -0700 Subject: [PATCH] fix schedduler. update test cases. --- .../src/schedulers/__mocks__/create-job.ts | 20 +++++++ .../delete-announcements-scheduler.spec.ts | 29 ++++++++++ .../delete-announcements-scheduler.ts | 4 +- backend/src/schedulers/run.all.spec.ts | 10 ++++ backend/src/schedulers/run.all.ts | 2 + .../v1/services/announcements-service.spec.ts | 55 ++++++++++++++++--- 6 files changed, 111 insertions(+), 9 deletions(-) create mode 100644 backend/src/schedulers/__mocks__/create-job.ts create mode 100644 backend/src/schedulers/delete-announcements-scheduler.spec.ts diff --git a/backend/src/schedulers/__mocks__/create-job.ts b/backend/src/schedulers/__mocks__/create-job.ts new file mode 100644 index 00000000..92a3cd06 --- /dev/null +++ b/backend/src/schedulers/__mocks__/create-job.ts @@ -0,0 +1,20 @@ +/** This mock returns a fake CronJob class. Running start() will immediate + * execute the callback instead of waiting for specified time */ +const mockCreateJob = jest.fn( + (cronTime, callback, mutex, { title, message }) => { + return { + start: jest.fn(async () => { + console.log(`Mock run`); + try { + await callback(); // Simulate the callback execution + } catch (e) { + console.error(`Mock error`); + } finally { + console.log(`Mock end run`); + } + }), + }; + }, +); + +export { mockCreateJob as createJob }; diff --git a/backend/src/schedulers/delete-announcements-scheduler.spec.ts b/backend/src/schedulers/delete-announcements-scheduler.spec.ts new file mode 100644 index 00000000..1b8d7fcb --- /dev/null +++ b/backend/src/schedulers/delete-announcements-scheduler.spec.ts @@ -0,0 +1,29 @@ +import waitFor from 'wait-for-expect'; +import deleteAnnouncementsJob from './delete-announcements-scheduler'; +import { announcementService } from '../v1/services/announcements-service'; + +jest.mock('../config', () => ({ + config: { + get: (key: string) => { + const settings = { + 'server:deleteAnnouncementsCronTime': '121212121', + }; + + return settings[key]; + }, + }, +})); + +jest.mock('./create-job'); +jest.mock('../v1/services/announcements-service'); + +describe('delete-announcements-scheduler', () => { + it('should run the function', async () => { + deleteAnnouncementsJob.start(); + await waitFor(async () => { + expect( + announcementService.deleteAnnouncementsSchedule, + ).toHaveBeenCalled(); + }); + }); +}); diff --git a/backend/src/schedulers/delete-announcements-scheduler.ts b/backend/src/schedulers/delete-announcements-scheduler.ts index c41c3fd8..3080f7ea 100644 --- a/backend/src/schedulers/delete-announcements-scheduler.ts +++ b/backend/src/schedulers/delete-announcements-scheduler.ts @@ -1,8 +1,8 @@ import { config } from '../config'; -import { schedulerService } from '../v1/services/scheduler-service'; import { logger as log } from '../logger'; import advisoryLock from 'advisory-lock'; import { createJob } from './create-job'; +import { announcementService } from '../v1/services/announcements-service'; const SCHEDULER_NAME = 'DeleteAnnouncements'; const mutex = advisoryLock(config.get('server:databaseUrl'))( @@ -14,7 +14,7 @@ export default createJob( crontime, async () => { log.info(`Starting scheduled job '${SCHEDULER_NAME}'.`); - await schedulerService.deleteAnnouncementSchedule(); + await announcementService.deleteAnnouncementsSchedule(); log.info(`Completed scheduled job '${SCHEDULER_NAME}'.`); }, mutex, diff --git a/backend/src/schedulers/run.all.spec.ts b/backend/src/schedulers/run.all.spec.ts index e1165927..8d1efd6d 100644 --- a/backend/src/schedulers/run.all.spec.ts +++ b/backend/src/schedulers/run.all.spec.ts @@ -45,6 +45,15 @@ jest.mock('./email-expiring-announcements-scheduler', () => ({ }, })); +const mockDeleteAnnouncementsScheduler = jest.fn(); +jest.mock('./delete-announcements-scheduler', () => ({ + __esModule: true, + + default: { + start: () => mockDeleteAnnouncementsScheduler(), + }, +})); + describe('run.all', () => { it('should start all jobs', async () => { run(); @@ -53,5 +62,6 @@ describe('run.all', () => { expect(mockStartReportLock).toHaveBeenCalled(); expect(mockExpireAnnouncementsLock).toHaveBeenCalled(); expect(mockEmailExpiringAnnouncementsScheduler).toHaveBeenCalled(); + expect(mockDeleteAnnouncementsScheduler).toHaveBeenCalled(); }); }); diff --git a/backend/src/schedulers/run.all.ts b/backend/src/schedulers/run.all.ts index 753d89cc..939d5cb2 100644 --- a/backend/src/schedulers/run.all.ts +++ b/backend/src/schedulers/run.all.ts @@ -4,6 +4,7 @@ import deleteUserErrorsJob from './delete-user-errors-scheduler'; import lockReportsJob from './lock-reports-scheduler'; import expireAnnouncementsJob from './expire-announcements-scheduler'; import emailExpiringAnnouncementsJob from './email-expiring-announcements-scheduler'; +import deleteAnnouncementsJob from './delete-announcements-scheduler'; export const run = () => { try { @@ -12,6 +13,7 @@ export const run = () => { lockReportsJob?.start(); expireAnnouncementsJob?.start(); emailExpiringAnnouncementsJob?.start(); + deleteAnnouncementsJob?.start(); } catch (error) { /* istanbul ignore next */ logger.error(error); diff --git a/backend/src/v1/services/announcements-service.spec.ts b/backend/src/v1/services/announcements-service.spec.ts index a3ee68e9..20ed76bd 100644 --- a/backend/src/v1/services/announcements-service.spec.ts +++ b/backend/src/v1/services/announcements-service.spec.ts @@ -977,9 +977,9 @@ describe('AnnouncementsService', () => { mockS3ApiDeleteFiles.mockResolvedValue(new Set(['file1', 'file2'])); // files deleted - mockDeleteManyHistory.mockResolvedValue({}); - mockDeleteManyResource.mockResolvedValue({}); mockDeleteManyResourceHistory.mockResolvedValue({}); + mockDeleteManyResource.mockResolvedValue({}); + mockDeleteManyHistory.mockResolvedValue({}); mockDeleteMany.mockResolvedValue({}); await announcementService.deleteAnnouncementsSchedule(); @@ -987,13 +987,13 @@ describe('AnnouncementsService', () => { expect(mockS3ApiDeleteFiles).toHaveBeenCalledWith(['file1', 'file2']); // Two files, so each of these are called twice - expect(mockDeleteManyHistory).toHaveBeenCalledTimes(2); - expect(mockDeleteManyResource).toHaveBeenCalledTimes(2); expect(mockDeleteManyResourceHistory).toHaveBeenCalledTimes(2); + expect(mockDeleteManyResource).toHaveBeenCalledTimes(2); + expect(mockDeleteManyHistory).toHaveBeenCalledTimes(2); expect(mockDeleteMany).toHaveBeenCalledTimes(2); }); - it('should log an error if announcement deletion fails', async () => { + it("shouldn't delete from the database when s3 fails to delete files", async () => { mockFindMany.mockResolvedValueOnce([ { announcement_id: 1, title: 'Announcement 1' }, { announcement_id: 2, title: 'Announcement 2' }, @@ -1008,13 +1008,54 @@ describe('AnnouncementsService', () => { await announcementService.deleteAnnouncementsSchedule(); // Even though there are two files, only one of them was deleted - expect(mockDeleteManyHistory).toHaveBeenCalledTimes(1); - expect(mockDeleteManyResource).toHaveBeenCalledTimes(1); expect(mockDeleteManyResourceHistory).toHaveBeenCalledTimes(1); + expect(mockDeleteManyResource).toHaveBeenCalledTimes(1); + expect(mockDeleteManyHistory).toHaveBeenCalledTimes(1); expect(mockDeleteMany).toHaveBeenCalledTimes(1); expect(mockDeleteMany).toHaveBeenCalledWith({ where: { announcement_id: 2 }, }); }); + + it("shouldn't do anything if there's nothing to delete", async () => { + //test that no announcements were found + mockFindMany.mockResolvedValueOnce([]); + await announcementService.deleteAnnouncementsSchedule(); + expect(mockFindManyResource).toHaveBeenCalledTimes(0); //should return before this function is called + }); + + it("shouldn't do anything if nothing is safe to delete", async () => { + //test that if the resources that were found couldn't be deleted from s3, then nothing happens + mockFindMany.mockResolvedValueOnce([ + { announcement_id: 1, title: 'Announcement 1' }, + { announcement_id: 2, title: 'Announcement 2' }, + ]); + mockFindManyResource.mockResolvedValueOnce([ + { announcement_id: 1, attachment_file_id: 'file1' }, + { announcement_id: 2, attachment_file_id: 'file2' }, + ]); + mockS3ApiDeleteFiles.mockResolvedValue(new Set([])); // didn't delete anything + + await announcementService.deleteAnnouncementsSchedule(); + expect(mockDeleteManyHistory).toHaveBeenCalledTimes(0); //should return before this function is called + }); + + it('should log if database failed to delete', async () => { + //test that if the resources that were found couldn't be deleted from s3, then nothing happens + mockFindMany.mockResolvedValueOnce([ + { announcement_id: 1, title: 'Announcement 1' }, + { announcement_id: 2, title: 'Announcement 2' }, + ]); + mockFindManyResource.mockResolvedValueOnce([ + { announcement_id: 1, attachment_file_id: 'file1' }, + { announcement_id: 2, attachment_file_id: 'file2' }, + ]); + mockS3ApiDeleteFiles.mockResolvedValue(new Set(['file1'])); // didn't delete anything + mockDeleteManyResourceHistory.mockRejectedValue(new Error('err')); + + await announcementService.deleteAnnouncementsSchedule(); + expect(mockDeleteManyResourceHistory).toHaveBeenCalledTimes(1); + expect(mockDeleteManyHistory).toHaveBeenCalledTimes(0); //should error before this function is called + }); }); });