From a82332fa204c94ff1d0c350cb6016e8ecba7d384 Mon Sep 17 00:00:00 2001 From: Edwin van den Houdt Date: Tue, 18 Aug 2026 15:31:20 +0200 Subject: [PATCH] docs: ADR-0006 test-data builders, close out WP-70 Writes up the principle behind WP-70's three tracks ("build test data through the same door production code uses") as ADR-0006, with a decision table for which fixture idiom fits which test type. Updates the test-strategy skill (adds the Fixtures rule, fixes its stale pre-monorepo src/app/... worked-example paths) and the shared Storybook testing.mdx page to match. Closes WP-70 with the signatures/counts as actually shipped. Co-Authored-By: Claude Sonnet 5 --- .claude/skills/test-strategy/SKILL.md | 17 +- docs/project/backlog/README.md | 1 + .../backlog/WP-70-test-data-builders.md | 118 +++++++++++++ .../architecture/0006-test-data-builders.md | 164 ++++++++++++++++++ libs/shared/docs/testing.mdx | 31 +++- 5 files changed, 327 insertions(+), 4 deletions(-) create mode 100644 docs/project/backlog/WP-70-test-data-builders.md create mode 100644 docs/reference/architecture/0006-test-data-builders.md diff --git a/.claude/skills/test-strategy/SKILL.md b/.claude/skills/test-strategy/SKILL.md index 49faa6e..5427abb 100644 --- a/.claude/skills/test-strategy/SKILL.md +++ b/.claude/skills/test-strategy/SKILL.md @@ -22,6 +22,13 @@ No `TestBed` for domain. Never assert on user-facing copy. only for wiring axe can't see. - **Never assert on `$localize` copy.** It changes per locale/edit — assert on the `Result`, the value object, or the message id. +- **Fixtures go through the production door, never a hand-built literal** (ADR-0006). + Replay real `Msg`s through the real `reduce` (`given(reduce, initial)(...msgs)`) for a + state machine; `unwrapOk(parseX(raw))` for a value object; a type-state builder + (`Given.Concept().Submitted()...`) for a backend aggregate with an ordered lifecycle. + The one deliberate exception is a trust-boundary `parse*` spec, below — there the fixture + must be a raw, possibly-malformed literal, because the test's whole point is "what if this + shape is wrong." See ADR-0006's decision table for which idiom fits which test type. ## Skeleton @@ -54,9 +61,13 @@ describe('parseThing', () => { ## Worked examples -- `src/app/registratie/domain/value-objects/postcode.spec.ts` — parser style. -- `src/app/registratie/infrastructure/brp.adapter.spec.ts` — trust boundary (`null`/`{}`). -- `src/app/registratie/domain/registratie-wizard.machine.spec.ts` — pure reducer. +- `apps/ssp/src/app/registratie/domain/value-objects/postcode.spec.ts` — parser style. +- `apps/ssp/src/app/registratie/infrastructure/brp.adapter.spec.ts` — trust boundary (`null`/`{}`). +- `apps/ssp/src/app/registratie/domain/registratie-wizard.machine.spec.ts` — pure reducer. +- `libs/shared/src/testing/{machine,remote-data,value-object}.ts` — the shared fixture + helpers (ADR-0006); `apps/ssp/src/app/herregistratie/domain/intake.testing.ts` — a + per-context wrapper (`givenIntake = given(reduce, initial)`); `intake.acceptance.spec.ts` + — a full journey expressed as one replayed message sequence. ## Verify diff --git a/docs/project/backlog/README.md b/docs/project/backlog/README.md index 36795e5..200ac83 100644 --- a/docs/project/backlog/README.md +++ b/docs/project/backlog/README.md @@ -120,6 +120,7 @@ for its existing violations, so every WP ends green. | [WP-67](WP-67-monorepo-behandelportal.md) | Merge behandelportal into this repo as a monorepo | 11 · Behandelportal | done | | [WP-68](WP-68-ddd-aggregate-hardening.md) | Aggregate invariants + status modelling (architecture review) | 12 · DDD hardening | done | | [WP-69](WP-69-intake-scholing-threshold-enforcement.md) | Enforce the scholing threshold server-side | 12 · DDD hardening | todo | +| [WP-70](WP-70-test-data-builders.md) | Test-data builders: illegal fixtures unrepresentable (ADR-0006) | 12 · DDD hardening | done | Sequencing dependencies (stated in the WPs too): 01 before 10–15 (axe covers story churn); 03/04 before 05–09 (boundaries stop new violations during refactors); 06 before 07 (typed diff --git a/docs/project/backlog/WP-70-test-data-builders.md b/docs/project/backlog/WP-70-test-data-builders.md new file mode 100644 index 0000000..e69620b --- /dev/null +++ b/docs/project/backlog/WP-70-test-data-builders.md @@ -0,0 +1,118 @@ +# WP-70 — Test-data builders: illegal fixtures unrepresentable + +Status: done +Phase: 12 — DDD hardening + +## Why + +Decision #3 in `CLAUDE.md` — "make illegal states unrepresentable" — is honoured in +production code (`AanvraagStatus`'s private-ctor/factory shape, the FE's tagged-union +machines, branded value objects behind `parse*`) but **not** in the test suites that exercise +them. Every layer independently reinvented ad-hoc, hand-built fixtures that reach around the +production construction path: + +- Backend: `Aanvraag` is a mutable EF-backed bag with independent public setters. Its own + `StatusAt` dereferences `Referentie!` three times on the unstated assumption + "Submitted ⇒ Referentie != null" — a convention two test files (`RuleTests.cs`, + `OpenZaakZaakSourceTests.cs`) kept consistent by hand across eight inline fixtures. +- Frontend: no shared fixture helper existed anywhere. Every machine spec redefined its own + throwaway literal helper (`editing1/2/3`, `editingWith`), each hardcoding fields like + `errors: {}` — asserting against shapes the real reducer may never produce. +- E2E: the seeded BSN and a diploma id were copy-pasted across all three specs, coupled to + `SeedData.cs`'s exact shape by comment only. + +## Read first + +- ADR-0006 (`docs/reference/architecture/0006-test-data-builders.md`) — the principle and the + full decision table this WP implements. +- `CLAUDE.md` §"The decisions" #3, #5. +- `backend/src/BigRegister.Api/Data/ApplicationStore.cs` (`Aanvraag`, `StatusAt`). +- `backend/src/BigRegister.Api/Domain/Applications/AanvraagStatus.cs` — the exemplar this + WP's backend builder mirrors. +- `backend/src/BigRegister.Api/Domain/Beoordeling/BeoordelingRules.cs`. + +## Decisions (pre-made, don't relitigate) + +1. **No `With*`-per-field builders anywhere.** A builder that opens every field back up is an + object literal with extra syntax — reject that shape on either side of the seam. +2. **Backend: a type-state builder.** `Given.Concept()` → `ConceptAanvraag` (only `.AtStep`/ + `.Submitted`/`.Build` exist) → `SubmittedAanvraag` (only `.Decided`/`.Build` exist) → + `DecidedAanvraag`. `Decided(...)` validates a toelichting by calling the real + `BeoordelingRules.RequiresToelichting`, not by re-stating the rule. +3. **`Aanvraag` itself stays mutable** — WP-68 deliberately kept it an EF-backed class; fixing + that for real is an EF-mapping refactor, out of scope here (see Follow-ups). +4. **Frontend: replay, don't fabricate.** One combinator, `given(reduce, initial)(...msgs)` + (`libs/shared/src/testing/machine.ts`), replaces every hand-written state literal. Value + objects: `unwrapOk(parseX(raw))`, never a cast. `RemoteData`: named constructors + (`loading()`/`success(v)`/`failure(e)`), replacing duplicated per-file literals. +5. **E2E stays a flat smoke suite** (WP-19's scope). Only extract shared `Actors`/`SeedRefs`/ + `loginAs` — no page-object layer, no Given/When/Then runner, no dev-only seeding API. + The shared-mutable-backend isolation problem is a documented follow-up, not fixed here. +6. **Convert worst offenders only**, not a full sweep: `RuleTests.cs`'s `Decided()` helper + + `OpenZaakZaakSourceTests.cs`'s seven inline initializers (backend); + `herregistratie.machine.spec.ts` + `change-request.machine.spec.ts` + both RemoteData + specs (frontend); all three e2e specs (actors/seed-refs only). + +## Files + +| Area | Path | +| ---------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------ | +| New (BE) | `backend/tests/BigRegister.Tests/Builders/AanvraagBuilder.cs`, `Acceptance/BesluitLifecycleTests.cs` | +| Edit (BE) | `RuleTests.cs`, `OpenZaakZaakSourceTests.cs` | +| New (FE) | `libs/shared/src/testing/{machine,remote-data,value-object}.ts`, `herregistratie/domain/intake.testing.ts`, `intake.acceptance.spec.ts` | +| Edit (FE) | `herregistratie.machine.spec.ts`, `change-request.machine.spec.ts`, `remote-data.spec.ts`, `machine-remote-data.spec.ts`, both `tsconfig.app.json`, `angular.json` | +| New (e2e) | `e2e/support/actors.ts` | +| Edit (e2e) | `smoke.spec.ts`, `brief-v2.spec.ts`, `error-state.spec.ts` | +| Docs | ADR-0006, `libs/shared/docs/testing.mdx`, `.claude/skills/test-strategy/SKILL.md`, this file + backlog README row | + +## Steps + +Executed as three file-disjoint parallel tracks (backend / frontend / e2e), each ending its +own layer's tests green, then a combined gate, then docs written up against the interfaces as +actually shipped. + +## Acceptance criteria + +- [x] `Given.Concept().Decided(...)` does not compile (proved live: temporarily inserted the + call, confirmed `dotnet build` fails with `CS1061`, reverted). +- [x] `Decided(Besluit.Afwijzen)`/`MeerInfoOpvragen` with no toelichting throws, via the real + `BeoordelingRules.RequiresToelichting`. +- [x] Backend tests: 220/220 passing (was 216 before; +4 from `BesluitLifecycleTests`). +- [x] Frontend: `npm test` green across all four projects (ssp/behandelportal/shared/beheer); + converted specs assert the same behaviour as before (diffed, not just re-passed) — + one case (`editing3`'s hardcoded `errors: {}` at step 3 with invalid punten) was + confirmed reachable via `SetField` after `Next`, not an unrepresentable state, so the + assertion carried over unchanged. +- [x] No fixture-only export (`givenIntake` etc.) leaks into a production bundle — confirmed + via `grep -rl` on `dist/` after both a plain and a `--localize` build. +- [x] `npm run ci` green (lint, format, tokens, both localized builds, audit, backend + format+test, snippet-drift, api-client-drift). +- [~] `npm run e2e` — refactor reviewed line-by-line (zero assertions changed), but not run to + completion in this environment: port 4200 was occupied by an unrelated container + (`team-monitor-web-1`), not this repo's stack. Confirm on a clean runner/CI before + relying on it; not a regression introduced by this WP. + +## Verification + +```bash +cd backend && dotnet format --verify-no-changes && dotnet test # 220/220 +npm run ci # green (2026-08-18) +npm run e2e # run on a clean port 4200 +``` + +## Out of scope + +- E2E test isolation (a dev-only seed endpoint) — the real fix for the shared-mutable-backend + problem; a new production-adjacent surface needing its own security review. +- Making `Aanvraag` itself illegal-states-unrepresentable (EF-mapping refactor). +- `RegistrationStatus`'s equivalent flat-record gap (`Domain/Registrations/`) — same class of + defect, separate WP. +- E2E coverage for `apps/behandelportal` (currently zero). + +## Risks + +- The backend type-state builder only guards the fields it models (`Submitted`, `Referentie`, + `SubmittedAt`, `BesluitStatus`, `BesluitToelichting`); other `Aanvraag` fields (e.g. + `ZaakUrl`) are still set post-`.Build()` directly, since `Aanvraag` remains mutable. A + future field added to the lifecycle needs a deliberate builder update, or it silently + reopens the same gap this WP closed. diff --git a/docs/reference/architecture/0006-test-data-builders.md b/docs/reference/architecture/0006-test-data-builders.md new file mode 100644 index 0000000..863817b --- /dev/null +++ b/docs/reference/architecture/0006-test-data-builders.md @@ -0,0 +1,164 @@ +# ADR-0006 — Test data through the production door (builders, replay, and where each applies) + +Status: Accepted · Date: 2026-08-18 + +## Context + +Decision #3 in `CLAUDE.md` is "make illegal states unrepresentable," and the production code +mostly honours it: `AanvraagStatus` (backend) is a `sealed class` with a private constructor +reachable only through five static factories; the frontend's wizards are tagged-union state +machines driven by a pure `reduce`; form inputs are branded value objects reachable only +through a `parse*` that returns `Result`. + +The test suites are the one place this invariant is not enforced — they build fixtures by +hand instead of through those same doors: + +- **Backend.** `Aanvraag` (`Data/ApplicationStore.cs`) is a mutable EF-backed bag: `Submitted`, + `Referentie`, `BesluitStatus`, `SubmittedAt` are independent public setters. Its own + `StatusAt` dereferences `Referentie!` three times — "Submitted ⇒ Referentie != null" is + convention, not type. Two test files (`RuleTests.cs`, `OpenZaakZaakSourceTests.cs`) kept + eight such fixtures internally consistent by hand, each re-deciding for itself which fields + a given scenario needs. +- **Frontend.** No shared fixture helper existed anywhere in `apps/` or `libs/`. Every spec + redefined its own throwaway literal function (`editing1/editing2/editing3`, `editingWith`, + a local `ok()`), each hardcoding fields like `errors: {}` — asserting against a shape the + real reducer may never actually produce, because the literal skips the reducer entirely. +- **E2E.** The one seeded citizen's BSN and a diploma id were duplicated as bare string + literals across every spec, coupled to `SeedData.cs`'s exact ordering by comment only, with + no compiler check if the seed ever changed shape. + +A hand-rolled literal is not "faster test setup" — it is a second, unchecked implementation +of the domain's construction rules, sitting right next to the real one. + +## Decision + +**Build test data through the same door production code uses. A test-data helper's job is to +supply _defaults_, never to bypass _invariants_.** + +Concretely: reject any test helper shaped as a field-by-field builder (`.withX().withY()...` +over an otherwise-open constructor) — that is an object literal with extra syntax, and it +re-opens every illegal state the production type closed. Each layer instead gets the +narrowest helper that **cannot** construct an illegal instance, because it has no path to one. + +### 1. Backend aggregates with a lifecycle → a type-state builder + +Where a production type enforces its invariants (or should), the test builder mirrors that +enforcement as separate **types per stage**, so an illegal call is a compile error, not a +runtime surprise: + +```csharp +Given.Concept() // ConceptAanvraag — only .Submitted() or .Build() exist + .Submitted() // SubmittedAanvraag — only .Decided() or .Build() exist + .Decided(Besluit.Afwijzen, "reden"); // DecidedAanvraag +``` + +`Given.Concept().Decided(...)` does not compile — `Decided` is simply not a member of +`ConceptAanvraag`. Where the production rule is more subtle than "which methods exist" +(e.g. "Afwijzen requires a toelichting"), the builder **calls the real production rule** +(`BeoordelingRules.RequiresToelichting`) rather than re-stating it — this is what keeps the +builder from drifting out of sync with the domain as the domain changes. + +Use this shape whenever a production aggregate has an ordered lifecycle and either (a) +already guards it with factories (mirror them 1:1), or (b) doesn't yet guard it (as with +`Aanvraag` itself, see Consequences) — the test-only builder is not a substitute for fixing +the production type, but it stops the test suite from being the place the ungated shape leaks +out into assertions. + +### 2. Frontend state machines → replay real messages through the real reducer + +No object is built directly. A fixture is the result of running real `Msg`s through the real +`reduce`: + +```ts +export const given = + (reduce: (s: S, m: M) => S, initial: S) => + (...msgs: M[]): S => + msgs.reduce(reduce, initial); + +export const givenIntake = given(reduce, initial); // per-context wrapper, pure TS +``` + +There is no way to hand-write a `Submitting` state whose draft contradicts its step, or to +assert `errors: {}` into existence — the only states reachable are the ones the reducer can +actually produce, because production is the only code path that produces them. + +### 3. Value objects → `unwrapOk`, never a cast + +A test that needs a valid branded value calls the real `parse*` and unwraps it: + +```ts +export const unwrapOk = (r: Result): T => { + if (!r.ok) throw new Error('unwrapOk: parser rejected the input'); + return r.value; +}; +const postcode = unwrapOk(parsePostcode('1234 AB')); +``` + +This closes the `'garbage' as Postcode` route — a spec can only ever hold a value the real +parser accepted. + +### 4. RemoteData → named constructors, not ad-hoc literals + +`loading()` / `success(v)` / `failure(e)` in `libs/shared/src/testing/remote-data.ts` replace +the per-spec local `ok()`/`loading`/`failure` literals. `RemoteData` has no invariant to +protect (it's a plain closed union with no smart constructor in production either), so this +one is about **removing duplication**, not closing an illegal-state gap — named constructors +belong here because they are shorter and consistent, not because the literal was unsafe. + +### 5. E2E — shared actors/seed-refs, not a DSL + +E2E fixtures are named, not built: `Actors.zorgverlener`, `SeedRefs.diplomaZonderPolicyVragen` +in `e2e/support/actors.ts`, with `loginAs(page, actor)` replacing the duplicated login +sequence. No page-object layer, no Given/When/Then runner — Playwright specs stay flat +`page.getByRole` sequences (matching WP-19's "smoke, not full coverage" scope), the only +change is that the values they use have one source instead of N copies. See "Where this does +**not** reach" below for why the deeper e2e problem is out of scope here. + +## Decision table — what to reach for, by test type + +| Test type | Where it lives | Fixture idiom | Do **not** | +| -------------------------------------------------------- | ------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -------------------------------------------------------------------------------------------- | +| Domain aggregate with a guarded lifecycle (backend) | `*.Tests/Builders/` | Type-state builder mirroring the production factories; delegate any non-trivial rule to the real rule class | A field-by-field `.WithX()` builder, or an object initializer with all fields public | +| Pure reducer / state machine (frontend) | `domain/*.testing.ts` | `given(reduce, initial)(...msgs)` — replay real messages | A literal returning `{ tag: 'Editing', ... }` by hand | +| Value object / parser | co-located with the parser's spec | `unwrapOk(parseX(raw))` | `'x' as BrandedType` | +| Plain closed union with no invariant (e.g. `RemoteData`) | `libs/shared/src/testing/` | Named one-line constructors (`loading()`, `success(v)`) | Redefining the same literal per spec file | +| Trust-boundary `parse*` (adapter) | co-located, per `test-strategy` skill | Hand-written DTO literals **are** correct here — the point of the test is "what if the untrusted shape is wrong," so the fixture must be a raw, possibly-malformed literal, not a validated domain value | Routing malformed-input tests through a builder that can't express malformed shapes | +| UI component | Storybook story + axe | Args as `input()`s on the component; no fixture builder needed | A component test with a hand-built store/model | +| Acceptance / behaviour test (either side) | `Acceptance/*Tests.cs` (backend), `*.acceptance.spec.ts` (frontend) | The same builder/replay idiom as above, composed into one Given→When→Then read | A separate BDD/Gherkin runner — the language's own test framework plus the builder is enough | +| E2E | `e2e/support/` | Named actor/seed-ref constants + a thin `loginAs`-style setup helper | A page-object framework or DSL — out of proportion to a 3-spec smoke suite | + +The common thread: **the fixture idiom is only ever a thinner or safer path to the same +construction the domain already performs** — never a parallel, unchecked one. The trust- +boundary row is the deliberate exception, not a contradiction: there the entire point of the +test is to exercise what happens when the input _isn't_ valid, so the fixture must be able to +represent the invalid shape a builder would refuse to construct. + +## Consequences + +- **+** An illegal backend fixture (e.g. a decided-but-not-submitted `Aanvraag`) is now a + compile error in the builder path, not a silent bad test. +- **+** Frontend specs can no longer assert against a state the reducer cannot actually reach; + a hardcoded `errors: {}` fixture literal can't drift from what validation actually produces. +- **+** One seeded identity/diploma reference in e2e instead of N copies — a reseed shows up as + one changed constant, not a hunt through three spec files. +- **−** `Aanvraag` itself is **not** made illegal-states-unrepresentable by this ADR — it + remains a mutable EF-backed class (WP-68 kept it that way deliberately; `ApplicationStore` + is its only production writer). The builder is a test-only enforcement layer sitting in + front of a production type that still allows the bad shape directly. Closing that gap for + real means an EF-mapping change, tracked as a follow-up, not done here. +- **−** A type-state builder is more ceremony than a constructor call for a one-off fixture. + Reach for it only where a lifecycle actually has ordered stages worth protecting — a flat + value type doesn't need one (see the `RemoteData` row above). + +## Where this does **not** reach (deliberately out of scope) + +- **E2E test isolation.** The three Playwright specs share one mutable backend and admit it in + their own comments ("restart the backend between CI runs"). The real fix is a dev-only seed + endpoint each test can call to build its own isolated citizen/aanvraag — a new + production-adjacent surface that needs its own security review, not a fixture-idiom change. + Tracked as a follow-up; not fixed here. +- **`RegistrationStatus`** (`Domain/Registrations/`) has the same class of gap as `Aanvraag` — + a flat record with four always-present nullable fields, whose own doc-comment says only one + tag ever uses the deadline field — but is out of this ADR's scope (a separate WP). +- **`apps/behandelportal` e2e coverage** is currently zero; adding it is a coverage gap, not a + fixture-idiom question, and is a separate follow-up. diff --git a/libs/shared/docs/testing.mdx b/libs/shared/docs/testing.mdx index cf6b871..6249f3b 100644 --- a/libs/shared/docs/testing.mdx +++ b/libs/shared/docs/testing.mdx @@ -63,9 +63,38 @@ expect(parseBrpAddress(null).ok).toBe(false); expect(parseBrpAddress({}).ok).toBe(false); // missing required field ``` -Elm-style machines test the pure `reduce` with inline state fixtures — no Angular +Elm-style machines test the pure `reduce` — no Angular (`registratie/domain/registratie-wizard.machine.spec.ts`). +## Fixtures: build test data through the production door + +A fixture is not a shortcut around the domain — it's the domain's own construction path, run +once for the test. [ADR-0006](../../../docs/reference/architecture/0006-test-data-builders.md) +covers this in full (with a backend example too); the frontend idiom is one combinator, +`given` (`libs/shared/src/testing/machine.ts`): + +```ts +export const given = + (reduce: (s: S, m: M) => S, initial: S) => + (...msgs: M[]): S => + msgs.reduce(reduce, initial); + +export const givenIntake = given(reduce, initial); // per-context wrapper, pure TS +``` + +A machine spec replays real `Msg`s instead of hand-writing a `State` literal — so a fixture +can only ever be a state the real reducer actually produces: + +```ts +const atStep3 = givenIntake(Start(), SetUren('1200'), Next(), SetDiplomaHerkomst('NL'), Next()); +``` + +The same rule extends to value objects (`unwrapOk(parseX(raw))` instead of a cast) and to +`RemoteData` (`loading()` / `success(v)` / `failure(e)` in +`libs/shared/src/testing/{value-object,remote-data}.ts` instead of a redefined-per-file +literal). **Never** a `.withX().withY()` builder over an open constructor — that just +re-opens whatever illegal state the domain closed. + ## UI = Storybook, not heavy component tests `@storybook/addon-a11y` runs the `wcag2a/2aa/21a/21aa` rule sets on **every** story;