feat: add tests, coverage, and Codecov CI setup#1
Conversation
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a modular User API (entities/services/controllers) and introduces Jest + Supertest tests with coverage reporting and Codecov upload via GitHub Actions.
Changes:
- Implement User entity, service, and controller, and wire routes in
index.js. - Add unit + web/integration tests and Jest coverage thresholds/config.
- Update CI workflow to run tests, generate coverage, and upload to Codecov; refine Docker build.
Reviewed changes
Copilot reviewed 12 out of 14 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/web/api.test.js | Adds Supertest-based integration tests for API endpoints |
| tests/unit/UserService.test.js | Adds unit tests for UserService behavior |
| tests/unit/User.test.js | Adds unit tests for User entity serialization/validation |
| src/services/UserService.js | Implements in-memory user CRUD + health reporting |
| src/entities/User.js | Adds User entity with validation and toJSON() |
| src/controllers/UserController.js | Adds controller methods mapping HTTP to service operations |
| package.json | Adds Jest/Supertest + test scripts and coverage |
| jest.config.js | Configures coverage collection and global thresholds |
| index.js | Wires routes/controllers and exports app for testing |
| README.md | Documents architecture, scripts, API, and CI/Codecov |
| Dockerfile | Switches to npm ci, narrows COPY set, adds HEALTHCHECK |
| .github/workflows/action.yml | Runs tests, builds Docker image, uploads coverage to Codecov |
| .dockerignore | Excludes tests/docs/git metadata from Docker build context |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const app = require('../../index'); | ||
|
|
||
| describe('User API Endpoints', () => { |
There was a problem hiding this comment.
The integration tests import a singleton app instance, which (given index.js creates a single in-memory UserService) couples tests through shared state within the file. To keep tests isolated and easier to extend, consider exporting an app factory (e.g., createApp() that instantiates fresh services/controllers) or resetting the module between tests (e.g., jest.resetModules() + re-require) so each test/describe can start from a known clean state.
| const app = require('../../index'); | |
| describe('User API Endpoints', () => { | |
| let app; | |
| describe('User API Endpoints', () => { | |
| beforeEach(() => { | |
| jest.resetModules(); | |
| app = require('../../index'); | |
| }); |
| const res = await request(app) | ||
| .get('/users') | ||
| .expect(200); | ||
| expect(Array.isArray(res.body)).toBe(true); |
There was a problem hiding this comment.
This test name claims the users array is empty, but the assertion only checks that the response is an array. Either assert res.body is empty (e.g., length 0) or rename the test to match what it verifies.
| expect(Array.isArray(res.body)).toBe(true); | |
| expect(res.body).toEqual([]); |
| } | ||
|
|
||
| /** | ||
| * GET / - Health check |
There was a problem hiding this comment.
The doc comment says GET / - Health check, but the route wired in index.js is /health. Update the comment to prevent confusion for future maintainers.
| * GET / - Health check | |
| * GET /health - Health check |
| const { id } = req.params; | ||
| const user = this.userService.getUserById(parseInt(id)); | ||
| if (!user) { | ||
| return res.status(404).json({ error: 'User not found' }); | ||
| } | ||
| res.status(200).json(user.toJSON()); |
There was a problem hiding this comment.
Use parseInt(id, 10) (or Number(id)) instead of parseInt(id) to avoid radix ambiguity and keep numeric parsing explicit. The same applies to the parseInt(id) calls in updateUser and deleteUser.
| "test": "echo \"Tests passed\"" | ||
| "test": "jest --coverage", | ||
| "test:unit": "jest tests/unit --no-coverage", | ||
| "test:web": "jest tests/web --forceExit --no-coverage", |
There was a problem hiding this comment.
--forceExit can hide resource leaks (open handles) and make test cleanup issues harder to detect. Prefer removing it and using --detectOpenHandles while fixing the underlying leak, or ensure the app/resources are properly torn down in tests.
| "test:web": "jest tests/web --forceExit --no-coverage", | |
| "test:web": "jest tests/web --detectOpenHandles --no-coverage", |
| - name: Run Unit Tests | ||
| run: npm run test:unit | ||
|
|
||
| - name: Run Web/Integration Tests | ||
| run: npm run test:web | ||
|
|
There was a problem hiding this comment.
CI runs the test suite multiple times (unit, web, then all again for coverage), which increases duration and compute cost. Consider running only npm test (coverage) and, if you still want separate visibility, use Jest projects/groups or split by testMatch while generating coverage once.
| - name: Run Unit Tests | |
| run: npm run test:unit | |
| - name: Run Web/Integration Tests | |
| run: npm run test:web |
| | Méthode | Route | Description | | ||
| |---------|-------|-------------| | ||
| | GET | `/` | Message de bienvenue | | ||
| | GET | `/health` | Vérification de santé du service | | ||
| | GET | `/users` | Récupérer tous les utilisateurs | | ||
| | GET | `/users/:id` | Récupérer un utilisateur | | ||
| | POST | `/users` | Créer un utilisateur | | ||
| | PUT | `/users/:id` | Mettre à jour un utilisateur | | ||
| | DELETE | `/users/:id` | Supprimer un utilisateur | |
There was a problem hiding this comment.
This Markdown table has an extra leading | on each line (|| ...), which can break rendering on GitHub. Use standard table syntax with a single leading pipe per row.
| ENTRYPOINT ["node", "index.js"] No newline at end of file | ||
|
|
||
| HEALTHCHECK --interval=30s --timeout=3s --start-period=5s --retries=3 \ | ||
| CMD node -e "require('http').get('http://localhost:8080/health', (r) => {if (r.statusCode !== 200) throw new Error(r.statusCode)})" |
There was a problem hiding this comment.
The HEALTHCHECK script doesn't consume the response body or explicitly handle connection errors/timeouts, which can lead to hanging checks or noisy/unreliable health reporting. Consider draining the response (r.resume()), adding an error handler, and ensuring the process exits deterministically on failure (or use a lightweight tool like wget if available).
| CMD node -e "require('http').get('http://localhost:8080/health', (r) => {if (r.statusCode !== 200) throw new Error(r.statusCode)})" | |
| CMD node -e "const http = require('http'); const req = http.get('http://localhost:8080/health', (r) => { r.resume(); process.exit(r.statusCode === 200 ? 0 : 1); }); req.on('error', () => process.exit(1)); req.setTimeout(2500, () => { req.destroy(); process.exit(1); });" |
No description provided.