From fe83f497c1bd497853632abac824d0bcc75f4c22 Mon Sep 17 00:00:00 2001 From: Paul Hovley Date: Fri, 23 Oct 2026 11:52:28 -0600 Subject: [PATCH] Fix test suite: lazy Slack encryption, stale tests, mock.module leaks The Run Tests workflow has failed on every PR since September 2025. - SlackOAuthService constructed EbbEncryption at import time, so every test file crashed on load when SLACK_ENCRYPTION_KEY was unset. Construct it lazily on first encrypt/decrypt instead. - Update tests that drifted from the code: LicenseService now checks getLicenseByUserId, LicenseRepo.updateLicenseByStripePaymentId was renamed to updateLicenseByCustomerId, checkout updates now set expiration_date to null, and the Discord weekly-report embed title changed. - Bun's mock.module applies to every test file in the run. The LicenseRepo stubs in two files leaked into the checkout e2e tests and the NotificationEngine stub leaked into WebhookService tests. Use spies for LicenseRepo and restore the real modules in afterAll for the notification tests. - Allow TEST_DB_PORT to override the test database port for local runs where 5433 is already taken. Co-Authored-By: Claude Fable 5.1 --- config/database.ts | 2 +- repos/__tests__/License.test.ts | 4 +- services/SlackOAuthService.ts | 14 +++++-- services/__tests__/LicenseService.test.ts | 39 +++++++++---------- tests/services/WebhookService.test.ts | 36 ++++++++--------- tests/unit/discordProvider.test.ts | 2 +- tests/unit/notificationEngine.test.ts | 16 +++++++- ...rocessNotificationsWithIdempotency.test.ts | 19 ++++++++- 8 files changed, 84 insertions(+), 48 deletions(-) diff --git a/config/database.ts b/config/database.ts index 71e60b0..93511d5 100644 --- a/config/database.ts +++ b/config/database.ts @@ -23,7 +23,7 @@ const testConfig = { client: 'pg', connection: { host: 'localhost', - port: isCI ? 5432 : 5433, + port: parseInt(process.env.TEST_DB_PORT || (isCI ? '5432' : '5433')), user: 'test_user', password: 'test_pass', database: 'ebb_test' diff --git a/repos/__tests__/License.test.ts b/repos/__tests__/License.test.ts index 4cc41df..eacfd25 100644 --- a/repos/__tests__/License.test.ts +++ b/repos/__tests__/License.test.ts @@ -104,9 +104,9 @@ describe('LicenseRepo', () => { }) - describe('updateLicenseByStripePaymentId', () => { + describe('updateLicenseByCustomerId', () => { it('should update a license', async () => { - const result = await LicenseRepo.updateLicenseByStripePaymentId('pi_test123', 'expired') + const result = await LicenseRepo.updateLicenseByCustomerId('cus_test123', 'expired') expect(result).not.toBeNull() expect(result?.user_id).toBe(user1Id) expect(result?.status).toBe('expired') diff --git a/services/SlackOAuthService.ts b/services/SlackOAuthService.ts index c3a659d..98502e0 100644 --- a/services/SlackOAuthService.ts +++ b/services/SlackOAuthService.ts @@ -54,7 +54,15 @@ const BOT_SCOPES = [ 'users:read' ].join(',') -const ENCRYPTION = new EbbEncryption(process.env.SLACK_ENCRYPTION_KEY) +// Constructed lazily so importing this module (e.g. in tests or in environments +// without Slack configured) does not throw when SLACK_ENCRYPTION_KEY is unset. +let encryption: EbbEncryption | undefined +const getEncryption = (): EbbEncryption => { + if (!encryption) { + encryption = new EbbEncryption(process.env.SLACK_ENCRYPTION_KEY) + } + return encryption +} const generateAuthUrl = async (userId: string, teamId?: string, redirectType?: 'dev' | 'prod'): Promise => { if (!CLIENT_ID) { @@ -199,11 +207,11 @@ const updateUserPreferences = async (userId: string, updates: SlackPreferences): const encrypt = (text: string): string => { - return ENCRYPTION.encrypt(text) + return getEncryption().encrypt(text) } const decrypt = (encryptedText: string): string => { - return ENCRYPTION.decrypt(encryptedText) + return getEncryption().decrypt(encryptedText) } const getDecryptedToken = (encryptedToken: string): string => { diff --git a/services/__tests__/LicenseService.test.ts b/services/__tests__/LicenseService.test.ts index f596143..ccbe142 100644 --- a/services/__tests__/LicenseService.test.ts +++ b/services/__tests__/LicenseService.test.ts @@ -1,49 +1,46 @@ -import { describe, it, expect, beforeEach, mock } from 'bun:test' +import { describe, it, expect, afterEach, mock, spyOn } from 'bun:test' import { LicenseService } from "../LicenseService" import { LicenseRepo, type License } from "../../repos/License" -// Mock the LicenseRepo module -mock.module("../../repos/License", () => ({ - LicenseRepo: { - getActiveLicenseByUserId: mock(), - createLicense: mock() - } -})) +// Use spies rather than mock.module: Bun applies mock.module to every test file +// in the run, so a module-level stub here would leak into later files. const mockLicense = { - id: 'license-123', user_id: 'test-user-id', license_type: 'free_trial', status: 'active', expiration_date: new Date(Date.now() + 14 * 24 * 60 * 60 * 1000) } as License + describe('LicenseService', () => { - beforeEach(() => { - // Reset all mocks before each test + afterEach(() => { + // Restore all spies so LicenseRepo is untouched for other test files mock.restore() }) describe('startFreeTrial', () => { - it('should throw error when user already has an active license', async () => { - // Mock getActiveLicenseByUserId to return an existing license - LicenseRepo.getActiveLicenseByUserId = mock(() => Promise.resolve(mockLicense)) + it('should throw error when user already has a license', async () => { + const getLicenseByUserId = spyOn(LicenseRepo, 'getLicenseByUserId').mockResolvedValue(mockLicense) await expect(LicenseService.startFreeTrial('test-user-id')).rejects.toThrow('User already has a license') - - expect(LicenseRepo.getActiveLicenseByUserId).toHaveBeenCalledWith('test-user-id') + + expect(getLicenseByUserId).toHaveBeenCalledWith('test-user-id') }) + it('should start a free trial when user has no existing license', async () => { - LicenseRepo.getActiveLicenseByUserId = mock(() => Promise.resolve(null)) - LicenseRepo.createLicense = mock(() => Promise.resolve(mockLicense)) + const getLicenseByUserId = spyOn(LicenseRepo, 'getLicenseByUserId').mockResolvedValue(null) + const createLicense = spyOn(LicenseRepo, 'createLicense').mockResolvedValue(mockLicense) + const result = await LicenseService.startFreeTrial('test-user-id') + expect(result).toBeDefined() - expect(LicenseRepo.getActiveLicenseByUserId).toHaveBeenCalledWith('test-user-id') - expect(LicenseRepo.createLicense).toHaveBeenCalledWith({ + expect(getLicenseByUserId).toHaveBeenCalledWith('test-user-id') + expect(createLicense).toHaveBeenCalledWith({ user_id: 'test-user-id', license_type: 'free_trial', expiration_date: expect.any(Date) }) }) }) -}) \ No newline at end of file +}) diff --git a/tests/services/WebhookService.test.ts b/tests/services/WebhookService.test.ts index 083f06b..08e2d8c 100644 --- a/tests/services/WebhookService.test.ts +++ b/tests/services/WebhookService.test.ts @@ -1,17 +1,10 @@ -import { beforeEach, describe, expect, test, mock } from 'bun:test' +import { afterEach, beforeEach, describe, expect, test, mock, spyOn } from 'bun:test' import { WebhookService } from '../../services/WebhookService.js' import { LicenseRepo } from '../../repos/License.js' import type Stripe from 'stripe' -mock.module('../../repos/License.js', () => ({ - LicenseRepo: { - createLicense: mock(() => Promise.resolve({ id: 'license-123' })), - updateLicenseByStripePaymentId: mock(() => Promise.resolve({ id: 'license-123' })), - getFreeTrialLicenseByUserId: mock(() => Promise.resolve(null)), - getExistingSubscriptionLicenseByUserId: mock(() => Promise.resolve(null)), - updateLicense: mock(() => Promise.resolve({ id: 'trial-license-456' })) - } -})) +// Use spies rather than mock.module: Bun applies mock.module to every test file +// in the run, so a module-level stub here would leak into later files. let defaultMockSession = { id: 'cs_test_b17yWHosFRVEk62aEegAVsREUlB415PcJqoS1B2ZNEGnVMgVm0Bfpnyz1C', @@ -33,15 +26,18 @@ let defaultMockSession = { describe('WebhookService', () => { beforeEach(() => { - // Clear call history for all mocks - (LicenseRepo.createLicense as any).mockClear(); - (LicenseRepo.updateLicense as any).mockClear(); - (LicenseRepo.updateLicenseByStripePaymentId as any).mockClear(); - (LicenseRepo.getFreeTrialLicenseByUserId as any).mockClear(); - (LicenseRepo.getExistingSubscriptionLicenseByUserId as any).mockClear(); + spyOn(LicenseRepo, 'createLicense').mockResolvedValue({ id: 'license-123' } as any) + spyOn(LicenseRepo, 'updateLicense').mockResolvedValue({ id: 'trial-license-456' } as any) + spyOn(LicenseRepo, 'getFreeTrialLicenseByUserId').mockResolvedValue(null) + spyOn(LicenseRepo, 'getExistingSubscriptionLicenseByUserId').mockResolvedValue(null) process.env.STRIPE_SANDBOX = 'true' }) + afterEach(() => { + // Restore all spies so LicenseRepo is untouched for other test files + mock.restore() + }) + describe('handleCheckoutSessionCompleted', () => { test('should create license for valid checkout session with metadata', async () => { @@ -53,7 +49,8 @@ describe('WebhookService', () => { license_type: 'subscription', purchase_date: expect.any(Date), stripe_customer_id: 'cus_Sxs0V3mlJjxzg7', - stripe_payment_id: 'sub_1S1wGyAuvxsld26UyyEWlujr' + stripe_payment_id: 'sub_1S1wGyAuvxsld26UyyEWlujr', + expiration_date: undefined }) }) @@ -109,6 +106,7 @@ describe('WebhookService', () => { status: 'active', license_type: 'subscription', stripe_payment_id: 'sub_1S1wGyAuvxsld26UyyEWlujr', + expiration_date: null, updated_at: expect.any(Date) }) @@ -131,6 +129,7 @@ describe('WebhookService', () => { expect(LicenseRepo.updateLicense).toHaveBeenCalledWith('expired-license-456', { status: 'active', stripe_payment_id: 'sub_1S1wGyAuvxsld26UyyEWlujr', + expiration_date: null, updated_at: expect.any(Date) }) @@ -180,7 +179,8 @@ describe('WebhookService', () => { license_type: 'subscription', purchase_date: expect.any(Date), stripe_customer_id: 'cus_DifferentCustomer', - stripe_payment_id: 'sub_DifferentSubscription' + stripe_payment_id: 'sub_DifferentSubscription', + expiration_date: undefined }) }) }) diff --git a/tests/unit/discordProvider.test.ts b/tests/unit/discordProvider.test.ts index a2dd1c4..5057cc6 100644 --- a/tests/unit/discordProvider.test.ts +++ b/tests/unit/discordProvider.test.ts @@ -229,7 +229,7 @@ describe('DiscordNotificationProvider', () => { expect(callArgs).toBeDefined() const body = JSON.parse(callArgs![1].body) - expect(body.embeds[0].title).toBe('📊 Weekly Report') + expect(body.embeds[0].title).toBe('📊 Weekly Report Reminder') expect(body.embeds[0].color).toBe(0x9932CC) // Purple }) }) diff --git a/tests/unit/notificationEngine.test.ts b/tests/unit/notificationEngine.test.ts index ec383ae..bb5579d 100644 --- a/tests/unit/notificationEngine.test.ts +++ b/tests/unit/notificationEngine.test.ts @@ -1,5 +1,19 @@ -import { describe, it, expect, beforeEach, mock, spyOn } from 'bun:test' +import { describe, it, expect, beforeEach, afterAll, mock, spyOn } from 'bun:test' import { NotificationEngine } from '../../services/NotificationEngine' +import * as RealDiscordNotificationProvider from '../../services/providers/DiscordNotificationProvider' +import * as RealNotificationService from '../../services/NotificationService' + +// Bun applies mock.module to every test file in the run, not just this one. +// Snapshot the real exports (static imports are evaluated before the mocks +// below) and put them back when this file finishes so later files get the +// real modules. +const realDiscordNotificationProvider = { ...RealDiscordNotificationProvider } +const realNotificationService = { ...RealNotificationService } + +afterAll(() => { + mock.module('../../services/providers/DiscordNotificationProvider', () => realDiscordNotificationProvider) + mock.module('../../services/NotificationService', () => realNotificationService) +}) import type { NotificationConfig, NotificationProvider, diff --git a/tests/unit/processNotificationsWithIdempotency.test.ts b/tests/unit/processNotificationsWithIdempotency.test.ts index 99ec89f..cc7e03a 100644 --- a/tests/unit/processNotificationsWithIdempotency.test.ts +++ b/tests/unit/processNotificationsWithIdempotency.test.ts @@ -1,6 +1,23 @@ -import { describe, it, expect, beforeEach, mock, spyOn } from 'bun:test' +import { describe, it, expect, beforeEach, afterAll, mock, spyOn } from 'bun:test' import { processNotificationsWithIdempotency } from '../../services/JobProcessors' import type { NewUserRecord, InactiveUserRecord } from '../../types/notifications' +import * as RealNotificationEngine from '../../services/NotificationEngine' +import * as RealUserNotificationsRepo from '../../repos/UserNotifications' +import * as RealNotificationsConfig from '../../config/notifications' + +// Bun applies mock.module to every test file in the run, not just this one. +// Snapshot the real exports (static imports are evaluated before the mocks +// below) and put them back when this file finishes so later files get the +// real modules. +const realNotificationEngine = { ...RealNotificationEngine } +const realUserNotificationsRepo = { ...RealUserNotificationsRepo } +const realNotificationsConfig = { ...RealNotificationsConfig } + +afterAll(() => { + mock.module('../../services/NotificationEngine', () => realNotificationEngine) + mock.module('../../repos/UserNotifications', () => realUserNotificationsRepo) + mock.module('../../config/notifications', () => realNotificationsConfig) +}) // Mock dependencies directly const mockNotificationEngine = {