From b3c903fd36f46a8f6bc8307a297c604d7c0eea2f Mon Sep 17 00:00:00 2001 From: Danijel Martinek Date: Tue, 5 May 2026 16:04:07 +0200 Subject: [PATCH] fix(tests): address Task 4 code review feedback - Deprecate mockPayloadModule with throw guard (hoisting incompatible) - Replace `as never` with stubPayloadConfig in payload-articles test (consistency) - Tighten pages contract to use toHaveLength (exact assertions) - Header contract: define CONTRACT_HEADER_SEED, assert item count + order Reviewer: superpowers:code-reviewer (Task 4 of Plan 7). Co-Authored-By: Claude Sonnet 4.6 --- .../payload-articles.repository.test.ts | 6 ++-- .../src/payload/mock-payload-module.ts | 29 +++++++++++++---- .../pages-repository.contract.ts | 6 ++-- .../header-repository.contract.ts | 32 +++++++++++++++++-- .../mock-header.repository.test.ts | 4 +-- .../repositories/mock-header.repository.ts | 22 ++++++++----- .../payload-header.repository.test.ts | 13 ++------ 7 files changed, 76 insertions(+), 36 deletions(-) diff --git a/packages/blog/src/infrastructure/repositories/payload-articles.repository.test.ts b/packages/blog/src/infrastructure/repositories/payload-articles.repository.test.ts index 6d96158..86cfb3d 100644 --- a/packages/blog/src/infrastructure/repositories/payload-articles.repository.test.ts +++ b/packages/blog/src/infrastructure/repositories/payload-articles.repository.test.ts @@ -109,8 +109,6 @@ describe("PayloadArticlesRepository", () => { // ------------------------------------------------------------------------- describe("Payload doc → domain mapping", () => { - const mockConfig = {} as never; - beforeEach(() => { vi.clearAllMocks(); }); @@ -135,7 +133,7 @@ describe("PayloadArticlesRepository", () => { find: findMock, }); - const repo = new PayloadArticlesRepository(mockConfig); + const repo = new PayloadArticlesRepository(stubPayloadConfig); const result = await repo.getArticleBySlug("hello"); expect(findMock).toHaveBeenCalledWith({ @@ -157,7 +155,7 @@ describe("PayloadArticlesRepository", () => { find: vi.fn().mockResolvedValue({ docs: [] }), }); - const repo = new PayloadArticlesRepository(mockConfig); + const repo = new PayloadArticlesRepository(stubPayloadConfig); const result = await repo.getArticleBySlug("missing"); expect(result).toBeUndefined(); }); diff --git a/packages/core-testing/src/payload/mock-payload-module.ts b/packages/core-testing/src/payload/mock-payload-module.ts index 29cd20f..b017952 100644 --- a/packages/core-testing/src/payload/mock-payload-module.ts +++ b/packages/core-testing/src/payload/mock-payload-module.ts @@ -1,10 +1,27 @@ -import { vi } from "vitest"; import type { Payload } from "payload"; -// Helper to mock the `payload` package with a custom getPayload impl. -// Call inside a test file BEFORE importing the SUT. +/** + * @deprecated DO NOT USE — hoisting incompatible. + * + * Vitest statically hoists `vi.mock()` calls to the top of the file. + * This helper wraps `vi.mock()` inside a regular function body, so the + * hoist never fires — the mock is installed too late and the real + * `payload` module loads before being intercepted. + * + * Instead, write at the TOP LEVEL of your test file: + * + * vi.mock("payload", () => ({ getPayload: vi.fn() })); + * + * // then in your test: + * const { getPayload } = await import("payload"); + * (getPayload as ReturnType).mockResolvedValue(yourStub); + * + * This export is preserved only so existing imports don't break and + * agents searching for it find this warning. + */ export function mockPayloadModule(impl: Partial): void { - vi.mock("payload", () => ({ - getPayload: vi.fn().mockResolvedValue(impl), - })); + void impl; + throw new Error( + "mockPayloadModule is deprecated and broken; see JSDoc. Call vi.mock('payload', ...) at the top of your test file instead.", + ); } diff --git a/packages/marketing-pages/src/__contracts__/pages-repository.contract.ts b/packages/marketing-pages/src/__contracts__/pages-repository.contract.ts index fd16715..f7bd7dc 100644 --- a/packages/marketing-pages/src/__contracts__/pages-repository.contract.ts +++ b/packages/marketing-pages/src/__contracts__/pages-repository.contract.ts @@ -72,7 +72,7 @@ export const pagesRepositoryContract = it("getPages with no filter returns all seeded pages", async () => { const list = await repo.getPages(); - expect(list.length).toBeGreaterThanOrEqual(2); + expect(list).toHaveLength(2); for (const page of list) { expect(page.id).toBeDefined(); expect(page.slug).toBeDefined(); @@ -82,7 +82,7 @@ export const pagesRepositoryContract = it("getPages({ status: 'published' }) returns only published pages", async () => { const published = await repo.getPages({ status: "published" }); - expect(published.length).toBeGreaterThanOrEqual(1); + expect(published).toHaveLength(1); for (const page of published) { expect(page.status).toBe("published"); } @@ -90,7 +90,7 @@ export const pagesRepositoryContract = it("getPages({ status: 'draft' }) returns only draft pages", async () => { const drafts = await repo.getPages({ status: "draft" }); - expect(drafts.length).toBeGreaterThanOrEqual(1); + expect(drafts).toHaveLength(1); for (const page of drafts) { expect(page.status).toBe("draft"); } diff --git a/packages/navigation/src/__contracts__/header-repository.contract.ts b/packages/navigation/src/__contracts__/header-repository.contract.ts index 431dd5e..62d20c6 100644 --- a/packages/navigation/src/__contracts__/header-repository.contract.ts +++ b/packages/navigation/src/__contracts__/header-repository.contract.ts @@ -1,12 +1,26 @@ import { it, expect, beforeEach } from "vitest"; import { defineContractSuite } from "@repo/core-testing/contract"; import type { IHeaderRepository } from "../application/repositories/header-repository.interface.js"; +import type { Header } from "../entities/header.js"; + +/** + * Known fixtures that every implementation's `buildSubject` must pre-seed. + * Exported so that test files can pass them to `MockHeaderRepository` or the + * Payload stub without duplicating definitions. + */ +export const CONTRACT_HEADER_SEED: Header = { + items: [ + { label: "Home", href: "/", external: false }, + { label: "Blog", href: "/blog", external: false }, + { label: "Docs", href: "/docs", external: true }, + ], +}; /** * Contract for IHeaderRepository. * * Header is a singleton (Payload Global). The interface exposes only - * getHeader(). The contract verifies the shape of the return value. + * getHeader(). The contract verifies the shape, count, and order of items. */ export const headerRepositoryContract = defineContractSuite( @@ -20,10 +34,24 @@ export const headerRepositoryContract = // --- getHeader --- - it("getHeader returns an object with an items array", async () => { + it("getHeader returns an object with an items array of the seeded length", async () => { const header = await repo.getHeader(); expect(header).toBeDefined(); expect(header.items).toBeInstanceOf(Array); + expect(header.items).toHaveLength(3); + }); + + it("getHeader items appear in the seeded order with correct shape", async () => { + const header = await repo.getHeader(); + expect(header.items[0]?.label).toBe("Home"); + expect(header.items[0]?.href).toBe("/"); + expect(header.items[0]?.external).toBe(false); + expect(header.items[1]?.label).toBe("Blog"); + expect(header.items[1]?.href).toBe("/blog"); + expect(header.items[1]?.external).toBe(false); + expect(header.items[2]?.label).toBe("Docs"); + expect(header.items[2]?.href).toBe("/docs"); + expect(header.items[2]?.external).toBe(true); }); it("getHeader items have label, href, and external fields", async () => { diff --git a/packages/navigation/src/infrastructure/repositories/mock-header.repository.test.ts b/packages/navigation/src/infrastructure/repositories/mock-header.repository.test.ts index 1982e37..469f672 100644 --- a/packages/navigation/src/infrastructure/repositories/mock-header.repository.test.ts +++ b/packages/navigation/src/infrastructure/repositories/mock-header.repository.test.ts @@ -1,7 +1,7 @@ import { describe } from "vitest"; import { MockHeaderRepository } from "@/infrastructure/repositories/mock-header.repository"; -import { headerRepositoryContract } from "@/__contracts__/header-repository.contract"; +import { headerRepositoryContract, CONTRACT_HEADER_SEED } from "@/__contracts__/header-repository.contract"; describe("MockHeaderRepository", () => { - headerRepositoryContract.run(() => new MockHeaderRepository()); + headerRepositoryContract.run(() => new MockHeaderRepository(CONTRACT_HEADER_SEED)); }); diff --git a/packages/navigation/src/infrastructure/repositories/mock-header.repository.ts b/packages/navigation/src/infrastructure/repositories/mock-header.repository.ts index fa9ae04..7333a1a 100644 --- a/packages/navigation/src/infrastructure/repositories/mock-header.repository.ts +++ b/packages/navigation/src/infrastructure/repositories/mock-header.repository.ts @@ -2,17 +2,23 @@ import "reflect-metadata"; import { injectable } from "inversify"; import type { IHeaderRepository } from "../../application/repositories/header-repository.interface"; -import type { Header } from "../../entities/header"; +import type { Header, HeaderItem } from "../../entities/header"; + +const DEFAULT_ITEMS: HeaderItem[] = [ + { label: "Home", href: "/", external: false }, + { label: "Blog", href: "/blog", external: false }, + { label: "Docs", href: "/docs", external: true }, +]; @injectable() export class MockHeaderRepository implements IHeaderRepository { + private readonly data: Header; + + constructor(initialData?: Header) { + this.data = initialData ?? { items: DEFAULT_ITEMS }; + } + async getHeader(): Promise
{ - return { - items: [ - { label: "Home", href: "/", external: false }, - { label: "Blog", href: "/blog", external: false }, - { label: "About", href: "/about", external: false }, - ], - }; + return this.data; } } diff --git a/packages/navigation/src/infrastructure/repositories/payload-header.repository.test.ts b/packages/navigation/src/infrastructure/repositories/payload-header.repository.test.ts index 43e32f9..44a3412 100644 --- a/packages/navigation/src/infrastructure/repositories/payload-header.repository.test.ts +++ b/packages/navigation/src/infrastructure/repositories/payload-header.repository.test.ts @@ -1,6 +1,6 @@ import { describe, vi } from "vitest"; import { PayloadHeaderRepository } from "@/infrastructure/repositories/payload-header.repository"; -import { headerRepositoryContract } from "@/__contracts__/header-repository.contract"; +import { headerRepositoryContract, CONTRACT_HEADER_SEED } from "@/__contracts__/header-repository.contract"; import { stubPayloadConfig } from "@repo/core-testing/payload/stub-config"; // --------------------------------------------------------------------------- @@ -8,18 +8,9 @@ import { stubPayloadConfig } from "@repo/core-testing/payload/stub-config"; // --------------------------------------------------------------------------- function buildHeaderStub() { - const globalData = { - logo: null, - items: [ - { label: "Home", href: "/", external: false }, - { label: "Blog", href: "/blog", external: false }, - { label: "About", href: "/about", external: false }, - ], - }; - return { findGlobal: vi.fn(async () => { - return { ...globalData }; + return { ...CONTRACT_HEADER_SEED }; }), }; }