Skip to content

Conversation

@tomast1337
Copy link
Member

Problem

After changing the database package from CommonJS to ESM, multiple tests were failing with module resolution errors and outdated test expectations that no longer matched the current implementation.

Root Causes

  1. Module Resolution Issue: The database package was compiled to CommonJS (require) but had "type": "module" in package.json, causing a mismatch when importing the ESM-only @nbw/config package
  2. Mongoose Model Compilation Errors: Tests were trying to create real Mongoose models in beforeEach, causing "Cannot overwrite model once compiled" errors
  3. Missing Dependency Injection Decorators: Controllers missing explicit @Inject decorators were failing dependency injection in Bun's test environment
  4. Outdated Test Expectations: Tests were checking for deprecated methods and incorrect behavior that no longer match the implementation

Changes Made

1. Fixed Module Resolution (packages/database/tsconfig.build.json)

  • Changed module from "CommonJS" to "ES2022" to match package.json's "type": "module"
  • This ensures compiled output uses ESM (import/export) compatible with @nbw/config

2. Fixed Mongoose Test Setup (apps/backend/src/song/song.service.spec.ts)

  • Replaced real Mongoose model creation with a mock model object
  • Added missing methods to mock: aggregate, populate
  • Added mongoose import for mongoose.Types.ObjectId() usage
  • Added getSongFile method to mockFileService

3. Fixed Dependency Injection (apps/backend/src/song/song.controller.ts, apps/backend/src/song/my-songs/my-songs.controller.ts)

  • Added explicit @Inject(SongService) and @Inject(FileService) decorators
  • Required for proper dependency injection in Bun's test environment with NestJS

4. Updated Test Expectations

SongController Tests (apps/backend/src/song/song.controller.spec.ts)

  • getSongFile: Updated to use getSongFileBuffer instead of getSongDownloadUrl, expects buffer/file response instead of redirect
  • searchSongs: Fixed to expect 2 parameters instead of 3 (removed undefined category parameter)
  • getSongList: Updated to use querySongs instead of deprecated getSongByPage
  • Error handling: Updated to expect controller's generic error message

SongService Tests (apps/backend/src/song/song.service.spec.ts)

  • getCategories: Fixed to use mock model's aggregate method directly
  • getRandomSongs: Fixed to use mock model methods instead of jest.spyOn, added populate mock

AuthController Tests (apps/backend/src/auth/auth.controller.spec.ts)

  • githubLogin/googleLogin/discordLogin: Fixed expectations - these are Passport guard entry points that don't call authService
  • Added jest.clearAllMocks() in beforeEach for test isolation
  • Updated tests to verify methods don't call authService (since actual login is handled by callback endpoints)

Testing

  • All 247 tests now pass (previously 13+ failing)
  • No module resolution errors
  • All test mocks properly isolated
  • Tests match current implementation behavior

Files Changed

  • packages/database/tsconfig.build.json - Changed module format to ESM
  • apps/backend/src/song/song.controller.ts - Added @Inject decorators
  • apps/backend/src/song/my-songs/my-songs.controller.ts - Added @Inject decorator
  • apps/backend/src/song/song.service.spec.ts - Fixed mock model setup and test expectations
  • apps/backend/src/song/song.controller.spec.ts - Updated test expectations to match implementation
  • apps/backend/src/auth/auth.controller.spec.ts - Fixed test expectations and added test isolation

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes test failures and module resolution issues that occurred after converting the database package from CommonJS to ESM. The changes address multiple root causes including module compilation mismatches, Mongoose model compilation errors, missing dependency injection decorators, and outdated test expectations.

Changes:

  • Fixed module resolution by changing TypeScript compilation target from CommonJS to ES2022 to match package.json's ESM configuration
  • Replaced real Mongoose models with mock objects in tests to prevent compilation errors
  • Added explicit @Inject decorators to controllers for proper dependency injection in test environments
  • Updated test expectations to match current implementation (method names, parameters, return types, and error handling)

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
packages/database/tsconfig.build.json Changed module compilation from CommonJS to ES2022 to align with package.json's "type": "module" setting
apps/backend/src/song/song.service.spec.ts Replaced real Mongoose models with comprehensive mock objects and updated test expectations to use mock methods directly
apps/backend/src/song/song.controller.ts Added explicit @Inject decorators for SongService and FileService dependencies
apps/backend/src/song/song.controller.spec.ts Updated test expectations to use current methods (querySongs instead of getSongByPage, getSongFileBuffer instead of getSongDownloadUrl) and corrected parameter counts
apps/backend/src/song/my-songs/my-songs.controller.ts Added explicit @Inject decorator for SongService dependency
apps/backend/src/auth/auth.controller.spec.ts Fixed test expectations to correctly verify that Passport guard entry points don't call authService, and added test isolation with jest.clearAllMocks()

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@Bentroen Bentroen merged commit 64cdca4 into develop Jan 11, 2026
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants