feat: implement honest test discovery with Jest projects and separate…#362
Merged
OlufunbiIK merged 1 commit intoOlufunbiIK:mainfrom Mar 29, 2026
Merged
Conversation
… test layers - Create separate Jest configurations for unit, integration, and E2E tests - Remove silent ignore-pattern coverage gaps from package.json - Make previously skipped feature suites (analytics, auth, comments, etc.) runnable - Add comprehensive npm scripts for different test layers - Create contract tests to separate mock-based tests from real wiring - Add detailed testing documentation for contributors - Implement proper test setup files with appropriate mocking - Provide CI-ready configuration and clear test structure Fixes OlufunbiIK#348
|
@patrickNwafo is attempting to deploy a commit to the olufunbiik's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThis PR establishes a comprehensive three-layer testing infrastructure (unit, integration, E2E) with dedicated Jest configurations, setup files, and documentation. It updates package.json to route test runs through explicit config files and adds an API contract test suite validating request/response shapes across multiple endpoints. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (10)
PR_DESCRIPTION.md (1)
29-33: Add language specifier to the API endpoints code block.The fenced code block showing API endpoints should have a language specified. Use
textorhttpfor endpoint documentation.📝 Proposed fix
### Enhanced API Endpoints -``` +```text GET /collaborations/invitations/pending # Get user's pending invitations DELETE /collaborations/:id # Remove collaborator (track owner only) GET /collaborations/tracks/:id/stats # Collaboration statistics</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@PR_DESCRIPTION.mdaround lines 29 - 33, The fenced code block in
PR_DESCRIPTION.md showing the API endpoints is missing a language specifier;
update the opening fence fromtotext (or ```http) so the block becomes/collaborations/invitations/pending", "DELETE /collaborations/:id", and "GET /collaborations/tracks/:id/stats" to ensure proper syntax highlighting in documentation.backend/test/jest-e2e-updated.json (1)
6-11: Consider removing unusedunit-spec.tsignore pattern.The
testPathIgnorePatternsincludes*.unit-spec.ts, but according to the documentation inTESTING.md, unit tests use the*.spec.tsnaming convention, not*.unit-spec.ts. SincetestMatchalready restricts to*.e2e-spec.ts, this ignore pattern appears unused.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/test/jest-e2e-updated.json` around lines 6 - 11, Remove the unused ignore pattern from the Jest config: open the JSON where testPathIgnorePatterns is defined and delete the "<rootDir>/../src/**/*.unit-spec.ts$" entry (it is redundant because TESTING.md and the existing testMatch/*.e2e-spec.ts restricts test files to *.spec.ts/* .e2e-spec.ts); ensure the resulting testPathIgnorePatterns still contains the other necessary patterns and that the JSON formatting remains valid.backend/test/setup/e2e.setup.ts (2)
19-24: Consider adding TypeScript type declarations for global helpers.Similar to
unit.setup.ts, the global helpers (createE2ETestModule,httpTestUtils,testFixtures) lack TypeScript type declarations which may cause type errors in strict mode.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/test/setup/e2e.setup.ts` around lines 19 - 24, Add TypeScript declarations for the global helpers (createE2ETestModule, httpTestUtils, testFixtures) by augmenting the global namespace (e.g., declare global { function createE2ETestModule(...): Promise<ReturnType>; const httpTestUtils: HttpTestUtilsType; const testFixtures: TestFixturesType; }) or by adding a .d.ts file like in unit.setup.ts; ensure you reference the actual return/type shapes used in e2e.setup.ts (e.g., Test.createTestingModule/compiled module type for createE2ETestModule and the specific interfaces/types for httpTestUtils and testFixtures) and export/import any custom types so the compiler in strict mode recognizes these globals.
33-33: Prefer static import over dynamicrequire.Using
require('@nestjs/common')at runtime is unusual whenValidationPipecould be statically imported at the top of the file alongsideTest.📝 Proposed fix
import 'jest'; import { Test } from '@nestjs/testing'; +import { ValidationPipe } from '@nestjs/common'; // ... later in code ... app.useGlobalPipes( - new (require('@nestjs/common').ValidationPipe)({ + new ValidationPipe({ whitelist: true,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/test/setup/e2e.setup.ts` at line 33, Replace the dynamic require usage for ValidationPipe with a static import: add ValidationPipe to the top-file imports from '@nestjs/common' alongside Test, then update the instantiation site that currently uses new (require('@nestjs/common').ValidationPipe) to new ValidationPipe(...) so the code uses the statically imported ValidationPipe symbol.backend/test/setup/unit.setup.ts (1)
62-103: Consider adding TypeScript type declarations forglobal.testUtils.The
global.testUtilsobject is assigned without TypeScript type declarations, which may cause type errors in strict mode. Consider adding a type declaration file or augmenting the global namespace.📝 Proposed type declaration
Add to the top of the file or a separate
global.d.ts:declare global { var testUtils: { createMockUser: (overrides?: Record<string, unknown>) => Record<string, unknown>; createMockArtist: (overrides?: Record<string, unknown>) => Record<string, unknown>; createMockTrack: (overrides?: Record<string, unknown>) => Record<string, unknown>; }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/test/setup/unit.setup.ts` around lines 62 - 103, Declare types for global.testUtils by augmenting the global namespace (either in this file or a separate global.d.ts) so TypeScript knows the shape of createMockUser, createMockArtist, and createMockTrack; add a declare global { var testUtils: { createMockUser: (overrides?: Record<string, unknown>) => Record<string, unknown>; createMockArtist: (overrides?: Record<string, unknown>) => Record<string, unknown>; createMockTrack: (overrides?: Record<string, unknown>) => Record<string, unknown>; }; } or use more specific interfaces for the returned objects, then ensure the declaration file is included by tsconfig so references to global.testUtils and its methods (createMockUser/createMockArtist/createMockTrack) are type-checked.backend/TESTING.md (1)
56-77: Add language specifier to the directory structure code block.The fenced code block showing the test structure should have a language specified for consistent rendering. Use
textorplaintextfor directory trees.📝 Proposed fix
-``` +```text backend/ ├── src/ │ ├── *.spec.ts # Unit tests🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/TESTING.md` around lines 56 - 77, The fenced code block showing the directory tree in TESTING.md is missing a language specifier; update the opening fence from ``` to ```text (or ```plaintext) so the directory structure is rendered consistently (locate the block that begins with the backend/ tree and change the fence to include "text").backend/test/api-contract.spec.ts (2)
473-484: Consider that hardcoded user limits role-based contract testing.The overridden
JwtAuthGuardalways injectsusers.listener. If any controllers return different response shapes based on user role (e.g., admin vs regular user), those contracts won't be validated. This is acceptable for basic shape validation but may miss role-specific response variations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/test/api-contract.spec.ts` around lines 473 - 484, The test overrides hardcode JwtAuthGuard to always inject users.listener which prevents validating role-specific response shapes; change the override in api-contract.spec.ts to inject a configurable test user (e.g., read from a local variable/currentTestUser or a helper getTestUser(role)) instead of users.listener, and update tests to set that variable to different roles (admin, listener, etc.) before calling controllers so JwtAuthGuard (and optionally RolesGuard) returns requests with the appropriate user shape for role-specific contract checks; keep ModerateMessagePipe override as-is or make it use the same configurable user helper if needed.
121-122: Repeated inlinerequire('@nestjs/common')throughout mock services.The pattern
throw new (require('@nestjs/common').NotFoundException)(...)appears in multiple mock functions. Import these exceptions at the top of the file for cleaner code.Add imports at top of file
import { BadRequestException, UnauthorizedException, NotFoundException } from '@nestjs/common';Then replace all occurrences like:
-throw new (require('@nestjs/common').NotFoundException)('Track not found'); +throw new NotFoundException('Track not found');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/test/api-contract.spec.ts` around lines 121 - 122, Replace repeated inline require calls for Nest exceptions with top-level imports: add "BadRequestException, UnauthorizedException, NotFoundException" imported from '@nestjs/common' at the top of the file, then replace occurrences of patterns like "throw new (require('@nestjs/common').NotFoundException)('...')" (and similar for BadRequestException/UnauthorizedException) with direct usage "throw new NotFoundException('...')" (or BadRequestException/UnauthorizedException) in the mock service functions referenced in this spec file to clean up and centralize exception imports.backend/test/setup/integration.setup.ts (2)
18-18: Relative entity path may not resolve correctly from test setup location.The entities path
'src/**/*.entity{.ts,.js}'is relative, but Jest's working directory may differ from what TypeORM expects. The production config inapp.module.tsuses__dirname + '/**/*.entity{.ts,.js}'for reliable resolution.Proposed fix to use absolute path
- entities: ['src/**/*.entity{.ts,.js}'], + entities: [__dirname + '/../../src/**/*.entity{.ts,.js}'],Alternatively, configure
rootDirin your Jest config and reference it here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/test/setup/integration.setup.ts` at line 18, The entities glob 'src/**/*.entity{.ts,.js}' in integration.setup.ts may not resolve under Jest; replace the relative value for the TypeORM 'entities' option with an absolute path (e.g. using __dirname + '/**/*.entity{.ts,.js}' or path.resolve(__dirname, '../../src/**/*.entity{.ts,.js}')) so TypeORM can reliably locate entity files during tests; update the code that sets the 'entities' array in the test setup (the 'entities' entry shown in the diff) to construct the absolute path or mirror the production logic from app.module.ts.
94-105:process.exit(1)prevents Jest from reporting test failures properly.Using
process.exit(1)terminates the process immediately, bypassing Jest's error reporting. Consider throwing an error instead to let Jest report the failure properly.Proposed fix
beforeAll(async () => { - // Verify test database is accessible try { const dataSource = await global.testUtils.createTestDataSource(); await dataSource.destroy(); console.log('✅ Test database is accessible'); } catch (error) { console.error('❌ Test database setup failed:', error); - process.exit(1); + throw new Error(`Test database setup failed: ${error.message}`); } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/test/setup/integration.setup.ts` around lines 94 - 105, The beforeAll hook currently calls process.exit(1) on failure which prevents Jest from reporting failures; replace the process.exit call in the beforeAll catch block with throwing an Error (or rethrowing the caught error) so Jest can surface the failure—specifically update the catch in the beforeAll that wraps global.testUtils.createTestDataSource() to throw a new Error including contextual text and the original error (or simply rethrow error) instead of calling process.exit(1).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/test/api-contract.spec.ts`:
- Line 518: The test currently asserts res.body.expiresAt is a Date instance but
JSON responses contain ISO date strings; change the assertion to check that
res.body.expiresAt is a string and is parseable as a valid date (e.g., assert
typeof res.body.expiresAt === 'string' and that Date.parse(res.body.expiresAt)
is not NaN) so the assertion uses res.body.expiresAt and verifies it is a valid
ISO date string rather than an instance of Date.
- Around line 55-58: The mock generateChallenge implementation currently permits
public keys with lengths other than Stellar's required 56 characters and uses an
inline require; update the validation in the generateChallenge jest.fn to
require publicKey && publicKey.startsWith('G') && publicKey.length === 56, and
replace the inline require('@nestjs/common').BadRequestException with an
imported BadRequestException (add import { BadRequestException,
UnauthorizedException, NotFoundException } from '@nestjs/common' at the top of
the file) so the code uses the BadRequestException symbol directly.
- Around line 594-606: The test fails because buildMockTracksService().findOne
returns tracks without a populated artist relation while the spec asserts
res.body.artist.id and res.body.artist.artistName; update the mock returned
value in buildMockTracksService().findOne (and/or the sample track data
referenced as tracks.sample) to include an artist object with the expected shape
(id and artistName) so the GET /tracks/:id contract test receives a track with a
populated artist relation.
In `@backend/test/setup/e2e.setup.ts`:
- Around line 31-38: E2E setup registers ValidationPipe but omits the production
SanitiseInputPipe; update the app.useGlobalPipes call to include the
SanitiseInputPipe (the same class used in production) alongside ValidationPipe
so E2E behavior matches production: import or require SanitiseInputPipe and pass
new SanitiseInputPipe() as the first argument to app.useGlobalPipes followed by
the existing new ValidationPipe({...}), ensuring you reference the
SanitiseInputPipe symbol and leave ValidationPipe configuration unchanged.
In `@backend/test/setup/integration.setup.ts`:
- Around line 39-60: The Test.createTestingModule call currently spreads
...overrides (which may include imports and providers) causing duplicate imports
and double-handling of providers; change createTestingModule to exclude
overrides.imports and overrides.providers when spreading (e.g., const { imports,
providers, ...rest } = overrides) and pass rest into Test.createTestingModule
while still explicitly merging the default imports with (imports || []), and
keep the existing overrideProvider loop to apply providers (do not also include
providers in the module metadata).
- Around line 62-71: global.cleanupTestDatabase currently deletes rows by
iterating entityMetadatas which can fail on FK constraints; change it to run
TRUNCATE with CASCADE (or temporarily disable referential integrity) instead of
DELETE for each entity.tableName using the DataSource.query or a QueryRunner so
FK dependencies are handled atomically; use entity.tableName from
dataSource.entityMetadatas and execute e.g. TRUNCATE TABLE "schema"."table"
CASCADE (or appropriate DB-specific disable/enable constraint commands) and
ensure the DataSource/QueryRunner is initialized and released after the
operation in global.cleanupTestDatabase.
---
Nitpick comments:
In `@backend/test/api-contract.spec.ts`:
- Around line 473-484: The test overrides hardcode JwtAuthGuard to always inject
users.listener which prevents validating role-specific response shapes; change
the override in api-contract.spec.ts to inject a configurable test user (e.g.,
read from a local variable/currentTestUser or a helper getTestUser(role))
instead of users.listener, and update tests to set that variable to different
roles (admin, listener, etc.) before calling controllers so JwtAuthGuard (and
optionally RolesGuard) returns requests with the appropriate user shape for
role-specific contract checks; keep ModerateMessagePipe override as-is or make
it use the same configurable user helper if needed.
- Around line 121-122: Replace repeated inline require calls for Nest exceptions
with top-level imports: add "BadRequestException, UnauthorizedException,
NotFoundException" imported from '@nestjs/common' at the top of the file, then
replace occurrences of patterns like "throw new
(require('@nestjs/common').NotFoundException)('...')" (and similar for
BadRequestException/UnauthorizedException) with direct usage "throw new
NotFoundException('...')" (or BadRequestException/UnauthorizedException) in the
mock service functions referenced in this spec file to clean up and centralize
exception imports.
In `@backend/test/jest-e2e-updated.json`:
- Around line 6-11: Remove the unused ignore pattern from the Jest config: open
the JSON where testPathIgnorePatterns is defined and delete the
"<rootDir>/../src/**/*.unit-spec.ts$" entry (it is redundant because TESTING.md
and the existing testMatch/*.e2e-spec.ts restricts test files to *.spec.ts/*
.e2e-spec.ts); ensure the resulting testPathIgnorePatterns still contains the
other necessary patterns and that the JSON formatting remains valid.
In `@backend/test/setup/e2e.setup.ts`:
- Around line 19-24: Add TypeScript declarations for the global helpers
(createE2ETestModule, httpTestUtils, testFixtures) by augmenting the global
namespace (e.g., declare global { function createE2ETestModule(...):
Promise<ReturnType>; const httpTestUtils: HttpTestUtilsType; const testFixtures:
TestFixturesType; }) or by adding a .d.ts file like in unit.setup.ts; ensure you
reference the actual return/type shapes used in e2e.setup.ts (e.g.,
Test.createTestingModule/compiled module type for createE2ETestModule and the
specific interfaces/types for httpTestUtils and testFixtures) and export/import
any custom types so the compiler in strict mode recognizes these globals.
- Line 33: Replace the dynamic require usage for ValidationPipe with a static
import: add ValidationPipe to the top-file imports from '@nestjs/common'
alongside Test, then update the instantiation site that currently uses new
(require('@nestjs/common').ValidationPipe) to new ValidationPipe(...) so the
code uses the statically imported ValidationPipe symbol.
In `@backend/test/setup/integration.setup.ts`:
- Line 18: The entities glob 'src/**/*.entity{.ts,.js}' in integration.setup.ts
may not resolve under Jest; replace the relative value for the TypeORM
'entities' option with an absolute path (e.g. using __dirname +
'/**/*.entity{.ts,.js}' or path.resolve(__dirname,
'../../src/**/*.entity{.ts,.js}')) so TypeORM can reliably locate entity files
during tests; update the code that sets the 'entities' array in the test setup
(the 'entities' entry shown in the diff) to construct the absolute path or
mirror the production logic from app.module.ts.
- Around line 94-105: The beforeAll hook currently calls process.exit(1) on
failure which prevents Jest from reporting failures; replace the process.exit
call in the beforeAll catch block with throwing an Error (or rethrowing the
caught error) so Jest can surface the failure—specifically update the catch in
the beforeAll that wraps global.testUtils.createTestDataSource() to throw a new
Error including contextual text and the original error (or simply rethrow error)
instead of calling process.exit(1).
In `@backend/test/setup/unit.setup.ts`:
- Around line 62-103: Declare types for global.testUtils by augmenting the
global namespace (either in this file or a separate global.d.ts) so TypeScript
knows the shape of createMockUser, createMockArtist, and createMockTrack; add a
declare global { var testUtils: { createMockUser: (overrides?: Record<string,
unknown>) => Record<string, unknown>; createMockArtist: (overrides?:
Record<string, unknown>) => Record<string, unknown>; createMockTrack:
(overrides?: Record<string, unknown>) => Record<string, unknown>; }; } or use
more specific interfaces for the returned objects, then ensure the declaration
file is included by tsconfig so references to global.testUtils and its methods
(createMockUser/createMockArtist/createMockTrack) are type-checked.
In `@backend/TESTING.md`:
- Around line 56-77: The fenced code block showing the directory tree in
TESTING.md is missing a language specifier; update the opening fence from ``` to
```text (or ```plaintext) so the directory structure is rendered consistently
(locate the block that begins with the backend/ tree and change the fence to
include "text").
In `@PR_DESCRIPTION.md`:
- Around line 29-33: The fenced code block in PR_DESCRIPTION.md showing the API
endpoints is missing a language specifier; update the opening fence from ``` to
```text (or ```http) so the block becomes ```text and preserves the three lines
starting with "GET /collaborations/invitations/pending", "DELETE
/collaborations/:id", and "GET /collaborations/tracks/:id/stats" to ensure
proper syntax highlighting in documentation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6a341464-5ef6-4a44-80eb-e08db08d92f7
📒 Files selected for processing (10)
PR_DESCRIPTION.mdbackend/TESTING.mdbackend/package.jsonbackend/test/api-contract.spec.tsbackend/test/jest-e2e-updated.jsonbackend/test/jest-integration.jsonbackend/test/jest-unit.jsonbackend/test/setup/e2e.setup.tsbackend/test/setup/integration.setup.tsbackend/test/setup/unit.setup.ts
Comment on lines
+55
to
+58
| generateChallenge: jest.fn(async (publicKey: string) => { | ||
| if (!publicKey || !publicKey.startsWith('G') || publicKey.length < 56) { | ||
| throw new (require('@nestjs/common').BadRequestException)('Invalid public key format'); | ||
| } |
There was a problem hiding this comment.
Public key validation is incorrect - allows invalid lengths.
The check publicKey.length < 56 allows keys longer than 56 characters. Stellar public keys are exactly 56 characters. Additionally, the inline require('@nestjs/common') is an anti-pattern.
Proposed fix
Add import at top of file:
import { BadRequestException, UnauthorizedException, NotFoundException } from '@nestjs/common';Then update the validation:
generateChallenge: jest.fn(async (publicKey: string) => {
- if (!publicKey || !publicKey.startsWith('G') || publicKey.length < 56) {
- throw new (require('@nestjs/common').BadRequestException)('Invalid public key format');
+ if (!publicKey || !publicKey.startsWith('G') || publicKey.length !== 56) {
+ throw new BadRequestException('Invalid public key format');
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/test/api-contract.spec.ts` around lines 55 - 58, The mock
generateChallenge implementation currently permits public keys with lengths
other than Stellar's required 56 characters and uses an inline require; update
the validation in the generateChallenge jest.fn to require publicKey &&
publicKey.startsWith('G') && publicKey.length === 56, and replace the inline
require('@nestjs/common').BadRequestException with an imported
BadRequestException (add import { BadRequestException, UnauthorizedException,
NotFoundException } from '@nestjs/common' at the top of the file) so the code
uses the BadRequestException symbol directly.
| expect(res.body).toHaveProperty('expiresAt'); | ||
| expect(typeof res.body.challengeId).toBe('string'); | ||
| expect(typeof res.body.challenge).toBe('string'); | ||
| expect(res.body.expiresAt).toBeInstanceOf(Date); |
There was a problem hiding this comment.
Assertion will fail - JSON responses contain date strings, not Date objects.
res.body.expiresAt will be a string (e.g., "2024-01-15T10:30:00.000Z") after JSON parsing, not a Date instance. This test will fail.
Proposed fix
- expect(res.body.expiresAt).toBeInstanceOf(Date);
+ expect(typeof res.body.expiresAt).toBe('string');
+ expect(new Date(res.body.expiresAt).getTime()).toBeGreaterThan(Date.now());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| expect(res.body.expiresAt).toBeInstanceOf(Date); | |
| expect(typeof res.body.expiresAt).toBe('string'); | |
| expect(new Date(res.body.expiresAt).getTime()).toBeGreaterThan(Date.now()); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/test/api-contract.spec.ts` at line 518, The test currently asserts
res.body.expiresAt is a Date instance but JSON responses contain ISO date
strings; change the assertion to check that res.body.expiresAt is a string and
is parseable as a valid date (e.g., assert typeof res.body.expiresAt ===
'string' and that Date.parse(res.body.expiresAt) is not NaN) so the assertion
uses res.body.expiresAt and verifies it is a valid ISO date string rather than
an instance of Date.
Comment on lines
+594
to
+606
| it('GET /tracks/:id - should return track contract', () => { | ||
| return request(app.getHttpServer()) | ||
| .get(`/tracks/${tracks.sample.id}`) | ||
| .expect(200) | ||
| .expect((res) => { | ||
| expect(res.body).toHaveProperty('id'); | ||
| expect(res.body).toHaveProperty('title'); | ||
| expect(res.body).toHaveProperty('genre'); | ||
| expect(res.body).toHaveProperty('artist'); | ||
| expect(res.body.artist).toHaveProperty('id'); | ||
| expect(res.body.artist).toHaveProperty('artistName'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Test will fail - mock findOne doesn't return artist relation.
The test expects res.body.artist.id and res.body.artist.artistName, but the buildMockTracksService().findOne (lines 118-124) returns tracks without a populated artist relation. The mock stores tracks as flat objects.
Proposed fix - update mock to include artist relation
findOne: jest.fn(async (id: string) => {
const found = trackStore.find((t) => t.id === id);
if (!found) {
throw new (require('@nestjs/common').NotFoundException)('Track not found');
}
- return found;
+ return {
+ ...found,
+ artist: {
+ id: found.artistId || artists.sample.id,
+ artistName: artists.sample.artistName,
+ },
+ };
}),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/test/api-contract.spec.ts` around lines 594 - 606, The test fails
because buildMockTracksService().findOne returns tracks without a populated
artist relation while the spec asserts res.body.artist.id and
res.body.artist.artistName; update the mock returned value in
buildMockTracksService().findOne (and/or the sample track data referenced as
tracks.sample) to include an artist object with the expected shape (id and
artistName) so the GET /tracks/:id contract test receives a track with a
populated artist relation.
Comment on lines
+31
to
+38
| // Set up global pipes | ||
| app.useGlobalPipes( | ||
| new (require('@nestjs/common').ValidationPipe)({ | ||
| whitelist: true, | ||
| forbidNonWhitelisted: true, | ||
| transform: true, | ||
| }), | ||
| ); |
There was a problem hiding this comment.
E2E setup missing SanitiseInputPipe that production uses.
The production main.ts (lines 31-37) registers both SanitiseInputPipe and ValidationPipe, but this E2E setup only configures ValidationPipe. This could cause behavioral differences where sanitization happens in production but not in E2E tests, potentially missing XSS or injection vulnerabilities that would be caught in production.
🔧 Proposed fix to match production pipe configuration
+import { ValidationPipe } from '@nestjs/common';
+// Import or mock SanitiseInputPipe to match production
+// import { SanitiseInputPipe } from '../../src/common/pipes/sanitise-input.pipe';
global.httpTestUtils = {
createTestApp: async (module: any) => {
const app = module.createNestApplication();
// Set up global pipes
app.useGlobalPipes(
- new (require('@nestjs/common').ValidationPipe)({
+ // Add SanitiseInputPipe to match production (main.ts:32-37)
+ // new SanitiseInputPipe(),
+ new ValidationPipe({
whitelist: true,
forbidNonWhitelisted: true,
transform: true,
}),
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/test/setup/e2e.setup.ts` around lines 31 - 38, E2E setup registers
ValidationPipe but omits the production SanitiseInputPipe; update the
app.useGlobalPipes call to include the SanitiseInputPipe (the same class used in
production) alongside ValidationPipe so E2E behavior matches production: import
or require SanitiseInputPipe and pass new SanitiseInputPipe() as the first
argument to app.useGlobalPipes followed by the existing new
ValidationPipe({...}), ensuring you reference the SanitiseInputPipe symbol and
leave ValidationPipe configuration unchanged.
Comment on lines
+39
to
+60
| global.createTestingModule = async (overrides: any = {}) => { | ||
| const moduleBuilder = Test.createTestingModule({ | ||
| imports: [ | ||
| ConfigModule.forRoot({ | ||
| isGlobal: true, | ||
| envFilePath: '.env.test', | ||
| }), | ||
| TypeOrmModule.forRoot(testDbConfig), | ||
| ...(overrides.imports || []), | ||
| ], | ||
| ...overrides, | ||
| }); | ||
|
|
||
| // Override any providers if specified | ||
| if (overrides.providers) { | ||
| for (const provider of overrides.providers) { | ||
| moduleBuilder.overrideProvider(provider.provide).useValue(provider.useValue); | ||
| } | ||
| } | ||
|
|
||
| return moduleBuilder.compile(); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Confusing override logic may cause duplicate module metadata.
The current implementation spreads ...overrides which includes imports (already handled at line 47) and providers. This causes:
importsto be added twice if present in overridesprovidersto be added to module metadata AND processed viaoverrideProvider, which are different mechanisms
Proposed fix to separate concerns cleanly
global.createTestingModule = async (overrides: any = {}) => {
+ const { imports = [], providers = [], ...rest } = overrides;
+
const moduleBuilder = Test.createTestingModule({
imports: [
ConfigModule.forRoot({
isGlobal: true,
envFilePath: '.env.test',
}),
TypeOrmModule.forRoot(testDbConfig),
- ...(overrides.imports || []),
+ ...imports,
],
- ...overrides,
+ ...rest,
});
// Override any providers if specified
- if (overrides.providers) {
- for (const provider of overrides.providers) {
+ if (providers.length > 0) {
+ for (const provider of providers) {
moduleBuilder.overrideProvider(provider.provide).useValue(provider.useValue);
}
}
return moduleBuilder.compile();
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/test/setup/integration.setup.ts` around lines 39 - 60, The
Test.createTestingModule call currently spreads ...overrides (which may include
imports and providers) causing duplicate imports and double-handling of
providers; change createTestingModule to exclude overrides.imports and
overrides.providers when spreading (e.g., const { imports, providers, ...rest }
= overrides) and pass rest into Test.createTestingModule while still explicitly
merging the default imports with (imports || []), and keep the existing
overrideProvider loop to apply providers (do not also include providers in the
module metadata).
Comment on lines
+62
to
+71
| global.cleanupTestDatabase = async (dataSource: DataSource) => { | ||
| if (dataSource && dataSource.isInitialized) { | ||
| // Clean all tables | ||
| const entities = dataSource.entityMetadatas; | ||
| for (const entity of entities) { | ||
| const repository = dataSource.getRepository(entity.name); | ||
| await repository.query(`DELETE FROM "${entity.tableName}";`); | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
Table cleanup may fail due to foreign key constraints.
Deleting from tables in arbitrary order (from entityMetadatas) can fail if foreign key constraints exist. Tables must be cleaned in dependency order, or constraints should be temporarily disabled.
Proposed fix using TRUNCATE with CASCADE
global.cleanupTestDatabase = async (dataSource: DataSource) => {
if (dataSource && dataSource.isInitialized) {
- // Clean all tables
const entities = dataSource.entityMetadatas;
for (const entity of entities) {
- const repository = dataSource.getRepository(entity.name);
- await repository.query(`DELETE FROM "${entity.tableName}";`);
+ await dataSource.query(`TRUNCATE TABLE "${entity.tableName}" CASCADE;`);
}
}
};Note: TRUNCATE ... CASCADE handles FK dependencies but requires appropriate privileges.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| global.cleanupTestDatabase = async (dataSource: DataSource) => { | |
| if (dataSource && dataSource.isInitialized) { | |
| // Clean all tables | |
| const entities = dataSource.entityMetadatas; | |
| for (const entity of entities) { | |
| const repository = dataSource.getRepository(entity.name); | |
| await repository.query(`DELETE FROM "${entity.tableName}";`); | |
| } | |
| } | |
| }; | |
| global.cleanupTestDatabase = async (dataSource: DataSource) => { | |
| if (dataSource && dataSource.isInitialized) { | |
| const entities = dataSource.entityMetadatas; | |
| for (const entity of entities) { | |
| await dataSource.query(`TRUNCATE TABLE "${entity.tableName}" CASCADE;`); | |
| } | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/test/setup/integration.setup.ts` around lines 62 - 71,
global.cleanupTestDatabase currently deletes rows by iterating entityMetadatas
which can fail on FK constraints; change it to run TRUNCATE with CASCADE (or
temporarily disable referential integrity) instead of DELETE for each
entity.tableName using the DataSource.query or a QueryRunner so FK dependencies
are handled atomically; use entity.tableName from dataSource.entityMetadatas and
execute e.g. TRUNCATE TABLE "schema"."table" CASCADE (or appropriate DB-specific
disable/enable constraint commands) and ensure the DataSource/QueryRunner is
initialized and released after the operation in global.cleanupTestDatabase.
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
2 participants
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.
#348 Make test discovery honest by splitting Jest projects and running skipped feature suites
🎯 Overview
This PR resolves critical test discovery issues by replacing silent ignore-pattern coverage gaps with explicit Jest projects and making all previously skipped feature suites runnable. The testing system now provides honest, transparent coverage across unit, integration, and E2E layers.
🔧 Key Fixes
1. Eliminated Silent Test Exclusions
testPathIgnorePatterns2. Jest Project Separation
3. Previously Skipped Modules Now Runnable
All 18 excluded modules are now fully testable:
🚀 New Test Commands
Layer-Specific Testing