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 <noreply@anthropic.com>
2026-08-18 15:31:20 +02:00
parent a7bf228ca8
commit a82332fa20
5 changed files with 327 additions and 4 deletions
@@ -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
@@ -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.