LAU-27: add integration tests for auth and workspace resolvers#23
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This pull request adds integration tests for authentication and workspace GraphQL resolvers and REST endpoints. The tests use supertest for HTTP assertions and mock modules to isolate external dependencies like Redis, EventBus, and Analytics.
Changes:
- Added
supertestas a dev dependency for HTTP integration testing - Created mock modules for testing infrastructure (DB, Redis, EventBus, Config, Analytics, Common, Auth)
- Added comprehensive integration tests for auth controller (OTP send/verify, current user)
- Added comprehensive integration tests for workspace resolver (getWorkspace, inviteWorkspaceMember, getInvite, redeemInvite)
- Removed placeholder e2e test and cleaned up global setup/teardown files
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Added supertest dependency for HTTP integration testing |
| pnpm-lock.yaml | Lock file updates for supertest and its transitive dependencies |
| apps/core-e2e/src/support/redis.module.mock.ts | Mock Redis PubSub provider for testing |
| apps/core-e2e/src/support/eventbus.module.mock.ts | Mock EventBus service for testing |
| apps/core-e2e/src/support/db.module.mock.ts | Mock database connection using real Postgres for integration tests |
| apps/core-e2e/src/support/config.module.mock.ts | Mock ConfigService that returns default values |
| apps/core-e2e/src/support/common.module.mock.ts | Mock common module with validation pipe, auth guard, and interceptor |
| apps/core-e2e/src/support/auth.guard.mock.ts | Mock auth guard that extracts user from Bearer token |
| apps/core-e2e/src/support/analytics.module.mock.ts | Mock analytics client for testing |
| apps/core-e2e/src/support/global-setup.ts | Removed server wait logic from global setup |
| apps/core-e2e/src/support/global-teardown.ts | Removed port cleanup from global teardown |
| apps/core-e2e/src/core/auth.controller.spec.ts | Integration tests for auth endpoints (sendOtp, verifyOtp, currentUser) |
| apps/core-e2e/src/core/workspace.resolver.spec.ts | Integration tests for workspace GraphQL operations |
| apps/core-e2e/src/core/core.spec.ts | Removed placeholder test file |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 17 changed files in this pull request and generated 12 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| afterAll(async () => { | ||
| await db.$client.end(); | ||
| await app.close(); |
There was a problem hiding this comment.
The database connection pool is closed in afterAll, but if any async operations are still running when the connection pool is closed, they could fail silently or cause errors. Consider using a more graceful shutdown pattern that ensures all pending operations complete before closing the connection.
| }, | ||
| }, | ||
| }); | ||
|
|
There was a problem hiding this comment.
Similar to the redeemInvite test, this test doesn't verify that the invitation was created successfully before attempting to retrieve it. The token value could be undefined. Consider adding an assertion after line 255 to verify createInviteRes.status and createInviteRes.body.data.inviteWorkspaceMember exist.
| expect(createInviteRes.status).toBe(200); | |
| expect(createInviteRes.body).toBeDefined(); | |
| expect(createInviteRes.body.data).toBeDefined(); | |
| expect(createInviteRes.body.data.inviteWorkspaceMember).toBeDefined(); |
| await db.$client.query('TRUNCATE TABLE "WorkspaceInvite" CASCADE;'); | ||
| await db.$client.query('TRUNCATE TABLE "WorkspaceMembership" CASCADE;'); | ||
| await db.$client.query('TRUNCATE TABLE "Workspace" CASCADE;'); | ||
| await db.$client.query('TRUNCATE TABLE "User" CASCADE;'); |
There was a problem hiding this comment.
Database cleanup is done using raw SQL TRUNCATE queries with CASCADE. While functional, this approach bypasses Drizzle ORM's type safety and could become inconsistent if table names change. Consider using Drizzle's delete API for type-safe cleanup or documenting why raw SQL is necessary here.
| await db.$client.query('TRUNCATE TABLE "Otp" CASCADE;'); | ||
| await db.$client.query('TRUNCATE TABLE "User" CASCADE;'); |
There was a problem hiding this comment.
Database cleanup is done using raw SQL TRUNCATE queries with CASCADE. While functional, this approach bypasses Drizzle ORM's type safety and could become inconsistent if table names change. Consider using Drizzle's delete API for type-safe cleanup or documenting why raw SQL is necessary here.
| await db.$client.query('TRUNCATE TABLE "Otp" CASCADE;'); | |
| await db.$client.query('TRUNCATE TABLE "User" CASCADE;'); | |
| await db.delete(otp); | |
| await db.delete(user); |
| const emailForOtp = 'email-otp@example.com'; | ||
| let app: INestApplication; | ||
| let db: NodePgDatabase & { $client: Pool }; | ||
| let verifiedUser: typeof user.$inferSelect; |
There was a problem hiding this comment.
The variable 'emailForOtp' is declared but never used in the tests. It appears to have been intended for the 'should return 401 when user does not exist' test at line 208, but 'emailForOtp' was defined instead as a test-specific value. Consider removing this unused variable.
| const emailForOtp = 'email-otp@example.com'; | |
| let app: INestApplication; | |
| let db: NodePgDatabase & { $client: Pool }; | |
| let verifiedUser: typeof user.$inferSelect; | |
| let app: INestApplication; | |
| let db: NodePgDatabase & { $client: Pool }; | |
| let verifiedUser: typeof user.$inferSelect; | |
| let verifiedUser: typeof user.$inferSelect; |
|
|
||
| /* eslint-disable */ | ||
| var __TEARDOWN_MESSAGE__: string; | ||
|
|
There was a problem hiding this comment.
The typo 'that that' should be corrected to 'that' in this comment.
| // Start services that the app needs to run (e.g. database, docker-compose, etc.). |
| return drizzle({ | ||
| connection: { | ||
| user: process.env['DB_USERNAME'] as string, | ||
| password: process.env['DB_PASS'], | ||
| database: process.env['DB_DATABASE'], | ||
| host: process.env['DB_HOST'], | ||
| port: parseInt(process.env['DB_PORT'], 10), |
There was a problem hiding this comment.
The database configuration uses environment variables without validation or fallback values. If these environment variables are not set, the tests will fail with unclear errors. Consider adding validation or default values for test environments to make the tests more robust.
| return drizzle({ | |
| connection: { | |
| user: process.env['DB_USERNAME'] as string, | |
| password: process.env['DB_PASS'], | |
| database: process.env['DB_DATABASE'], | |
| host: process.env['DB_HOST'], | |
| port: parseInt(process.env['DB_PORT'], 10), | |
| const { | |
| DB_USERNAME = 'postgres', | |
| DB_PASS = 'postgres', | |
| DB_DATABASE = 'postgres', | |
| DB_HOST = 'localhost', | |
| DB_PORT = '5432', | |
| } = process.env; | |
| const port = Number.parseInt(DB_PORT, 10); | |
| if (Number.isNaN(port)) { | |
| throw new Error('Invalid DB_PORT environment variable: must be a number'); | |
| } | |
| return drizzle({ | |
| connection: { | |
| user: DB_USERNAME as string, | |
| password: DB_PASS, | |
| database: DB_DATABASE, | |
| host: DB_HOST, | |
| port, |
| useValue: { | ||
| verify: { | ||
| services: jest.fn().mockReturnThis(), | ||
| verifications: { | ||
| create: jest.fn(), | ||
| }, | ||
| capture: jest.fn(), | ||
| }, | ||
| }, |
There was a problem hiding this comment.
The ANALYTICS_CLIENT mock has an incorrect structure. It provides verify.capture but PostHog's capture method is called directly (analyticsClient.capture). The mock should be { capture: jest.fn(), alias: jest.fn() } similar to the MockAnalyticsModule implementation, not nested under a verify property.
| beforeAll(async () => { | ||
| const moduleFixture: TestingModule = await Test.createTestingModule({ | ||
| imports: [ | ||
| GraphQLModule.forRoot<ApolloDriverConfig>({ | ||
| driver: ApolloDriver, | ||
| autoSchemaFile: true, | ||
| subscriptions: { | ||
| 'graphql-ws': true, | ||
| }, | ||
| }), | ||
| WorkspaceModule, | ||
| MockCommonModule, | ||
| MockConfigModule, | ||
| MockDbModule, | ||
| MockRedisModule, | ||
| MockEventBusModule, | ||
| MockAnalyticsModule, | ||
| ], | ||
| }) | ||
| .overrideProvider(MainAuthGuard) | ||
| .useClass(MockMainAuthGuard) | ||
| .compile(); | ||
|
|
||
| app = moduleFixture.createNestApplication(); | ||
| db = app.get(DB_CONNECTION); | ||
| eventBus = app.get(EventBusService); | ||
|
|
||
| await app.init(); |
There was a problem hiding this comment.
The test suite shares a single database connection pool across all tests, which could lead to test interference if tests run in parallel. Jest runs tests in parallel by default. Consider either configuring Jest to run these integration tests serially (using --runInBand or --maxWorkers=1) or creating separate database connections per test to ensure proper isolation.
| }, | ||
| }); | ||
|
|
||
| expect(redeemedInviteRes.status).toBe(200); |
There was a problem hiding this comment.
The test verifies that the redeemed invite returns null but doesn't check for errors. If the query failed, res.body.data could also be null due to an error rather than the invite being properly deleted. Consider asserting that res.body.errors is undefined or that the response structure indicates successful query execution with no data.
| expect(redeemedInviteRes.status).toBe(200); | |
| expect(redeemedInviteRes.status).toBe(200); | |
| expect(redeemedInviteRes.body.errors).toBeUndefined(); |
Summary
Added basic integration test.
Related issue(s)
Type of change
Component
Description
n/a
How to test
n/a
Checklist
Screenshots (if UI)
Release notes
Short note for changelog/release (one line).