ehoandClaude Sonnet 5
3652ff8d3f
bdd.mdx previously banned "Given/When/Then ceremony" outright, which
directly contradicted WP-70's own acceptance tests (Acceptance/
BesluitLifecycleTests.cs already used // Given/When/Then comments) and
the backend's organically-evolved PascalCase_snake_sentence convention,
which the doc gave zero guidance for. Reverses that rule: every test is
now structured Given -> When -> Then, with a genuinely empty phase
omitted rather than faked; present-tense declarative naming and the
one-behaviour-per-test rule are unchanged. ADR-0006 gets a cross-reference
so both documents agree everywhere, not just in acceptance tests.
Also closes out the doc's other named-but-unenforced rules found by the
audit: fixes the 5 files asserting rendered $localize copy instead of
the underlying tag/message-id (the compliant pattern already existed in
werkvoorraad-item-view.spec.ts), splits the multi-behaviour titles the
doc itself calls a smell (";", "and", "/"), and fixes bdd.mdx's own false
citation of registratie-wizard.machine.spec.ts as "one transition per
test" by actually splitting that test into one-transition-per-test.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
176 lines
13 KiB
Markdown
176 lines
13 KiB
Markdown
# 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 =
|
||
<S, M>(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 = <E, T>(r: Result<E, T>): 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.
|
||
|
||
## A note on Given/When/Then and `bdd.mdx`
|
||
|
||
This ADR's "Given.Concept()...Build()" builder chain and the acceptance-test row above
|
||
("composed into one Given→When→Then read") already used Given/When/Then before it was the
|
||
repo-wide default. `libs/shared/docs/bdd.mdx` has since made G/W/T structure — a `// Given` /
|
||
`// When` / `// Then` comment (or, in TypeScript, the equivalent unlabelled ordering) inside
|
||
every test body, acceptance or not — the documented convention for **all** tests, not only
|
||
acceptance ones (reversing its own earlier "no G/W/T ceremony" rule). The two documents now
|
||
agree everywhere: this ADR's `Given` builder is the fixture idiom; `bdd.mdx` rule 1 is the
|
||
structural convention every test using that fixture (and every other test besides) follows.
|
||
|
||
## 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.
|