feat(testing): Vitest unit tests + GitHub Actions CI#12
Conversation
There was a problem hiding this comment.
Pull request overview
This PR sets up comprehensive testing infrastructure with Vitest and GitHub Actions CI, while also introducing significant new application functionality for driver management and location tracking. The PR includes 33 unit tests covering auth, ride, and driver services, plus schema validation tests. However, contrary to the PR description which states "Does not change any application code," this PR actually introduces substantial new features including a complete driver location/availability system using Redis geospatial features and PostGIS queries.
Changes:
- Vitest testing framework with comprehensive service and schema tests (33 tests total)
- GitHub Actions CI workflow for automated testing on PRs and main branch pushes
- New driver service with Redis-backed geospatial location tracking and availability matching
- Breaking changes to ride API endpoints (renamed routes)
- Enhanced health check endpoint with Redis status monitoring
Reviewed changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| turbo.json | Adds test task configuration with proper build dependencies |
| package.json | Adds test script command at workspace root |
| packages/common/schemas/index.ts | Exports new driver schema |
| packages/common/schemas/driver.schema.ts | New Zod schemas for driver status and location validation |
| apps/api-gateway/vitest.config.ts | Vitest configuration with globals, coverage, and common package alias |
| apps/api-gateway/package.json | Adds Vitest and coverage dependencies, test scripts |
| apps/api-gateway/src/utils/redis.ts | New Redis client utility with connection management and event handlers |
| apps/api-gateway/src/services/driver.service.ts | New driver service with online/offline status, location updates, and geospatial ride matching |
| apps/api-gateway/src/services/tests/auth.service.test.ts | Comprehensive auth service tests (9 tests) |
| apps/api-gateway/src/services/tests/ride.service.test.ts | Ride service tests (4 tests) |
| apps/api-gateway/src/services/tests/driver.service.test.ts | Driver service tests (9 tests) |
| apps/api-gateway/src/schemas/schemas.test.ts | Schema validation tests (11 tests) |
| apps/api-gateway/src/routes/user.routes.ts | New driver status and location endpoints |
| apps/api-gateway/src/routes/ride.routes.ts | Breaking route renames and new available rides endpoint |
| apps/api-gateway/src/routes.ts | Enhanced health check with Redis status |
| apps/api-gateway/src/mocks/db.ts | Prisma mock setup (unused) |
| apps/api-gateway/src/mocks/redis.ts | Redis mock setup (unused) |
| .github/workflows/ci.yml | CI workflow for automated testing |
| .gitignore | Adds .vscode/, .turbo/, learn/ exclusions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| router.get('/health', async (req, res) => { | ||
| let redisStatus = 'ok'; | ||
| try { | ||
| await redis.ping(); |
There was a problem hiding this comment.
The health check endpoint now calls Redis synchronously without error handling for the connection attempt. If Redis is slow or hangs, this could cause the health endpoint to timeout or block. Consider adding a timeout to the redis.ping() call to ensure the health check responds quickly even if Redis is unresponsive.
There was a problem hiding this comment.
Good point. Added try/catch with logger.warn({ err }, 'Redis health check failed') in the M1 review-fix commit — the error is now logged and surfaced. A ping timeout/race won't silently eat the error. We can add an explicit Promise.race timeout if needed in a follow-up.
| }); | ||
|
|
||
| rideRouter.patch('/cancelRide', authenticate, async (req, res) => { | ||
| rideRouter.patch('/cancel', authenticate, async (req, res) => { |
There was a problem hiding this comment.
API breaking change: The route path changed from '/cancelRide' to '/cancel'. This is a breaking change for existing API consumers and should be documented in the PR description or handled with deprecation notices. Consider whether this change is intentional and if it should include backwards compatibility or migration guidance.
There was a problem hiding this comment.
Intentional RESTful refactor — /cancel follows REST conventions better than /cancelRide. Documented as a breaking change in issue #13. Existing clients should update to the new paths. No backwards compatibility is planned at this stage given we are pre-production.
| @@ -0,0 +1,135 @@ | |||
| import redis from '../utils/redis.js'; | |||
There was a problem hiding this comment.
The PR description states "Does not change any application code" but this is inaccurate. The PR includes significant application code changes: new Redis utility module, new driver service with 4 exported functions, new driver schemas, new driver routes in user.routes.ts, a new ride route, enhanced health check endpoint, and breaking changes to existing ride route paths. The PR description should accurately reflect all these changes.
There was a problem hiding this comment.
You're right — the PR description was inaccurate. The testing PR inadvertently included the M1 application code. Updated the description and separated concerns: M1 code is now in its own merged PR (#11) with accurate documentation, and this PR focuses purely on the testing infrastructure.
| @@ -0,0 +1,23 @@ | |||
| import { z } from 'zod'; | |||
|
|
|||
| const locationTuple = z.tuple([z.number(), z.number()]).describe('[longitude, latitude]'); | |||
There was a problem hiding this comment.
Inconsistent location coordinate schema definition. The driver.schema.ts uses z.tuple([z.number(), z.number()]) for coordinates, while ride.schema.ts uses z.array(z.number()).length(2) for the same concept. Consider using tuple consistently across all schemas for type safety, as tuples provide better type inference than length-constrained arrays.
| const locationTuple = z.tuple([z.number(), z.number()]).describe('[longitude, latitude]'); | |
| const locationTuple = z.array(z.number()).length(2).describe('[longitude, latitude]'); |
There was a problem hiding this comment.
Fixed — ride.schema.ts now uses z.tuple with min/max bounds validation (lon: -180..180, lat: -90..90) consistent with driver.schema.ts. Updated in the M1 review-fix commit.
| WHERE status = 'REQUESTED' | ||
| AND ST_DWithin( | ||
| "pickupLocation"::geography, | ||
| ST_SetSRID(ST_MakePoint(${parseFloat(lon)}, ${parseFloat(lat)}), 4326)::geography, |
There was a problem hiding this comment.
SQL injection vulnerability: The driver location coordinates from Redis (lon, lat) are directly interpolated into the raw SQL query using template literals. Although Redis returns strings, using parseFloat and directly interpolating into SQL can be risky. Consider using Prisma's parameter binding or validating/sanitizing the parsed values before interpolation to prevent potential SQL injection if Redis data is compromised.
There was a problem hiding this comment.
Fixed — switched from a raw template literal to Prisma.sql``` tagged template in the M1 review-fix commit. lonandlatare now proper Prisma bind parameters. TheparseFloat` values are also validated as numbers before being used.
| if (!req.user) { | ||
| return res.status(401).json({ | ||
| success: false, | ||
| message: 'Unauthorized', | ||
| }); | ||
| } |
There was a problem hiding this comment.
Redundant authentication check. The authenticate middleware already ensures req.user is set, so the check if (!req.user) at line 56 will never be true after authenticate middleware passes. This check is redundant and can be removed.
There was a problem hiding this comment.
Agreed - the authenticate middleware guarantees req.user is set. The if (!req.user) guard is TypeScript type-narrowing to satisfy the compiler since Express does not infer the middleware contract. Will replace with a proper Express request extension type in a follow-up.
|
|
||
| describe('driverStatusSchema', () => { | ||
| it('accepts going offline without location', async () => { | ||
| const result = await driverStatusSchema.safeParseAsync({ body: { isActive: false } }); |
There was a problem hiding this comment.
Using safeParseAsync is unnecessary for these synchronous schemas. The schemas don't use any async refinements or transformations, so safeParse (synchronous version) would be more appropriate and slightly more efficient. This applies to all schema tests in this file.
There was a problem hiding this comment.
Fixed — replaced all safeParseAsync calls with safeParse in schemas.test.ts. The schemas have no async refinements so the synchronous version is correct and more efficient. Updated in the latest testing-branch commit.
| // Vitest auto-mock for Redis client | ||
| import { vi } from 'vitest'; | ||
|
|
||
| const redisMock = { | ||
| geoadd: vi.fn(), | ||
| geopos: vi.fn(), | ||
| zrem: vi.fn(), | ||
| set: vi.fn(), | ||
| del: vi.fn(), | ||
| ping: vi.fn(), | ||
| call: vi.fn(), | ||
| }; | ||
|
|
||
| export default redisMock; |
There was a problem hiding this comment.
These mock files in mocks/ directory are not being used by the tests. The test files use vi.hoisted() and vi.mock() to create inline mocks instead of these manual mock files. Either remove these unused files or update the tests to use Vitest's automatic mocking feature by placing these files in the correct location and using vi.mock() without the second argument.
| // Vitest auto-mock for Redis client | |
| import { vi } from 'vitest'; | |
| const redisMock = { | |
| geoadd: vi.fn(), | |
| geopos: vi.fn(), | |
| zrem: vi.fn(), | |
| set: vi.fn(), | |
| del: vi.fn(), | |
| ping: vi.fn(), | |
| call: vi.fn(), | |
| }; | |
| export default redisMock; | |
| // Helper mock for a Redis client used in tests. | |
| type RedisMock = { | |
| geoadd: (...args: unknown[]) => unknown; | |
| geopos: (...args: unknown[]) => unknown; | |
| zrem: (...args: unknown[]) => unknown; | |
| set: (...args: unknown[]) => unknown; | |
| del: (...args: unknown[]) => unknown; | |
| ping: (...args: unknown[]) => unknown; | |
| call: (...args: unknown[]) => unknown; | |
| }; | |
| export const redisMock: RedisMock = { | |
| geoadd: (..._args: unknown[]) => undefined, | |
| geopos: (..._args: unknown[]) => undefined, | |
| zrem: (..._args: unknown[]) => undefined, | |
| set: (..._args: unknown[]) => undefined, | |
| del: (..._args: unknown[]) => undefined, | |
| ping: (..._args: unknown[]) => undefined, | |
| call: (..._args: unknown[]) => undefined, | |
| }; |
There was a problem hiding this comment.
Fixed — removed the entire __mocks__/ directory. Tests use vi.hoisted + vi.mock with factory functions, which is the correct Vitest ESM mocking pattern. The manual mock files were never auto-applied and have been cleaned up.
| await redis.zrem(DRIVERS_GEO_KEY, driver.id); | ||
| await redis.del(heartbeatKey(driver.id)); | ||
|
|
||
| await prisma.driver.update({ where: { id: driver.id }, data: { isActive: false } }); | ||
|
|
||
| logger.info({ driverId: driver.id }, 'Driver went offline'); | ||
| return { success: true, message: 'Driver is now offline' }; |
There was a problem hiding this comment.
Potential data inconsistency: The function performs Redis operations (zrem, del) and Prisma update sequentially without transaction handling or rollback mechanism. If the Prisma update fails after Redis operations succeed, the driver will be removed from Redis but still marked as active in the database. Consider wrapping these operations in a try-catch and implementing rollback logic, or documenting this as a known limitation.
There was a problem hiding this comment.
Fixed in M1 review-fix commit — reordered to DB-first: Prisma update (isActive: false) runs before Redis cleanup. If the DB update fails, Redis is unchanged. If Redis zrem/del fails after DB update, the driver is inactive in the DB but still has a stale GEO entry — this will naturally expire via the heartbeat TTL (60s). Acceptable eventual consistency for now; a periodic cleanup job is tracked in issue #13.
| })); | ||
| vi.mock('../../logger.js', () => ({ default: { info: vi.fn(), error: vi.fn(), warn: vi.fn() } })); | ||
|
|
||
| import { comparePassword } from '../../utils/password.js';import { registerUser, loginUser, getUserProfile } from '../auth.service.js'; |
There was a problem hiding this comment.
There is a missing newline between the two import statements. The import from 'password.js' and the import from 'auth.service.js' should be on separate lines.
| import { comparePassword } from '../../utils/password.js';import { registerUser, loginUser, getUserProfile } from '../auth.service.js'; | |
| import { comparePassword } from '../../utils/password.js'; | |
| import { registerUser, loginUser, getUserProfile } from '../auth.service.js'; |
There was a problem hiding this comment.
Fixed — added the missing newline between the two import statements in auth.service.test.ts. Updated in the latest testing-branch commit.
- Install Vitest + @vitest/coverage-v8 in api-gateway - Configure vitest.config.ts with ESM + common alias resolution - Unit tests for auth.service (9 tests), ride.service (4 tests), driver.service (9 tests) - Schema validation tests for driverStatusSchema, driverLocationSchema, rideSchema (11 tests) - Use vi.hoisted pattern for correct mock initialization with ESM - Add .github/workflows/ci.yml: build common then run tests on every PR/push to main - Add pnpm test script to root and api-gateway package.json - Add test task to turbo.json All 33 tests passing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add zscore to redisMock; add 'not in GEO set' test case for updateDriverLocation (required after M1 review fix introduced zscore guard) - Fix missing newline between imports in auth.service.test.ts - Replace safeParseAsync with safeParse in schemas.test.ts (schemas are synchronous) - Remove unused __mocks__/ directory (tests use inline vi.hoisted mocks) - Alias 'common' to TypeScript source in vitest.config.ts (avoids stale root-owned dist) - Add Typecheck step to GitHub Actions CI workflow - Rebase onto main to include M1 review fixes (DB-first ordering, coordinate validation, Prisma.sql parameterization, lazyConnect:true, etc.) All 34 tests pass locally. Fixes: #13 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1580ec9 to
93b8f04
Compare
…ge.json pnpm/action-setup@v4 reads pnpm@10.18.0 from the root package.json packageManager field automatically; specifying version: 10 alongside it causes ERR_PNPM_BAD_PM_VERSION. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CI requires the lockfile for --frozen-lockfile install. pnpm-lock.yaml should always be committed to the repo. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… task turbo.json has no 'typecheck' task and packages have no 'typecheck' script, so 'pnpm typecheck' fails with 'Could not find task typecheck in project'. Use pnpm --filter api-gateway exec tsc --noEmit directly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
Sets up the test infrastructure for the project: Vitest unit tests for all service functions, Zod schema tests, and automated CI on every PR.
Changes
Testing Framework
api-gateway(ESM-native, no config hacks needed)vitest.config.tswithglobals: true,clearMocks, andcommonaliasTest Files (33 tests total, all passing)
auth.service.test.tsride.service.test.tsdriver.service.test.tsschemas.test.tsMocking Pattern
Uses
vi.hoistedto initialize mocks before ESM imports — the correct way to mock in Vitest with ESM.GitHub Actions (
.github/workflows/ci.yml)Runs on every PR and push to
main:pnpm install --frozen-lockfilecommonpackage (needed fordist/types)api-gatewaytests (no Docker/infra required — all mocked)Does not change any application code