Conversation
Contributor
There was a problem hiding this comment.
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
@Injectdecorators 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
approved these changes
Jan 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
databasepackage was compiled to CommonJS (require) but had"type": "module"inpackage.json, causing a mismatch when importing the ESM-only@nbw/configpackagebeforeEach, causing "Cannot overwrite model once compiled" errors@Injectdecorators were failing dependency injection in Bun's test environmentChanges Made
1. Fixed Module Resolution (
packages/database/tsconfig.build.json)modulefrom"CommonJS"to"ES2022"to matchpackage.json's"type": "module"import/export) compatible with@nbw/config2. Fixed Mongoose Test Setup (
apps/backend/src/song/song.service.spec.ts)aggregate,populatemongooseimport formongoose.Types.ObjectId()usagegetSongFilemethod tomockFileService3. Fixed Dependency Injection (
apps/backend/src/song/song.controller.ts,apps/backend/src/song/my-songs/my-songs.controller.ts)@Inject(SongService)and@Inject(FileService)decorators4. Updated Test Expectations
SongController Tests (
apps/backend/src/song/song.controller.spec.ts)getSongFileBufferinstead ofgetSongDownloadUrl, expects buffer/file response instead of redirectquerySongsinstead of deprecatedgetSongByPageSongService Tests (
apps/backend/src/song/song.service.spec.ts)aggregatemethod directlyjest.spyOn, addedpopulatemockAuthController Tests (
apps/backend/src/auth/auth.controller.spec.ts)authServicejest.clearAllMocks()inbeforeEachfor test isolationauthService(since actual login is handled by callback endpoints)Testing
Files Changed
packages/database/tsconfig.build.json- Changed module format to ESMapps/backend/src/song/song.controller.ts- Added @Inject decoratorsapps/backend/src/song/my-songs/my-songs.controller.ts- Added @Inject decoratorapps/backend/src/song/song.service.spec.ts- Fixed mock model setup and test expectationsapps/backend/src/song/song.controller.spec.ts- Updated test expectations to match implementationapps/backend/src/auth/auth.controller.spec.ts- Fixed test expectations and added test isolation