Repository navigation
Testing: tests E2E 99% - #147
Merged
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughRecorridoEste PR agrega nueve archivos de pruebas E2E que ejercitan integralmente endpoints relacionados con entregas, asignaciones, reseñas y órdenes. Las pruebas mockean Prisma, JWT y servicios para validar lógica de autenticación, autorización, validación de entrada y comportamiento transaccional sin depender de una base de datos real. CambiosSuites de pruebas E2E para entregas y órdenes
Estimado de esfuerzo de revisión🎯 3 (Moderado) | ⏱️ ~25 minutos PRs posiblemente relacionados
Revisores sugeridos
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
tests/e2e/orders-available-deliveries.test.js (1)
31-32: ⚡ Quick winConstantes y mocks declarados pero nunca utilizados.
sellerCookie2(línea 32) se define pero ningún test lo referencia.mockOrderPickup(líneas 61-66) se define pero ningún test lo utiliza; hay un escenario de orden sin dirección (fk_address: null) que queda sin cobertura.mockDeliveryInactive(líneas 80-84) se define pero no se usa en ningún test (ver comentario anterior).Eliminar las constantes no utilizadas o cubrir los escenarios que representan.
Also applies to: 61-66, 80-84
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/orders-available-deliveries.test.js` around lines 31 - 32, Remove the unused constants or add tests that exercise them: either delete sellerCookie2, mockOrderPickup, and mockDeliveryInactive if they’re unnecessary, or use them in new/updated tests in tests/e2e/orders-available-deliveries.test.js — specifically add a test that authenticates with sellerCookie2 to cover a second-seller scenario, add a test that inserts/requests an order with fk_address: null using mockOrderPickup to cover pickup-addressless orders, and add a test that uses mockDeliveryInactive to verify inactive deliveries are excluded; update any setup/teardown helpers used by the existing GET /orders/available-deliveries flow so these mocks are applied.tests/e2e/orders-delivery-reviews.test.js (1)
206-230: ⚡ Quick winEl error
P2002se mockea como objeto plano en lugar de una instancia deError, inconsistente condelivery.test.jsEn
delivery.test.js(líneas 265-267) se construye correctamente el error de Prisma como:const prismaError = new Error("..."); prismaError.code = "P2002";Acá se usa un objeto plano
{ code: "P2002", meta: {...} }. Si el middleware de manejo de errores accede a.message,.stack, o verificainstanceof Error, la simulación no representará fielmente un error real de Prisma.♻️ Usar instancia de Error consistente con el resto del PR
- prisma.deliveryReviews.create.mockRejectedValue({ - code: "P2002", - meta: { target: ["fk_order"] }, - }); + const prismaError = new Error("Unique constraint failed on the fields: (`fk_order`)"); + prismaError.code = "P2002"; + prismaError.meta = { target: ["fk_order"] }; + prisma.deliveryReviews.create.mockRejectedValue(prismaError);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/orders-delivery-reviews.test.js` around lines 206 - 230, The test mocks prisma.deliveryReviews.create rejecting with a plain object instead of an Error instance; change the mockRejectedValue to throw an Error instance (e.g., const prismaError = new Error("..."); prismaError.code = "P2002"; prismaError.meta = { target: ["fk_order"] };) and use that with prisma.deliveryReviews.create.mockRejectedValue(prismaError) so the error behaves consistently with delivery.test.js and any middleware checks like .message or instanceof Error.tests/e2e/delivery-register.test.js (2)
34-46: ⚡ Quick winFalta un test para el caso sin autenticación (401)
El mock JWT en las líneas 19-30 ya implementa el retorno de 401 cuando no hay cookie (
if (!req.cookies?.userToken)), pero ningún test ejercita ese camino. Agregar este caso completa la cobertura del middleware.✅ Caso de prueba sugerido
+ it("retorna 401 cuando no hay autenticación", async () => { + const res = await request(app) + .post("/api/deliveries/register") + .send({ vehicleType: "CAR" }); + + expect(res.status).toBe(401); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/delivery-register.test.js` around lines 34 - 46, Add a new test in the describe("POST /api/deliveries/register") suite to exercise the unauthenticated path: send a POST to "/api/deliveries/register" without setting the authCookie (so the JWT mock branch if (!req.cookies?.userToken) is triggered) and assert that the response status is 401 and the body contains the expected error code/message for unauthorized access; place the test next to the existing 400 case so the middleware branch covered by the JWT mock (lines around the mock) is exercised.
6-17: ⚡ Quick winEl mock de
$transactionno contempla la forma de callback, a diferencia del resto de los archivos del PRTodos los demás archivos de este PR (
delivery-assignments-orders-history.test.js,delivery-assignments-response.test.js,orders-delivery-reviews.test.js) manejan ambas formas de$transaction. Este archivo solo maneja el array form: si el servicio usaprisma.$transaction(async (tx) => {...}), pasar la función aPromise.alllanza unTypeErrorporque las funciones no son iterables.♻️ Alinear con el patrón del resto del PR
- $transaction: vi.fn((operations) => Promise.all(operations)), + $transaction: vi.fn((callbackOrArray) => { + if (typeof callbackOrArray === "function") { + return callbackOrArray({ + users: prisma.users, + deliveries: prisma.deliveries, + }); + } + return Promise.all(callbackOrArray); + }),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/delivery-register.test.js` around lines 6 - 17, El mock de $transaction en el bloque vi.mock solo soporta arreglo y falla si el código llama prisma.$transaction(fn); cambia la implementación de $transaction (en el mock dentro de delivery-register.test.js) para detectar si el argumento es una función y, en ese caso, invocarla pasándole el objeto de mock de prisma (o un proxy con las mismas propiedades usadas en tests) y devolver su resultado (o Promise.resolve si devuelve síncrono); si el argumento no es función, seguir usando Promise.all(operations) como ahora. Asegúrate de que la referencia al objeto mock (ej. prisma, models como users/deliveries) se use al invocar la función para mantener paridad con los otros tests que ya manejan ambas formas.tests/e2e/delivery-profile.test.js (1)
27-35: ⚡ Quick win
adminTokense crea pero nunca se usa; falta cobertura para acceso no autenticado
adminToken(línea 35) se genera pero ningún test lo consume. El comportamiento esperado para el rolADMINantePUT /api/deliveries/:idqueda sin validar.Además, no hay ningún caso que verifique la respuesta ante una request sin cookie/token (esperaría un
401), lo cual es un camino de error relevante para la autenticación.✅ Tests sugeridos a agregar
it("Retorna 401 cuando no hay token de autenticación", async () => { const res = await request(app) .put("/api/deliveries/1") .send({ name: "Sin Auth" }); expect(res.status).toBe(401); }); it("Retorna 403 cuando el rol es ADMIN", async () => { const res = await request(app) .put("/api/deliveries/1") .set("Cookie", authCookie(adminToken)) .send({ name: "Intento Admin" }); expect(res.status).toBe(403); });Also applies to: 172-179
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/delivery-profile.test.js` around lines 27 - 35, Add tests covering unauthenticated and ADMIN-role access for the PUT /api/deliveries/:id endpoint: keep the existing adminToken in beforeAll and add one test that issues request(app).put("/api/deliveries/1").send(...) with no Cookie and asserts a 401 response, and another test that issues request(app).put("/api/deliveries/1").set("Cookie", authCookie(adminToken)).send(...) and asserts a 403 response; locate where tests reference request, app, authCookie, and adminToken in delivery-profile.test.js and add these two it(...) cases near the existing delivery/seller tests so adminToken is used and unauthenticated behavior is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/e2e/deliveries-list-and-disengagement.test.js`:
- Around line 118-141: The tests in deliveries-list-and-disengagement.test.js
assert error text against res.body.message but the global errorHandler formats
errors as { error: { code, message } }; update the two failing assertions in the
DELETE tests ("DELETE - retorna 400 si tiene assignments activos
(PENDING/ACCEPTED)" and "DELETE - 404 si el delivery no pertenece a la tienda")
to check res.body.error.message instead of res.body.message so they match the
middleware output (leave the status assertions as-is).
In `@tests/e2e/delivery-assignments-orders-history.test.js`:
- Around line 147-164: The test uses mockAssignments (and then builds
filteredAssignments) but all items in mockAssignments already have
assignment_status "DELIVERED", so the test never verifies exclusion of
non-matching items; update the fixture mockAssignments to include at least one
item with a different assignment_status (e.g., "PENDING"), keep
prisma.deliveryAssignments.findMany.mockResolvedValue(filteredAssignments) to
return only the matching items, and adjust the assertions in the test
(expect(res.body.content).toHaveLength(... ) and the delivered-every check) to
reflect the actual filtered count so the test will fail if the server does not
filter by assignment_status; reference variables: mockAssignments,
filteredAssignments, prisma.deliveryAssignments.findMany, and the test case
it("retorna 200 con pedidos filtrados por estado DELIVERED"...).
In `@tests/e2e/delivery-profile.test.js`:
- Line 17: El mock de $transaction está pasando objetos anónimos en lugar de las
mismas referencias a prisma.users.update y prisma.deliveries.update, por lo que
las aserciones top-level son falsas positivas; fix: create shared mock
references (e.g. mockTxUsers and mockTxDeliveries) and return those same
references from the vi.mock factory for $transaction so tx.users.update /
tx.deliveries.update call the same spies as
prisma.users.update/prisma.deliveries.update, and because vi.mock factories are
hoisted define those spies with vi.hoisted(...) so they are available inside the
factory; update the "Body vacío" test to use these shared mocks when asserting
not.toHaveBeenCalled().
- Around line 22-24: The tests use vi.resetAllMocks() which removes the runtime
mock implementation of upsertUserImage (defined via
vi.fn().mockResolvedValue(...)) causing false positives; replace
vi.resetAllMocks() with vi.clearAllMocks() so mocks keep their factory
implementations, or if you must reset implementations, reassign the
upsertUserImage mock inside the beforeEach (recreate
vi.mock/vi.fn(...).mockResolvedValue(...)) so the upsertUserImage used by the
code under test always returns the expected URL; update the hooks around the
delivery-profile.test.js cases (and the other occurrences noted) to use
vi.clearAllMocks() or reconfigure upsertUserImage in beforeEach accordingly.
In `@tests/e2e/orders-available-deliveries.test.js`:
- Around line 138-141: The current test only asserts that
res.body.available_deliveries is an array, which is insufficient; update the
assertions in tests/e2e/orders-available-deliveries.test.js to validate the
actual filtering behavior by asserting the array is empty
(expect(res.body.available_deliveries.length).toBe(0)) or by asserting none of
the items have status 'PENDING' or 'ACCEPTED' (e.g.,
expect(res.body.available_deliveries.every(d =>
!['PENDING','ACCEPTED'].includes(d.status))).toBe(true)); replace or augment the
existing expect(Array.isArray(...)) check so the test will fail if the service
does not filter out PENDING/ACCEPTED deliveries.
- Around line 143-162: The test "retorna 200 cuando filtra correctamente
deliveries ACTIVE y estan disponibles" currently only returns ACTIVE mocks and
uses a fragile id check; update prisma.deliveries.findMany to include
mockDeliveryInactive alongside mockDeliveryActive1 and mockDeliveryActive2, then
assert that the response excludes the inactive delivery (e.g., verify no item in
res.body.available_deliveries has id_delivery ===
mockDeliveryInactive.id_delivery or has status === 'INACTIVE') and replace the
id-based toBeLessThanOrEqual(2) assertion with an assertion that every returned
delivery has status 'ACTIVE' (or equivalent domain property) to validate the
filtering logic implemented by the service.
---
Nitpick comments:
In `@tests/e2e/delivery-profile.test.js`:
- Around line 27-35: Add tests covering unauthenticated and ADMIN-role access
for the PUT /api/deliveries/:id endpoint: keep the existing adminToken in
beforeAll and add one test that issues
request(app).put("/api/deliveries/1").send(...) with no Cookie and asserts a 401
response, and another test that issues
request(app).put("/api/deliveries/1").set("Cookie",
authCookie(adminToken)).send(...) and asserts a 403 response; locate where tests
reference request, app, authCookie, and adminToken in delivery-profile.test.js
and add these two it(...) cases near the existing delivery/seller tests so
adminToken is used and unauthenticated behavior is covered.
In `@tests/e2e/delivery-register.test.js`:
- Around line 34-46: Add a new test in the describe("POST
/api/deliveries/register") suite to exercise the unauthenticated path: send a
POST to "/api/deliveries/register" without setting the authCookie (so the JWT
mock branch if (!req.cookies?.userToken) is triggered) and assert that the
response status is 401 and the body contains the expected error code/message for
unauthorized access; place the test next to the existing 400 case so the
middleware branch covered by the JWT mock (lines around the mock) is exercised.
- Around line 6-17: El mock de $transaction en el bloque vi.mock solo soporta
arreglo y falla si el código llama prisma.$transaction(fn); cambia la
implementación de $transaction (en el mock dentro de delivery-register.test.js)
para detectar si el argumento es una función y, en ese caso, invocarla pasándole
el objeto de mock de prisma (o un proxy con las mismas propiedades usadas en
tests) y devolver su resultado (o Promise.resolve si devuelve síncrono); si el
argumento no es función, seguir usando Promise.all(operations) como ahora.
Asegúrate de que la referencia al objeto mock (ej. prisma, models como
users/deliveries) se use al invocar la función para mantener paridad con los
otros tests que ya manejan ambas formas.
In `@tests/e2e/orders-available-deliveries.test.js`:
- Around line 31-32: Remove the unused constants or add tests that exercise
them: either delete sellerCookie2, mockOrderPickup, and mockDeliveryInactive if
they’re unnecessary, or use them in new/updated tests in
tests/e2e/orders-available-deliveries.test.js — specifically add a test that
authenticates with sellerCookie2 to cover a second-seller scenario, add a test
that inserts/requests an order with fk_address: null using mockOrderPickup to
cover pickup-addressless orders, and add a test that uses mockDeliveryInactive
to verify inactive deliveries are excluded; update any setup/teardown helpers
used by the existing GET /orders/available-deliveries flow so these mocks are
applied.
In `@tests/e2e/orders-delivery-reviews.test.js`:
- Around line 206-230: The test mocks prisma.deliveryReviews.create rejecting
with a plain object instead of an Error instance; change the mockRejectedValue
to throw an Error instance (e.g., const prismaError = new Error("...");
prismaError.code = "P2002"; prismaError.meta = { target: ["fk_order"] };) and
use that with prisma.deliveryReviews.create.mockRejectedValue(prismaError) so
the error behaves consistently with delivery.test.js and any middleware checks
like .message or instanceof Error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 51ae9c85-a7a5-4510-84ce-062fb9b44a7a
📒 Files selected for processing (10)
tests/e2e/deliveries-list-and-disengagement.test.jstests/e2e/delivery-assignments-orders-history.test.jstests/e2e/delivery-assignments-response.test.jstests/e2e/delivery-profile.test.jstests/e2e/delivery-register.test.jstests/e2e/delivery-reviews.test.jstests/e2e/delivery-status.test.jstests/e2e/delivery.test.jstests/e2e/orders-available-deliveries.test.jstests/e2e/orders-delivery-reviews.test.js
Comment on lines
+118
to
+141
| it("DELETE - retorna 400 si tiene assignments activos (PENDING/ACCEPTED)", async () => { | ||
| prisma.stores.findUnique.mockResolvedValue(mockStore); | ||
| deliveriesTxMock.findFirst.mockResolvedValue({ id_delivery: 10, delivery_assignments: [{ id_delivery_assignment: 1 }] }); | ||
|
|
||
| const res = await request(app).delete("/api/stores/1/deliveries/10").set("Cookie", sellerCookie); | ||
| expect(res.status).toBe(400); | ||
| expect(res.body.message).toMatch(/entregas activas/i); | ||
| }); | ||
|
|
||
| it("DELETE - 403 si el requester no es dueño del comercio", async () => { | ||
| prisma.stores.findUnique.mockResolvedValue(mockStoreOtherOwner); | ||
|
|
||
| const res = await request(app).delete("/api/stores/1/deliveries/10").set("Cookie", sellerCookie); | ||
| expect(res.status).toBe(403); | ||
| }); | ||
|
|
||
| it("DELETE - 404 si el delivery no pertenece a la tienda", async () => { | ||
| prisma.stores.findUnique.mockResolvedValue(mockStore); | ||
| deliveriesTxMock.findFirst.mockResolvedValue(null); | ||
|
|
||
| const res = await request(app).delete("/api/stores/1/deliveries/999").set("Cookie", sellerCookie); | ||
| expect(res.status).toBe(404); | ||
| expect(res.body.message).toMatch(/delivery no encontrado/i); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Buscar el handler o controller del DELETE /stores/:id/deliveries/:deliveryId
rg -n --type=js -C5 'fk_store.*null|entregas activas|delivery no encontrado' --iglob '!tests/**'Repository: CrisNAC/BackendMarketplace
Length of output: 15382
🏁 Script executed:
rg -n "ValidationError|NotFoundError" --type=js -A2 --iglob '!tests/**' | head -50Repository: CrisNAC/BackendMarketplace
Length of output: 3795
🏁 Script executed:
# Find error middleware or error handler
rg -n "express.*error|app\.use.*error|Error.*handler" --type=js --iglob '!tests/**' | head -20Repository: CrisNAC/BackendMarketplace
Length of output: 108
🏁 Script executed:
# Look for response formatting in error handlers
rg -n "res\.status.*json|res\.json.*error|res\.json.*message" --type=js -B2 -A2 --iglob '!tests/**' | grep -A2 -B2 "error\|message" | head -40Repository: CrisNAC/BackendMarketplace
Length of output: 2420
🏁 Script executed:
# Find the error class definitions
cat src/lib/errors.jsRepository: CrisNAC/BackendMarketplace
Length of output: 1421
🏁 Script executed:
# View the complete errorHandler middleware
cat src/middlewares/errorHandler.jsRepository: CrisNAC/BackendMarketplace
Length of output: 2213
Las aserciones de error del DELETE usan res.body.message pero el middleware devuelve res.body.error.message
El middleware global errorHandler.js formatea todos los AppError (incluyendo ValidationError y NotFoundError) con estructura { error: { code, message } }. Las aserciones en L124 y L140 acceden directamente a res.body.message, lo que causará que fallen porque la propiedad no existe. Deben cambiar a res.body.error.message para coincidir con la respuesta real del servidor.
Cambios requeridos
it("DELETE - retorna 400 si tiene assignments activos (PENDING/ACCEPTED)", async () => {
prisma.stores.findUnique.mockResolvedValue(mockStore);
deliveriesTxMock.findFirst.mockResolvedValue({ id_delivery: 10, delivery_assignments: [{ id_delivery_assignment: 1 }] });
const res = await request(app).delete("/api/stores/1/deliveries/10").set("Cookie", sellerCookie);
expect(res.status).toBe(400);
- expect(res.body.message).toMatch(/entregas activas/i);
+ expect(res.body.error.message).toMatch(/entregas activas/i);
});
it("DELETE - 404 si el delivery no pertenece a la tienda", async () => {
prisma.stores.findUnique.mockResolvedValue(mockStore);
deliveriesTxMock.findFirst.mockResolvedValue(null);
const res = await request(app).delete("/api/stores/1/deliveries/999").set("Cookie", sellerCookie);
expect(res.status).toBe(404);
- expect(res.body.message).toMatch(/delivery no encontrado/i);
+ expect(res.body.error.message).toMatch(/delivery no encontrado/i);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it("DELETE - retorna 400 si tiene assignments activos (PENDING/ACCEPTED)", async () => { | |
| prisma.stores.findUnique.mockResolvedValue(mockStore); | |
| deliveriesTxMock.findFirst.mockResolvedValue({ id_delivery: 10, delivery_assignments: [{ id_delivery_assignment: 1 }] }); | |
| const res = await request(app).delete("/api/stores/1/deliveries/10").set("Cookie", sellerCookie); | |
| expect(res.status).toBe(400); | |
| expect(res.body.message).toMatch(/entregas activas/i); | |
| }); | |
| it("DELETE - 403 si el requester no es dueño del comercio", async () => { | |
| prisma.stores.findUnique.mockResolvedValue(mockStoreOtherOwner); | |
| const res = await request(app).delete("/api/stores/1/deliveries/10").set("Cookie", sellerCookie); | |
| expect(res.status).toBe(403); | |
| }); | |
| it("DELETE - 404 si el delivery no pertenece a la tienda", async () => { | |
| prisma.stores.findUnique.mockResolvedValue(mockStore); | |
| deliveriesTxMock.findFirst.mockResolvedValue(null); | |
| const res = await request(app).delete("/api/stores/1/deliveries/999").set("Cookie", sellerCookie); | |
| expect(res.status).toBe(404); | |
| expect(res.body.message).toMatch(/delivery no encontrado/i); | |
| }); | |
| it("DELETE - retorna 400 si tiene assignments activos (PENDING/ACCEPTED)", async () => { | |
| prisma.stores.findUnique.mockResolvedValue(mockStore); | |
| deliveriesTxMock.findFirst.mockResolvedValue({ id_delivery: 10, delivery_assignments: [{ id_delivery_assignment: 1 }] }); | |
| const res = await request(app).delete("/api/stores/1/deliveries/10").set("Cookie", sellerCookie); | |
| expect(res.status).toBe(400); | |
| expect(res.body.error.message).toMatch(/entregas activas/i); | |
| }); | |
| it("DELETE - 403 si el requester no es dueño del comercio", async () => { | |
| prisma.stores.findUnique.mockResolvedValue(mockStoreOtherOwner); | |
| const res = await request(app).delete("/api/stores/1/deliveries/10").set("Cookie", sellerCookie); | |
| expect(res.status).toBe(403); | |
| }); | |
| it("DELETE - 404 si el delivery no pertenece a la tienda", async () => { | |
| prisma.stores.findUnique.mockResolvedValue(mockStore); | |
| deliveriesTxMock.findFirst.mockResolvedValue(null); | |
| const res = await request(app).delete("/api/stores/1/deliveries/999").set("Cookie", sellerCookie); | |
| expect(res.status).toBe(404); | |
| expect(res.body.error.message).toMatch(/delivery no encontrado/i); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/e2e/deliveries-list-and-disengagement.test.js` around lines 118 - 141,
The tests in deliveries-list-and-disengagement.test.js assert error text against
res.body.message but the global errorHandler formats errors as { error: { code,
message } }; update the two failing assertions in the DELETE tests ("DELETE -
retorna 400 si tiene assignments activos (PENDING/ACCEPTED)" and "DELETE - 404
si el delivery no pertenece a la tienda") to check res.body.error.message
instead of res.body.message so they match the middleware output (leave the
status assertions as-is).
Comment on lines
+147
to
+164
| it("retorna 200 con pedidos filtrados por estado DELIVERED", async () => { | ||
| prisma.deliveries.findUnique.mockResolvedValue(mockDelivery); | ||
| const filteredAssignments = mockAssignments.filter( | ||
| (a) => a.assignment_status === "DELIVERED" | ||
| ); | ||
| prisma.deliveryAssignments.findMany.mockResolvedValue(filteredAssignments); | ||
| prisma.deliveryAssignments.count.mockResolvedValue(filteredAssignments.length); | ||
|
|
||
| const res = await request(app) | ||
| .get("/api/assignments/1/orders?assignment_status=DELIVERED") | ||
| .set("Cookie", authCookie) | ||
| .expect(200); | ||
|
|
||
| expect(res.body.content).toHaveLength(2); | ||
| expect(res.body.content.every((a) => a.assignment_status === "DELIVERED")).toBe( | ||
| true | ||
| ); | ||
| }); |
Contributor
There was a problem hiding this comment.
El test de filtro por DELIVERED no valida que ítems no coincidentes sean excluidos
Todos los elementos de mockAssignments ya tienen assignment_status: "DELIVERED", por lo que filteredAssignments siempre tiene longitud 2. El test esperaría toHaveLength(2) inclusive si el filtro del servidor estuviera completamente roto.
Para que el test tenga valor real, el fixture debería incluir al menos un elemento con un estado diferente (p.ej. "PENDING") y el mock debería devolver solo los que coincidan.
🛠 Fixture con ítems de estado mixto
const mockAssignments = [
{
id_delivery_assignment: 1,
assignment_status: "DELIVERED",
// ...
},
{
id_delivery_assignment: 2,
- assignment_status: "DELIVERED",
+ assignment_status: "PENDING", // ítem que NO debería aparecer en el filtro
// ...
},
];
it("retorna 200 con pedidos filtrados por estado DELIVERED", async () => {
prisma.deliveries.findUnique.mockResolvedValue(mockDelivery);
const filteredAssignments = mockAssignments.filter(
(a) => a.assignment_status === "DELIVERED"
);
prisma.deliveryAssignments.findMany.mockResolvedValue(filteredAssignments);
prisma.deliveryAssignments.count.mockResolvedValue(filteredAssignments.length);
// ...
- expect(res.body.content).toHaveLength(2);
+ expect(res.body.content).toHaveLength(1); // Solo el ítem DELIVERED🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/e2e/delivery-assignments-orders-history.test.js` around lines 147 -
164, The test uses mockAssignments (and then builds filteredAssignments) but all
items in mockAssignments already have assignment_status "DELIVERED", so the test
never verifies exclusion of non-matching items; update the fixture
mockAssignments to include at least one item with a different assignment_status
(e.g., "PENDING"), keep
prisma.deliveryAssignments.findMany.mockResolvedValue(filteredAssignments) to
return only the matching items, and adjust the assertions in the test
(expect(res.body.content).toHaveLength(... ) and the delivered-every check) to
reflect the actual filtered count so the test will fail if the server does not
filter by assignment_status; reference variables: mockAssignments,
filteredAssignments, prisma.deliveryAssignments.findMany, and the test case
it("retorna 200 con pedidos filtrados por estado DELIVERED"...).
| update: vi.fn(), | ||
| findUnique: vi.fn(), | ||
| }, | ||
| $transaction: vi.fn((callback) => callback({ users: { update: vi.fn() }, deliveries: { update: vi.fn() } })), |
Contributor
There was a problem hiding this comment.
Los mocks de $transaction son instancias anónimas desconectadas de prisma.users.update y prisma.deliveries.update
El callback de $transaction recibe { users: { update: vi.fn() }, deliveries: { update: vi.fn() } } — son instancias anónimas y efímeras, distintas de prisma.users.update y prisma.deliveries.update importadas en el test.
Si el controlador hace:
await prisma.$transaction(async (tx) => {
await tx.users.update(...);
await tx.deliveries.update(...);
});Las llamadas a tx.users.update y tx.deliveries.update van a las instancias anónimas del callback, nunca a prisma.users.update y prisma.deliveries.update.
Las aserciones del test "Body vacío" (líneas 168–169) son vacuamente verdaderas independientemente de lo que haga el servicio: aunque la implementación llame a los updates dentro de la transacción, los mocks top-level nunca se invocan y el not.toHaveBeenCalled() siempre pasa.
🛠️ Corrección propuesta: compartir las mismas referencias de mocks
+const mockTxUsers = { update: vi.fn() };
+const mockTxDeliveries = { update: vi.fn() };
+
vi.mock("../../src/lib/prisma.js", () => {
const mockPrisma = {
deliveries: {
findUnique: vi.fn(),
- update: vi.fn(),
+ update: mockTxDeliveries.update,
},
users: {
update: vi.fn(),
+ update: mockTxUsers.update,
findUnique: vi.fn(),
},
- $transaction: vi.fn((callback) => callback({ users: { update: vi.fn() }, deliveries: { update: vi.fn() } })),
+ $transaction: vi.fn((callback) => callback({ users: mockTxUsers, deliveries: mockTxDeliveries })),
};
return { prisma: mockPrisma };
});Luego en el test de "Body vacío":
- expect(prisma.users.update).not.toHaveBeenCalled();
- expect(prisma.deliveries.update).not.toHaveBeenCalled();
+ expect(mockTxUsers.update).not.toHaveBeenCalled();
+ expect(mockTxDeliveries.update).not.toHaveBeenCalled();Nota:
vi.mockhoistea el factory antes que los imports, por lo que las variablesmockTxUsers/mockTxDeliveriesdeben definirse convi.hoisted(...)para ser accesibles dentro del factory.
Also applies to: 156-170
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/e2e/delivery-profile.test.js` at line 17, El mock de $transaction está
pasando objetos anónimos en lugar de las mismas referencias a
prisma.users.update y prisma.deliveries.update, por lo que las aserciones
top-level son falsas positivas; fix: create shared mock references (e.g.
mockTxUsers and mockTxDeliveries) and return those same references from the
vi.mock factory for $transaction so tx.users.update / tx.deliveries.update call
the same spies as prisma.users.update/prisma.deliveries.update, and because
vi.mock factories are hoisted define those spies with vi.hoisted(...) so they
are available inside the factory; update the "Body vacío" test to use these
shared mocks when asserting not.toHaveBeenCalled().
Comment on lines
+22
to
+24
| vi.mock("../../src/modules/images/services/user-image.service.js", () => ({ | ||
| upsertUserImage: vi.fn().mockResolvedValue("https://storage.example.com/avatar.jpg"), | ||
| })); |
Contributor
There was a problem hiding this comment.
vi.resetAllMocks() elimina la implementación por defecto de upsertUserImage, generando falsos positivos en el test de avatar
resetAllMocks reemplaza las implementaciones de los mocks con funciones vacías — es una trampa frecuente porque destruye implementaciones que dependen del factory de vi.mock.
upsertUserImage se define como vi.fn().mockResolvedValue("..."). En Vitest 4, mockReset() solo restaura la implementación original pasada a vi.fn(impl). Como vi.fn() no tiene impl original, .mockResolvedValue(...) (agregado en runtime) se pierde.
Después de cada beforeEach, upsertUserImage retorna undefined. El test de líneas 119–139 igual pasa porque:
expect(upsertUserImage).toHaveBeenCalled()→ la función fue invocada (aunque devuelvaundefined)expect(res.body.user).toHaveProperty("avatar_url", "https://...")→ se satisface gracias al segundo mock defindUnique, no por lo que realmente retornóupsertUserImage
Esto es un falso positivo: el servicio recibe undefined de upsertUserImage pero el test da verde.
🛠️ Corrección propuesta
Opción A — cambiar a vi.clearAllMocks() (solo limpia el historial de llamadas, no destruye implementaciones):
- beforeEach(() => vi.resetAllMocks());
+ beforeEach(() => vi.clearAllMocks());Opción B — si se necesita limpiar también las implementaciones en runtime, re-configurar upsertUserImage en beforeEach:
beforeEach(() => {
vi.resetAllMocks();
+ const { upsertUserImage } = await import("../../src/modules/images/services/user-image.service.js");
+ upsertUserImage.mockResolvedValue("https://storage.example.com/avatar.jpg");
});Dado que la opción B es asíncrona en un hook, la Opción A (vi.clearAllMocks()) es la más simple y directa.
Also applies to: 75-75, 119-139
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/e2e/delivery-profile.test.js` around lines 22 - 24, The tests use
vi.resetAllMocks() which removes the runtime mock implementation of
upsertUserImage (defined via vi.fn().mockResolvedValue(...)) causing false
positives; replace vi.resetAllMocks() with vi.clearAllMocks() so mocks keep
their factory implementations, or if you must reset implementations, reassign
the upsertUserImage mock inside the beforeEach (recreate
vi.mock/vi.fn(...).mockResolvedValue(...)) so the upsertUserImage used by the
code under test always returns the expected URL; update the hooks around the
delivery-profile.test.js cases (and the other occurrences noted) to use
vi.clearAllMocks() or reconfigure upsertUserImage in beforeEach accordingly.
Comment on lines
+138
to
+141
| expect(res.status).toBe(200); | ||
| // Si el servicio filtra correctamente, la lista debería estar vacía | ||
| expect(Array.isArray(res.body.available_deliveries)).toBe(true); | ||
| }); |
Contributor
There was a problem hiding this comment.
La aserción del test de filtrado es trivialmente verdadera y no valida el comportamiento esperado.
El bloque de assertions solo verifica que available_deliveries es un array, lo que siempre será verdadero independientemente de si el servicio filtra o no las asignaciones con estado PENDING/ACCEPTED. El propio comentario en línea dice "la lista debería estar vacía" pero nunca se afirma eso. El test pasará aunque el endpoint devuelva las 2 entregas sin filtrar.
✅ Corrección propuesta: agregar aserción específica sobre el resultado filtrado
expect(res.status).toBe(200);
- // Si el servicio filtra correctamente, la lista debería estar vacía
- expect(Array.isArray(res.body.available_deliveries)).toBe(true);
+ // Ambas entregas tienen asignaciones activas (PENDING/ACCEPTED), por lo que deben ser excluidas
+ expect(res.body.available_deliveries).toHaveLength(0);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| expect(res.status).toBe(200); | |
| // Si el servicio filtra correctamente, la lista debería estar vacía | |
| expect(Array.isArray(res.body.available_deliveries)).toBe(true); | |
| }); | |
| expect(res.status).toBe(200); | |
| // Ambas entregas tienen asignaciones activas (PENDING/ACCEPTED), por lo que deben ser excluidas | |
| expect(res.body.available_deliveries).toHaveLength(0); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/e2e/orders-available-deliveries.test.js` around lines 138 - 141, The
current test only asserts that res.body.available_deliveries is an array, which
is insufficient; update the assertions in
tests/e2e/orders-available-deliveries.test.js to validate the actual filtering
behavior by asserting the array is empty
(expect(res.body.available_deliveries.length).toBe(0)) or by asserting none of
the items have status 'PENDING' or 'ACCEPTED' (e.g.,
expect(res.body.available_deliveries.every(d =>
!['PENDING','ACCEPTED'].includes(d.status))).toBe(true)); replace or augment the
existing expect(Array.isArray(...)) check so the test will fail if the service
does not filter out PENDING/ACCEPTED deliveries.
Comment on lines
+143
to
+162
| // Excluir deliveries INACTIVE | ||
| it("retorna 200 cuando filtra correctamente deliveries ACTIVE y estan disponibles", async () => { | ||
| prisma.stores.findUnique.mockResolvedValue(mockStore1); | ||
| prisma.orders.findFirst.mockResolvedValue(mockOrder); | ||
| prisma.deliveryAssignments.findFirst.mockResolvedValue(null); | ||
| prisma.deliveryAssignments.findMany.mockResolvedValue([]); | ||
| // Se retorna solo deliveries ACTIVE | ||
| prisma.deliveries.findMany.mockResolvedValue([mockDeliveryActive1, mockDeliveryActive2]); | ||
|
|
||
| const res = await request(app) | ||
| .get("/api/stores/1/orders/5/deliveries") | ||
| .set("Cookie", sellerCookie1); | ||
|
|
||
| expect(res.status).toBe(200); | ||
| // Verificar que todos los retornados son ACTIVE | ||
| expect(res.body.available_deliveries).toHaveLength(2); | ||
| res.body.available_deliveries.forEach((delivery) => { | ||
| expect(delivery.id_delivery).toBeLessThanOrEqual(2); // IDs de los ACTIVE | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
El test que describe "Excluir deliveries INACTIVE" no verifica ese comportamiento, y su aserción es frágil.
El mock de deliveries.findMany solo retorna mockDeliveryActive1 y mockDeliveryActive2; nunca se incluye mockDeliveryInactive en el arreglo devuelto. En consecuencia, el test no verifica que el servicio filtra deliveries INACTIVE, sino únicamente que retorna lo que el mock ya provee. Adicionalmente, la aserción en la línea 160 (toBeLessThanOrEqual(2)) está atada a IDs de datos de mock y no refleja ninguna propiedad funcional real del negocio.
✅ Corrección propuesta: incluir entrega INACTIVE en el mock y validar que no aparece en la respuesta
- // Se retorna solo deliveries ACTIVE
- prisma.deliveries.findMany.mockResolvedValue([mockDeliveryActive1, mockDeliveryActive2]);
+ // El mock simula que Prisma ya filtra por ACTIVE (como haría la query real con WHERE delivery_status = 'ACTIVE')
+ // pero incluimos el escenario donde el servicio recibe ACTIVE + INACTIVE para asegurar el filtrado
+ prisma.deliveries.findMany.mockResolvedValue([mockDeliveryActive1, mockDeliveryActive2, mockDeliveryInactive]);
const res = await request(app)
.get("/api/stores/1/orders/5/deliveries")
.set("Cookie", sellerCookie1);
expect(res.status).toBe(200);
- // Verificar que todos los retornados son ACTIVE
- expect(res.body.available_deliveries).toHaveLength(2);
- res.body.available_deliveries.forEach((delivery) => {
- expect(delivery.id_delivery).toBeLessThanOrEqual(2); // IDs de los ACTIVE
- });
+ // Solo deben aparecer las entregas ACTIVE, no la INACTIVE
+ expect(res.body.available_deliveries).toHaveLength(2);
+ const returnedIds = res.body.available_deliveries.map((d) => d.id_delivery);
+ expect(returnedIds).not.toContain(mockDeliveryInactive.id_delivery);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/e2e/orders-available-deliveries.test.js` around lines 143 - 162, The
test "retorna 200 cuando filtra correctamente deliveries ACTIVE y estan
disponibles" currently only returns ACTIVE mocks and uses a fragile id check;
update prisma.deliveries.findMany to include mockDeliveryInactive alongside
mockDeliveryActive1 and mockDeliveryActive2, then assert that the response
excludes the inactive delivery (e.g., verify no item in
res.body.available_deliveries has id_delivery ===
mockDeliveryInactive.id_delivery or has status === 'INACTIVE') and replace the
id-based toBeLessThanOrEqual(2) assertion with an assertion that every returned
delivery has status 'ACTIVE' (or equivalent domain property) to validate the
filtering logic implemented by the service.
|
CrisNAC
approved these changes
May 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Tests