Add e2e tests and unit tests for product CRUD functionality

.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

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
 <!--
 Imágenes por agregar en docs/images/ (los nombres deben coincidir con las referencias del informe):
 - pipeline-release.png        corrida "Release" completa (jobs ci, docker, deploy) con todo en verde
-- pr-checks.png               Pull Request con los checks de CI (Lint y Unit tests)
+- pr-checks.png               Pull Request con los checks de CI (Lint, Unit tests y E2E tests)
 - branch-protection.png       configuración de protección de `main`
 - dockerhub-tags.png          lista de tags del repo en Docker Hub
 - dockerhub-tag-detalle.png   detalle de v0.1.0 (linux/amd64, tamaño)
@@ -174,7 +180,7 @@ Imágenes por agregar en docs/images/ (los nombres deben coincidir con las refer
 ```mermaid
 flowchart LR
     A["git push a rama de feature"] --> 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ó.

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);
+  });
+});

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;
+}

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')

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<keyof ProductsService, ReturnType<typeof vi.fn>>;
 
+  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>(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');
+    });
+  });
 });