diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 692c211..3ba110d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -35,3 +35,18 @@ jobs: - run: npm ci - run: npm test + + e2e: + name: E2E tests + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-node@v4 + with: + node-version: "24" + cache: "npm" + + - run: npm ci + + - run: npm run test:e2e diff --git a/README.md b/README.md index 7450a03..6909a60 100644 --- a/README.md +++ b/README.md @@ -20,9 +20,12 @@ containerización, CI/CD y observabilidad. - [x] API REST con lógica de negocio básica (CRUD de `Products`: create/findAll/findOne/update/remove, con DTOs y `ValidationPipe` global) -- [ ] Suite de pruebas unitarias — existen specs (`products.service.spec.ts`, `products.controller.spec.ts`, - `app.e2e-spec.ts`) pero son el boilerplate de `nest generate` (`should be defined`); falta cubrir la lógica real del - CRUD +- [x] Suite de pruebas unitarias — 34 tests que cubren la lógica real del CRUD (`products.service.spec.ts`), la + conversión de parámetros del controlador (`products.controller.spec.ts`) y el middleware de logs + (`logger.middleware.spec.ts`). Cobertura: 100 % de statements y funciones sobre los archivos con lógica + (ver [Estrategia de Pruebas](#estrategia-de-pruebas)) +- [x] Pruebas end-to-end — 16 tests sobre el CRUD completo contra la app real (`products.e2e-spec.ts`): ciclo + create/read/update/delete, 404 y validación de DTOs. Corren en CI como job propio - [x] Documentación interactiva (Swagger/OpenAPI) — `@nestjs/swagger`, expuesta en `/api` (Swagger UI) y `/api-json` (spec OpenAPI); DTOs anotados vía CLI plugin (`nest-cli.json`), sin requerir `@ApiProperty` manual @@ -104,8 +107,11 @@ telemetría. ```bash npm run test # unitarios npm run test:e2e # end-to-end +npm run test:cov # unitarios + reporte de cobertura ``` +Los unitarios y los e2e corren en CI como jobs separados en cada Pull Request; la cobertura se consulta localmente. + ## Convenciones de Desarrollo ### Nombres de Ramas @@ -157,7 +163,7 @@ Un cambio incompatible hacia atrás se marca agregando `!` después del tipo/sco B["Pull Request a main"] - B --> C{"ci.yml: Lint + Unit tests"} + B --> C{"ci.yml: Lint + Unit tests + E2E tests"} C -- "falla" --> X["PR no se integra (Andon Cord)"] C -- "pasa" --> D["Merge a main"] D --> E["git tag vX.Y.Z + push del tag"] @@ -207,8 +213,8 @@ _La etapa de Render (últimos dos pasos) está pendiente de verificar._ #### Workflows -- **`ci.yml`:** se ejecuta en Pull Requests hacia `main` y además es reutilizable (`workflow_call`). Tiene dos jobs: - `Lint` y `Unit tests`. +- **`ci.yml`:** se ejecuta en Pull Requests hacia `main` y además es reutilizable (`workflow_call`). Tiene tres + jobs, que corren en paralelo: `Lint`, `Unit tests` y `E2E tests`. - **`release.yml`:** se dispara al pushear un tag `v*.*.*`. Sus jobs se encadenan con `needs`: `ci` (reutiliza `ci.yml`) → `docker` (build y push a Docker Hub) → `deploy` (deploy hook de Render). - **Credenciales:** secrets del repositorio (`DOCKERHUB_USERNAME`, `DOCKERHUB_TOKEN`, `RENDER_DEPLOY_HOOK_URL`); nunca @@ -241,7 +247,46 @@ complejidad recomendada para equipos pequeños — el caso de este TP. ![Configuración de protección de la rama main](docs/images/branch-protection.png) _Pendiente: confirmar en Settings > Branches (o Rulesets) que "Require status checks to pass" esté activo para los -jobs `Lint` y `Unit tests`; de eso depende que el Andon Cord bloquee el merge de un PR con el CI en rojo._ +jobs `Lint`, `Unit tests` y `E2E tests`; de eso depende que el Andon Cord bloquee el merge de un PR con el CI en +rojo._ + +#### Estrategia de Pruebas + +Los tests son la condición que corta el flujo en un Pull Request, así que lo que importa no es solo que pasen sino +que el número que reportan sea confiable. + +Las pruebas siguen el patrón de `@nestjs/testing`: cada test construye el módulo con `Test.createTestingModule`, lo +que da una instancia nueva por caso y evita estado compartido entre tests. El servicio guarda los productos en +memoria y se prueba directo; en el controlador el servicio va mockeado (`useValue`), porque lo único propio que tiene +es convertir el `id` de la URL de string a número — la lógica del CRUD ya está cubierta en el test del servicio y no +tiene sentido volver a ejercitarla a través suyo. + +**El denominador de la cobertura.** Por defecto, Vitest solo mide los archivos que algún test importa: los que no +tienen test quedan fuera de la cuenta y el porcentaje sale inflado. Con esa configuración el proyecto reportaba +100 %, y midiendo todo `src/` el número real era 79 %. Por eso `vitest.config.ts` declara `coverage.include` +explícitamente, y excluye solo lo que no tiene lógica que probar: + +| Excluido | Motivo | +|---|---| +| `src/main.ts` | Bootstrap: levanta el servidor, sin lógica propia | +| `src/**/*.module.ts` | Solo declaran `controllers` y `providers` | + +Con ese denominador la cobertura es de 100 % en statements y funciones. El 90 % en ramas corresponde a una sola rama +sin cubrir, que es el decorador `@Controller()` de `app.controller.ts`: un artefacto de la instrumentación, no un +camino de código real. + +**Los e2e tienen que probar la app que se despliega.** Los tests end-to-end no ejecutan `main.ts`: arman la +aplicación con `createNestApplication()`. Como la configuración global vivía dentro de `bootstrap()`, la app de los +tests salía sin el `ValidationPipe` y por lo tanto no validaba nada. Un `POST /products` con un body inválido +devolvía **201 en los tests y 400 en producción**: la suite habría dado luz verde a una regresión en la validación. + +La corrección fue extraer esa configuración a `src/app.setup.ts` (`configureApp`), que ahora usan tanto `main.ts` +como los e2e, de modo que no puedan volver a divergir. Se verificó quitando la llamada del setup de los tests: 6 de +los 16 e2e fallan, así que la suite detecta efectivamente esa clase de regresión. Swagger se queda en `main.ts` +porque expone documentación y no altera el manejo de los requests. + +Es el mismo problema de paridad entre entornos que motivó el Dockerfile y la decisión sobre los colores del logger, +solo que acá afectaba a la propia red de contención. #### Optimización de Contenedores (Dockerfile) @@ -419,14 +464,15 @@ _Borrador. Los apartados marcados como Pendiente dependen de la Fase 5 o del exp (`node:24.20-alpine3.24`) y dependencias instaladas con `npm ci` desde el lockfile. - La imagen se construye una sola vez en CI; ese artefacto, identificado por su tag, es el que debe desplegarse (_pendiente de verificar en Render_). -- Matiz: `Lint` y `Unit tests` corren directamente en el runner con Node 24 (el mismo major que la imagen), no dentro +- Matiz: los tres jobs corren directamente en el runner con Node 24 (el mismo major que la imagen), no dentro del contenedor. #### Segunda Forma (Feedback rápido y Andon Cord) **Puntos donde el flujo corta el cable** -1. **Pull Request:** si `Lint` o `Unit tests` fallan, el check queda en rojo. Que el merge quede bloqueado depende de +1. **Pull Request:** si `Lint`, `Unit tests` o `E2E tests` fallan, el check queda en rojo. Que el merge quede + bloqueado depende de que "Require status checks to pass" esté activo en la protección de `main` (_pendiente de confirmar_). 2. **Release:** el job `docker` tiene `needs: ci`, así que si lint o tests fallan sobre el tag no se construye ni se publica la imagen; el job `deploy` tiene `needs: docker`, así que no se dispara si la imagen no se publicó. diff --git a/src/app.setup.spec.ts b/src/app.setup.spec.ts new file mode 100644 index 0000000..9545270 --- /dev/null +++ b/src/app.setup.spec.ts @@ -0,0 +1,21 @@ +import { INestApplication, ValidationPipe } from '@nestjs/common'; +import { configureApp } from './app.setup.js'; + +describe('configureApp', () => { + const appDouble = () => ({ useGlobalPipes: vi.fn() }) as unknown as INestApplication; + + it('should register the global ValidationPipe', () => { + const app = appDouble(); + + configureApp(app); + + expect(app.useGlobalPipes).toHaveBeenCalledOnce(); + expect(vi.mocked(app.useGlobalPipes).mock.calls[0][0]).toBeInstanceOf(ValidationPipe); + }); + + it('should return the same app instance, so it can be chained', () => { + const app = appDouble(); + + expect(configureApp(app)).toBe(app); + }); +}); diff --git a/src/app.setup.ts b/src/app.setup.ts new file mode 100644 index 0000000..ac48c65 --- /dev/null +++ b/src/app.setup.ts @@ -0,0 +1,20 @@ +import { INestApplication, ValidationPipe } from '@nestjs/common'; + +/** + * Configuración global que afecta cómo la aplicación procesa los requests. + * + * Vive acá, y no dentro de `bootstrap()`, porque los tests e2e no ejecutan `main.ts`: + * arman la app con `createNestApplication()`. Si esta configuración estuviera solo en + * `main.ts`, los e2e probarían una aplicación distinta de la que se despliega — de hecho + * pasaba: sin el ValidationPipe, un POST con un body inválido devolvía 201 en los tests + * y 400 en producción. + * + * Todo lo que cambie el comportamiento de un request va acá, para que `main.ts` y los + * tests no puedan divergir. La configuración de Swagger se queda en `main.ts`: expone + * documentación, no altera el manejo de los requests. + */ +export function configureApp(app: INestApplication): INestApplication { + app.useGlobalPipes(new ValidationPipe()); + + return app; +} diff --git a/src/main.ts b/src/main.ts index 9a7a7a2..b4edf4f 100644 --- a/src/main.ts +++ b/src/main.ts @@ -1,7 +1,8 @@ import { NestFactory } from '@nestjs/core'; -import { ConsoleLogger, ValidationPipe } from '@nestjs/common'; +import { ConsoleLogger } from '@nestjs/common'; import { DocumentBuilder, SwaggerModule } from '@nestjs/swagger'; import { AppModule } from './app.module.js'; +import { configureApp } from './app.setup.js'; async function bootstrap() { const app = await NestFactory.create(AppModule, { @@ -14,7 +15,8 @@ async function bootstrap() { flattenParams: true, }), }); - app.useGlobalPipes(new ValidationPipe()); + // Compartida con los tests e2e, para que prueben la misma app que se despliega. + configureApp(app); const config = new DocumentBuilder() .setTitle('Curso DevOps API') diff --git a/src/products/products.controller.spec.ts b/src/products/products.controller.spec.ts index 33aa6c6..594340e 100644 --- a/src/products/products.controller.spec.ts +++ b/src/products/products.controller.spec.ts @@ -1,14 +1,34 @@ import { Test, TestingModule } from '@nestjs/testing'; import { ProductsController } from './products.controller.js'; import { ProductsService } from './products.service.js'; +import { Product } from './entities/product.entity.js'; describe('ProductsController', () => { let controller: ProductsController; + let service: Record>; + const product: Product = { + id: 2, + name: 'Mouse inalámbrico', + description: 'Sensor óptico 1600 DPI', + price: 18000, + stock: 30, + }; + + // El servicio va mockeado: lo que importa acá es que el controlador delegue con los + // argumentos correctos, no la lógica del CRUD, que ya cubre products.service.spec.ts. beforeEach(async () => { + service = { + create: vi.fn(), + findAll: vi.fn(), + findOne: vi.fn(), + update: vi.fn(), + remove: vi.fn(), + }; + const module: TestingModule = await Test.createTestingModule({ controllers: [ProductsController], - providers: [ProductsService], + providers: [{ provide: ProductsService, useValue: service }], }).compile(); controller = module.get(ProductsController); @@ -17,4 +37,55 @@ describe('ProductsController', () => { it('should be defined', () => { expect(controller).toBeDefined(); }); + + describe('create', () => { + it('should pass the body through and return the created product', () => { + const dto = { name: 'Webcam', description: 'HD 1080p', price: 32000, stock: 12 }; + service.create.mockReturnValue(product); + + expect(controller.create(dto)).toBe(product); + expect(service.create).toHaveBeenCalledWith(dto); + }); + }); + + describe('findAll', () => { + it('should return the list from the service', () => { + service.findAll.mockReturnValue([product]); + + expect(controller.findAll()).toEqual([product]); + }); + }); + + // El id llega como string desde la URL y el servicio espera un number: la conversión + // es la única lógica propia del controlador, así que se verifica en cada ruta con :id. + describe('id conversion', () => { + it('should convert the id to a number on findOne', () => { + service.findOne.mockReturnValue(product); + + expect(controller.findOne('2')).toBe(product); + expect(service.findOne).toHaveBeenCalledWith(2); + }); + + it('should convert the id to a number on update', () => { + const dto = { price: 19000 }; + service.update.mockReturnValue(product); + + expect(controller.update('2', dto)).toBe(product); + expect(service.update).toHaveBeenCalledWith(2, dto); + }); + + it('should convert the id to a number on remove', () => { + controller.remove('2'); + + expect(service.remove).toHaveBeenCalledWith(2); + }); + + it('should pass a number, not a numeric string', () => { + controller.findOne('2'); + + const [id] = service.findOne.mock.calls[0]; + expect(id).toBe(2); + expect(typeof id).toBe('number'); + }); + }); }); diff --git a/src/products/products.service.spec.ts b/src/products/products.service.spec.ts index db7b409..951f56b 100644 --- a/src/products/products.service.spec.ts +++ b/src/products/products.service.spec.ts @@ -1,9 +1,13 @@ +import { NotFoundException } from '@nestjs/common'; import { Test, TestingModule } from '@nestjs/testing'; import { ProductsService } from './products.service.js'; describe('ProductsService', () => { let service: ProductsService; + // El servicio guarda los productos en memoria. Test.createTestingModule construye + // una instancia nueva en cada test, así que todos arrancan con los datos semilla + // intactos y no hay que limpiar estado entre uno y otro. beforeEach(async () => { const module: TestingModule = await Test.createTestingModule({ providers: [ProductsService], @@ -15,4 +19,92 @@ describe('ProductsService', () => { it('should be defined', () => { expect(service).toBeDefined(); }); + + describe('findAll', () => { + it('should return every seeded product', () => { + const products = service.findAll(); + + expect(products).toHaveLength(3); + expect(products.map((p) => p.id)).toEqual([1, 2, 3]); + }); + }); + + describe('findOne', () => { + it('should return the product matching the id', () => { + expect(service.findOne(2)).toMatchObject({ id: 2, name: 'Mouse inalámbrico' }); + }); + + it('should throw NotFoundException when the id does not exist', () => { + expect(() => service.findOne(999)).toThrow(NotFoundException); + }); + + it('should name the missing id in the error message', () => { + expect(() => service.findOne(999)).toThrow('Product #999 not found'); + }); + }); + + describe('create', () => { + const dto = { name: 'Webcam', description: 'HD 1080p', price: 32000, stock: 12 }; + + it('should return the new product with a generated id', () => { + expect(service.create(dto)).toEqual({ id: 4, ...dto }); + }); + + it('should make the new product retrievable', () => { + const created = service.create(dto); + + expect(service.findAll()).toHaveLength(4); + expect(service.findOne(created.id)).toMatchObject(dto); + }); + + it('should assign consecutive ids', () => { + expect(service.create(dto).id).toBe(4); + expect(service.create(dto).id).toBe(5); + }); + }); + + describe('update', () => { + it('should apply the given fields', () => { + expect(service.update(1, { price: 50000 }).price).toBe(50000); + }); + + it('should leave the fields that were not sent untouched', () => { + const before = { ...service.findOne(1) }; + + const updated = service.update(1, { price: 50000 }); + + expect(updated.name).toBe(before.name); + expect(updated.description).toBe(before.description); + expect(updated.stock).toBe(before.stock); + }); + + it('should persist the change', () => { + service.update(1, { stock: 0 }); + + expect(service.findOne(1).stock).toBe(0); + }); + + it('should throw NotFoundException when the id does not exist', () => { + expect(() => service.update(999, { price: 1 })).toThrow(NotFoundException); + }); + }); + + describe('remove', () => { + it('should delete the product', () => { + service.remove(1); + + expect(service.findAll()).toHaveLength(2); + expect(() => service.findOne(1)).toThrow(NotFoundException); + }); + + it('should leave the other products in place', () => { + service.remove(1); + + expect(service.findAll().map((p) => p.id)).toEqual([2, 3]); + }); + + it('should throw NotFoundException when the id does not exist', () => { + expect(() => service.remove(999)).toThrow(NotFoundException); + }); + }); }); diff --git a/test/app.e2e-spec.ts b/test/app.e2e-spec.ts index fd20981..1496a2e 100644 --- a/test/app.e2e-spec.ts +++ b/test/app.e2e-spec.ts @@ -1,11 +1,11 @@ import { Test, TestingModule } from '@nestjs/testing'; import { INestApplication } from '@nestjs/common'; import request from 'supertest'; -import { App } from 'supertest/types'; import { AppModule } from './../src/app.module.js'; +import { configureApp } from './../src/app.setup.js'; describe('AppController (e2e)', () => { - let app: INestApplication; + let app: INestApplication; beforeEach(async () => { const moduleFixture: TestingModule = await Test.createTestingModule({ @@ -13,6 +13,8 @@ describe('AppController (e2e)', () => { }).compile(); app = moduleFixture.createNestApplication(); + // Sin esto la app de test no lleva el ValidationPipe y difiere de la desplegada. + configureApp(app); await app.init(); }); diff --git a/test/products.e2e-spec.ts b/test/products.e2e-spec.ts new file mode 100644 index 0000000..dc2f223 --- /dev/null +++ b/test/products.e2e-spec.ts @@ -0,0 +1,157 @@ +import { Test, TestingModule } from '@nestjs/testing'; +import { INestApplication } from '@nestjs/common'; +import request from 'supertest'; +import { AppModule } from './../src/app.module.js'; +import { configureApp } from './../src/app.setup.js'; + +describe('ProductsController (e2e)', () => { + let app: INestApplication; + + const validProduct = { + name: 'Webcam', + description: 'HD 1080p con micrófono', + price: 32000, + stock: 12, + }; + + // Una app nueva por test: el servicio guarda los productos en memoria, así que cada + // caso arranca con los datos semilla y no hereda lo que hicieron los anteriores. + beforeEach(async () => { + const moduleFixture: TestingModule = await Test.createTestingModule({ + imports: [AppModule], + }).compile(); + + app = moduleFixture.createNestApplication(); + configureApp(app); + await app.init(); + }); + + afterEach(async () => { + await app.close(); + }); + + describe('GET /products', () => { + it('should return the seeded products', async () => { + const res = await request(app.getHttpServer()).get('/products').expect(200); + + expect(res.body).toHaveLength(3); + expect(res.body[0]).toMatchObject({ id: 1, name: 'Teclado mecánico' }); + }); + }); + + describe('GET /products/:id', () => { + it('should return the requested product', async () => { + const res = await request(app.getHttpServer()).get('/products/2').expect(200); + + expect(res.body).toMatchObject({ id: 2, name: 'Mouse inalámbrico' }); + }); + + it('should return 404 for an unknown id', async () => { + const res = await request(app.getHttpServer()).get('/products/999').expect(404); + + expect(res.body.message).toBe('Product #999 not found'); + }); + }); + + describe('POST /products', () => { + it('should create the product and return 201', async () => { + const res = await request(app.getHttpServer()) + .post('/products') + .send(validProduct) + .expect(201); + + expect(res.body).toMatchObject(validProduct); + expect(res.body.id).toBeGreaterThan(0); + }); + + // Este bloque es el que antes no se podía probar: sin el ValidationPipe la app de + // test aceptaba cualquier body y devolvía 201. + describe('validation', () => { + it('should reject a body with wrong types', async () => { + await request(app.getHttpServer()) + .post('/products') + .send({ name: 123, description: 'x', price: 'no-es-numero', stock: -5 }) + .expect(400); + }); + + it('should reject a body with missing fields', async () => { + await request(app.getHttpServer()).post('/products').send({ name: 'Solo nombre' }).expect(400); + }); + + it('should reject a negative price', async () => { + await request(app.getHttpServer()) + .post('/products') + .send({ ...validProduct, price: -1 }) + .expect(400); + }); + + it('should report which field failed', async () => { + const res = await request(app.getHttpServer()) + .post('/products') + .send({ ...validProduct, stock: -5 }) + .expect(400); + + expect(res.body.message.join(' ')).toContain('stock'); + }); + + it('should not persist a rejected product', async () => { + await request(app.getHttpServer()).post('/products').send({ name: 'Invalido' }).expect(400); + + const res = await request(app.getHttpServer()).get('/products').expect(200); + expect(res.body).toHaveLength(3); + }); + }); + }); + + describe('PATCH /products/:id', () => { + it('should apply a partial update', async () => { + const res = await request(app.getHttpServer()) + .patch('/products/1') + .send({ price: 50000 }) + .expect(200); + + expect(res.body).toMatchObject({ id: 1, price: 50000, name: 'Teclado mecánico' }); + }); + + it('should return 404 for an unknown id', async () => { + await request(app.getHttpServer()).patch('/products/999').send({ price: 1 }).expect(404); + }); + + it('should validate the body', async () => { + await request(app.getHttpServer()).patch('/products/1').send({ price: 'gratis' }).expect(400); + }); + }); + + describe('DELETE /products/:id', () => { + it('should remove the product', async () => { + await request(app.getHttpServer()).delete('/products/1').expect(200); + + await request(app.getHttpServer()).get('/products/1').expect(404); + }); + + it('should return 404 for an unknown id', async () => { + await request(app.getHttpServer()).delete('/products/999').expect(404); + }); + }); + + describe('full lifecycle', () => { + it('should create, read, update and delete a product', async () => { + const created = await request(app.getHttpServer()) + .post('/products') + .send(validProduct) + .expect(201); + const { id } = created.body; + + await request(app.getHttpServer()).get(`/products/${id}`).expect(200); + + const updated = await request(app.getHttpServer()) + .patch(`/products/${id}`) + .send({ stock: 0 }) + .expect(200); + expect(updated.body.stock).toBe(0); + + await request(app.getHttpServer()).delete(`/products/${id}`).expect(200); + await request(app.getHttpServer()).get(`/products/${id}`).expect(404); + }); + }); +}); diff --git a/vitest.config.ts b/vitest.config.ts index 9384f47..15549b1 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -9,5 +9,18 @@ export default defineConfig({ globals: true, root: './', include: ['**/*.spec.ts'], + coverage: { + // Sin `include`, la cobertura solo mide los archivos que algún test importa: + // los que no tienen test quedan fuera del denominador y el porcentaje sale + // inflado. Midiendo todo src/ el número refleja el proyecto real. + include: ['src/**/*.ts'], + exclude: [ + 'src/**/*.spec.ts', + // Bootstrap: arranca el servidor y no tiene lógica propia que testear. + 'src/main.ts', + // Módulos: solo declaran controllers y providers, sin lógica. + 'src/**/*.module.ts', + ], + }, }, });