Conversation
…ucture and naming
…ntegration test steps
…ss-validator depends on a validor < 6.0 which has a security issue
There was a problem hiding this comment.
Pull request overview
This PR implements test infrastructure and initial test suites for the payment records application, including unit tests for utilities and middleware, integration tests for the authentication service, and CI/CD pipeline enhancements to run these tests automatically.
Changes:
- Added Jest testing framework with TypeScript support and test scripts for unit and integration tests
- Implemented unit tests for password comparison utility and body parser middleware
- Created integration tests for user registration in the auth service with test database configuration
- Enhanced CI/CD pipeline to include test execution with Docker-based PostgreSQL test database
- Updated error message casing for consistency with test expectations
- Added test database container to docker-compose configuration
Reviewed changes
Copilot reviewed 9 out of 11 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tests/unit/utils.spec.ts | Unit test for password comparison utility function |
| src/tests/unit/middlewares.spec.ts | Unit tests for Zod schema validation middleware |
| src/tests/integration/db.ts | Test database configuration using TypeORM DataSource |
| src/tests/integration/auth.spec.ts | Integration tests for auth service registration |
| src/lib/enums.ts | Updated USER_ALREADY_EXISTS error message casing to lowercase |
| package.json | Added Jest, ts-jest, and @types/jest dependencies with test scripts |
| jest.config.js | Jest configuration with TypeScript support and path mapping |
| docker-compose.yaml | Added postgres-test service for integration testing with health checks |
| .github/workflows/ci.yaml | Enhanced CI pipeline with test job including database setup and test execution |
| .vscode/mcp.json | Added VSCode MCP server configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| test("comparePasswords return true for matching passwords", () => { | ||
| const password = 'mypassword'; | ||
| const digest = bcrypt.hashSync(password, 10); | ||
|
|
||
| comparePasswords(password, digest).then((res) => { | ||
| expect(res).toBe(true); | ||
| }) | ||
|
|
||
| const wrongPassword = 'wrongpassword'; | ||
| comparePasswords(wrongPassword, digest).then((res) => { | ||
| expect(res).toBe(false); | ||
| }) | ||
| }) No newline at end of file |
There was a problem hiding this comment.
This test does not properly handle the asynchronous nature of the comparePasswords function. The test will complete before the promises resolve, meaning the assertions may never run or may not be awaited properly. The test should either use async/await or return the promises. Additionally, testing two different scenarios in a single test case is not a best practice - these should be split into separate test cases.
| test("comparePasswords return true for matching passwords", () => { | |
| const password = 'mypassword'; | |
| const digest = bcrypt.hashSync(password, 10); | |
| comparePasswords(password, digest).then((res) => { | |
| expect(res).toBe(true); | |
| }) | |
| const wrongPassword = 'wrongpassword'; | |
| comparePasswords(wrongPassword, digest).then((res) => { | |
| expect(res).toBe(false); | |
| }) | |
| }) | |
| test("comparePasswords returns true for matching passwords", async () => { | |
| const password = 'mypassword'; | |
| const digest = bcrypt.hashSync(password, 10); | |
| const res = await comparePasswords(password, digest); | |
| expect(res).toBe(true); | |
| }); | |
| test("comparePasswords returns false for non-matching passwords", async () => { | |
| const password = 'mypassword'; | |
| const digest = bcrypt.hashSync(password, 10); | |
| const wrongPassword = 'wrongpassword'; | |
| const res = await comparePasswords(wrongPassword, digest); | |
| expect(res).toBe(false); | |
| }); |
| import { comparePasswords } from "../../lib/utils"; | ||
| import bcrypt from "bcryptjs"; | ||
|
|
||
| test("comparePasswords return true for matching passwords", () => { |
There was a problem hiding this comment.
The test description has a grammatical error. It should be "returns" instead of "return" to match subject-verb agreement (singular subject "comparePasswords" requires singular verb).
| test("comparePasswords return true for matching passwords", () => { | |
| test("comparePasswords returns true for matching passwords", () => { |
src/tests/integration/auth.spec.ts
Outdated
| dataSource.query( | ||
| ` | ||
| TRUNCATE TABLE "users" CASCADE | ||
| ` | ||
| ) |
There was a problem hiding this comment.
The await keyword is missing before the dataSource.query call. This means the TRUNCATE TABLE query will execute asynchronously without waiting for completion, which could lead to race conditions where tests start running before the database is properly cleaned. This should be: await dataSource.query(...).
| { | ||
| "servers": { | ||
| "atlassian/atlassian-mcp-server": { | ||
| "type": "http", | ||
| "url": "https://mcp.atlassian.com/v1/sse", | ||
| "gallery": "https://api.mcp.github.com", | ||
| "version": "1.0.0" | ||
| } | ||
| }, | ||
| "inputs": [] | ||
| } No newline at end of file |
There was a problem hiding this comment.
This VSCode configuration file for the Atlassian MCP server appears to be unrelated to the test implementation work described in the PR. Unless this is intentionally part of the development setup for this project, it should likely be excluded from this PR or added to .gitignore to prevent individual developer configurations from being committed.
| { | |
| "servers": { | |
| "atlassian/atlassian-mcp-server": { | |
| "type": "http", | |
| "url": "https://mcp.atlassian.com/v1/sse", | |
| "gallery": "https://api.mcp.github.com", | |
| "version": "1.0.0" | |
| } | |
| }, | |
| "inputs": [] | |
| } | |
| {} |
.github/workflows/ci.yaml
Outdated
| until docker exec payment_records_test pg_isready; do | ||
| echo "Waiting for database..." |
There was a problem hiding this comment.
The database readiness check is attempting to run pg_isready inside the 'payment_records_test' container, but this container is defined to depend on the 'postgres' service's health check. The wait loop may fail because it's checking the wrong container or the container might not be accessible by this name. Consider using 'docker compose ps' to verify the container is running first, or use the health check mechanism already defined in docker-compose.yaml.
| until docker exec payment_records_test pg_isready; do | |
| echo "Waiting for database..." | |
| echo "Waiting for postgres service to become healthy..." | |
| POSTGRES_CONTAINER_ID=$(docker compose ps -q postgres) | |
| if [ -z "$POSTGRES_CONTAINER_ID" ]; then | |
| echo "Postgres service container not found. Current services:" | |
| docker compose ps | |
| exit 1 | |
| fi | |
| until [ "$(docker inspect -f '{{.State.Health.Status}}' "$POSTGRES_CONTAINER_ID")" = "healthy" ]; do | |
| echo "Database not ready yet, waiting..." |
| import { comparePasswords } from "../../lib/utils"; | ||
| import bcrypt from "bcryptjs"; | ||
|
|
||
| test("comparePasswords return true for matching passwords", () => { | ||
| const password = 'mypassword'; | ||
| const digest = bcrypt.hashSync(password, 10); | ||
|
|
||
| comparePasswords(password, digest).then((res) => { | ||
| expect(res).toBe(true); | ||
| }) | ||
|
|
||
| const wrongPassword = 'wrongpassword'; | ||
| comparePasswords(wrongPassword, digest).then((res) => { | ||
| expect(res).toBe(false); | ||
| }) | ||
| }) No newline at end of file |
There was a problem hiding this comment.
The utils.spec.ts file only tests the comparePasswords function, but there are other utility functions in src/lib/utils.ts (hashPassword and extractTokenFromHeader) that lack test coverage. For consistency with comprehensive testing, consider adding tests for these functions as well.
| - name: Download build artifacts | ||
| uses: actions/download-artifact@v4 | ||
| with: | ||
| name: build |
There was a problem hiding this comment.
The build artifacts are downloaded but there's no specified path parameter, which means they will be downloaded to the current directory. However, the build artifacts should be placed in the 'dist/' directory to match the project structure. Add 'path: dist/' to this step's 'with' configuration.
| name: build | |
| name: build | |
| path: dist/ |
docker-compose.yaml
Outdated
| environment: | ||
| POSTGRES_HOST: localhost | ||
| POSTGRES_PORT: 5432 | ||
| POSTGRES_USER: postgres | ||
| POSTGRES_PASSWORD: postgres | ||
| POSTGRES_DB: payment_records_test No newline at end of file |
There was a problem hiding this comment.
The postgres-test service has incorrect environment variable values. POSTGRES_PORT is set to 5432, but since this is running inside a container and mapped to host port 5433, the internal port should remain 5432. However, POSTGRES_HOST should be the service name for inter-container communication or localhost if accessed from host. More critically, these environment variables (POSTGRES_HOST, POSTGRES_PORT) are not standard PostgreSQL Docker image variables and won't affect the database configuration - the standard variables are POSTGRES_USER, POSTGRES_PASSWORD, and POSTGRES_DB which are correctly set.
docker-compose.yaml
Outdated
| environment: | ||
| POSTGRES_HOST: localhost | ||
| POSTGRES_PORT: 5432 | ||
| POSTGRES_USER: postgres | ||
| POSTGRES_PASSWORD: postgres | ||
| POSTGRES_DB: payment_records |
There was a problem hiding this comment.
POSTGRES_HOST and POSTGRES_PORT are not standard PostgreSQL Docker image environment variables and have no effect on the database configuration. The PostgreSQL official Docker image only recognizes POSTGRES_USER, POSTGRES_PASSWORD, POSTGRES_DB, POSTGRES_INITDB_ARGS, POSTGRES_INITDB_WALDIR, and POSTGRES_HOST_AUTH_METHOD. These two variables can be safely removed unless they're intended for application configuration (in which case they should be documented).
src/tests/integration/auth.spec.ts
Outdated
| describe('Testing auth service and operation: register, login and token refresh', () => { | ||
| let authService: AuthService; | ||
| let emailService: Email<MailOptions>; | ||
|
|
||
| beforeAll(async () => { | ||
| await dataSource.initialize(); | ||
|
|
||
| dataSource.query( | ||
| ` | ||
| TRUNCATE TABLE "users" CASCADE | ||
| ` | ||
| ) | ||
|
|
||
| emailService = new MockEmailService() | ||
| authService = new AuthService(dataSource.getRepository(User), emailService); | ||
| }) | ||
|
|
||
| afterAll(() => { | ||
| dataSource.destroy().then(() => { | ||
| console.log("Data source destroyed after auth service tests"); | ||
| }) | ||
| }) | ||
|
|
||
| test('Register user', async () => { | ||
| const user = { | ||
| name: 'John Doe', | ||
| age: 30, | ||
| email: `john.doe@example.com`, | ||
| password: 'strongPassword123' | ||
| } | ||
|
|
||
| const response = await authService.register(user); | ||
|
|
||
| expect(response).toHaveProperty('id'); | ||
| expect(response).toHaveProperty('name', user.name); | ||
| expect(response).toHaveProperty('email', user.email); | ||
| }) | ||
|
|
||
| test('Register user with existing email should fail', async () => { | ||
| const user = { | ||
| name: 'John Doe', | ||
| age: 30, | ||
| email: `john.doe@example.com`, | ||
| password: 'strongPassword123' | ||
| } | ||
|
|
||
| await expect(authService.register(user)).rejects.toThrow('User already exists'); | ||
| }) | ||
|
|
||
| test('Register user with missing attributes should fail', async () => { | ||
| const user = { | ||
| name: 'John Doe', | ||
| age: 30, | ||
| password: 'strongPassword123' | ||
| } | ||
|
|
||
| await expect(authService.register(user as CreateUserDto)).rejects.toThrow(); | ||
| }) | ||
| }) No newline at end of file |
There was a problem hiding this comment.
The integration tests don't test the login and token refresh operations despite the describe block stating they will test 'register, login and token refresh'. Only the register operation is tested. Consider adding tests for login with valid credentials, login with invalid credentials, and token refresh functionality to match the test suite description.
…stead of change the dto object
…rvice, and add integration tests for account operations
…amline afterAll cleanup in auth service tests
…al:Jaedsonn/payment_records into KAN-15-arch-005-implementar-testes
No description provided.