docs(test): make Given/When/Then the default BDD structure (WP-71)

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>
This commit is contained in:
eho
2026-08-18 20:25:30 +02:00
co-authored by Claude Sonnet 5
parent 306d002221
commit 3652ff8d3f
9 changed files with 247 additions and 34 deletions
@@ -27,32 +27,49 @@ describe('statusLabel', () => {
describe('detailRows', () => {
it('lists soort/status/referentie/eigenaar/ingediend', () => {
// Given a case InBehandeling.
// When its detail rows are derived...
const rows = detailRows({
...base,
status: { tag: 'InBehandeling', referentie: 'R1', manual: false },
});
const values = rows.map((r) => r.value);
// Then the type label (via TYPE_LABELS, not a literal), the reference, and the
// owner all appear as rows.
expect(values).toContain(TYPE_LABELS.herregistratie);
expect(values).toContain('R1');
expect(values).toContain(base.owner);
expect(rows.length).toBe(5);
});
it('adds a reden row for Afgewezen and MeerInfoGevraagd only', () => {
// One behaviour ("a reden row is added exactly for the two statuses that carry a
// reden") checked as a truth table over three statuses — kept together per
// bdd.mdx's "truth-table of one rule" exception, rather than split apart.
//
// `reden` itself is raw domain data (a free-text field on the status union, not an
// enum), passed through `detailRows` unchanged and never wrapped by `$localize` —
// there is no reason-code/tag to assert on instead; the value under test IS the
// string the Given supplied, so checking it reappears in the Then is a
// pass-through check, not a translated-copy assertion.
it('a reden row is present only for Afgewezen and MeerInfoGevraagd', () => {
// Given three cases: rejected, more-info-requested, and approved.
// When their detail rows are derived...
const afgewezen = detailRows({
...base,
status: { tag: 'Afgewezen', referentie: 'R1', reden: 'Onvoldoende uren' },
});
expect(afgewezen.length).toBe(6);
expect(afgewezen.map((r) => r.value)).toContain('Onvoldoende uren');
const meerInfo = detailRows({
...base,
status: { tag: 'MeerInfoGevraagd', referentie: 'R1', reden: 'Diploma ontbreekt' },
});
expect(meerInfo.length).toBe(6);
const goedgekeurd = detailRows({ ...base, status: { tag: 'Goedgekeurd', referentie: 'R1' } });
// Then only the rejected and more-info-requested cases gain a reden row (carrying
// the reason through unchanged); the approved case does not.
expect(afgewezen.length).toBe(6);
expect(afgewezen.map((r) => r.value)).toContain('Onvoldoende uren');
expect(meerInfo.length).toBe(6);
expect(goedgekeurd.length).toBe(5);
});
});
@@ -31,7 +31,13 @@ describe('parseBeoordelingStatus', () => {
);
});
it('rejects a missing status, unknown tag, and wrong-typed fields', () => {
// One behaviour ("rejects a malformed status") checked over several malformed
// shapes — a loop asserting one rule over many inputs, kept together per bdd.mdx.
it('rejects a malformed status', () => {
// Given a status that is missing entirely, has an unknown tag, or is missing a
// required field for its tag.
// When each is parsed...
// Then all are rejected.
expect(parseBeoordelingStatus(undefined).ok).toBe(false);
expect(parseBeoordelingStatus({ tag: 'Concept' } as never).ok).toBe(false);
expect(parseBeoordelingStatus({ tag: 'InBehandeling', referentie: 'BIG-1' }).ok).toBe(false);
@@ -50,7 +56,13 @@ describe('parseBeoordelingView', () => {
expect(r.value.canBesluiten).toBe(true);
});
it('rejects a missing owner, bad type, missing decisions, and non-objects', () => {
// One behaviour ("rejects a malformed view") checked over several malformed
// shapes — a loop asserting one rule over many inputs, kept together per bdd.mdx.
it('rejects a malformed view', () => {
// Given a view that is a non-object, missing the owner, has an unknown aanvraag
// type, or is missing decisions.
// When each is parsed...
// Then all are rejected.
expect(parseBeoordelingView(null).ok).toBe(false);
expect(
parseBeoordelingView({ ...view, aanvraag: { ...view.aanvraag, owner: undefined } }).ok,
@@ -4,7 +4,7 @@ import { Result } from '@shared/kernel/fp';
import { Brief, BriefDecisions, CaseContext, LetterBlock } from '@brief/domain/brief';
import { OrgTemplate } from '@brief/domain/org-template';
import { BriefAdapter, BriefView } from '@brief/infrastructure/brief.adapter';
import { LetterPreviewAdapter } from '@brief/infrastructure/letter-preview.adapter';
import { LetterPreviewAdapter, PREVIEW_FAILED } from '@brief/infrastructure/letter-preview.adapter';
import { RevealBigNummerAdapter } from '@brief/infrastructure/reveal-bignummer.adapter';
import { BriefStore } from './brief.store';
@@ -164,25 +164,64 @@ async function loadedStore(over: Partial<BriefAdapter> = {}): Promise<BriefStore
}
describe('BriefStore undo/redo history', () => {
it('records an edit, undoes and redoes it; buttons mirror; a no-op edit is not recorded', async () => {
it('starts with nothing to undo', async () => {
// Given a freshly loaded brief.
// When no edit has happened yet...
const store = await loadedStore();
expect(store.canUndo()).toBe(false);
// Then there is nothing to undo.
expect(store.canUndo()).toBe(false);
});
it('records an edit and makes it undoable', async () => {
// Given a loaded brief with one block.
const store = await loadedStore();
// When a block is removed...
store.edit({ tag: 'BlockRemoved', blockId: 'local-1' });
// Then the block is gone and the edit becomes undoable.
expect(loadedBrief(store).sections[0].blocks.length).toBe(0);
expect(store.canUndo()).toBe(true);
});
it('undo reverts the edit and enables redo', async () => {
// Given a brief with one recorded edit (a removed block).
const store = await loadedStore();
store.edit({ tag: 'BlockRemoved', blockId: 'local-1' });
// When the edit is undone...
store.undo();
// Then the block is back, and redo becomes available.
expect(loadedBrief(store).sections[0].blocks.length).toBe(1);
expect(store.canRedo()).toBe(true);
});
it('redo reapplies the undone edit', async () => {
// Given an edit that was undone.
const store = await loadedStore();
store.edit({ tag: 'BlockRemoved', blockId: 'local-1' });
store.undo();
// When it is redone...
store.redo();
expect(loadedBrief(store).sections[0].blocks.length).toBe(0);
// A no-op edit (unknown block) changes nothing → leaves no dead history step.
// Then the edit is reapplied.
expect(loadedBrief(store).sections[0].blocks.length).toBe(0);
});
it('a no-op edit does not clear the redo future', async () => {
// Given an undone edit, with redo available.
const store = await loadedStore();
store.edit({ tag: 'BlockRemoved', blockId: 'local-1' });
store.undo(); // back to 1 block, redo available
// When an edit that changes nothing (an unknown block) is applied...
store.edit({ tag: 'BlockRemoved', blockId: 'does-not-exist' });
expect(store.canRedo()).toBe(true); // future NOT cleared by a no-op
// Then the no-op leaves no dead history step — redo is still available.
expect(store.canRedo()).toBe(true);
});
it('a new edit clears the redo future', async () => {
@@ -268,12 +307,12 @@ describe('BriefStore.previewLetter', () => {
const open = vi.spyOn(window, 'open').mockImplementation(() => null);
vi.spyOn(TestBed.inject(LetterPreviewAdapter), 'preview').mockResolvedValue({
ok: false,
error: 'De voorvertoning kon niet worden geopend.',
error: PREVIEW_FAILED,
});
await store.previewLetter();
expect(open).not.toHaveBeenCalled();
expect(store.lastError()).toBe('De voorvertoning kon niet worden geopend.');
expect(store.lastError()).toBe(PREVIEW_FAILED);
});
});
@@ -4,7 +4,9 @@ import { currentRole } from '@shared/infrastructure/role';
import { problemDetail } from '@shared/infrastructure/api-error';
import { environment } from '@shared/environments/environment';
const PREVIEW_FAILED = $localize`:@@brief.preview.failed:De voorvertoning kon niet worden geopend.`;
/** Exported so specs can assert against the same message id instead of retyping the
Dutch sentence (see `brief.store.spec.ts`'s `previewLetter` failure test). */
export const PREVIEW_FAILED = $localize`:@@brief.preview.failed:De voorvertoning kon niet worden geopend.`;
/**
* `/brief/preview` returns `text/html`, not JSON, and is `.ExcludeFromDescription()`'d
@@ -77,15 +77,20 @@ describe('intake acceptance journeys', () => {
});
});
it('buitenland gewerkt requires land + hours abroad, and gaNaarStap corrects an earlier answer', () => {
it('buitenland gewerkt requires land and hours abroad before advancing', () => {
// Given a user who says they worked abroad.
// When they try to advance without a country...
const blocked = givenIntake(
{ tag: 'SetAnswer', key: 'buitenlandGewerkt', value: 'ja' },
{ tag: 'Next' },
);
// Then they stay on the same step, blocked by a missing 'land' answer.
expect(blocked.tag).toBe('Answering');
expect(blocked.tag === 'Answering' && blocked.cursor).toBe(0);
expect(blocked.tag === 'Answering' && blocked.errors.land).toBeTruthy();
// When land and hours abroad are supplied, and the rest of the journey answered...
const reviewing = given(reduce, blocked)(
{ tag: 'SetAnswer', key: 'land', value: 'Duitsland' },
{ tag: 'SetAnswer', key: 'buitenlandseUren', value: '300' },
@@ -93,16 +98,34 @@ describe('intake acceptance journeys', () => {
{ tag: 'SetAnswer', key: 'uren', value: '1200' },
{ tag: 'Next' }, // werk step valid, uren high enough to skip scholing -> review
);
// Then the journey advances all the way to review.
expect(reviewing.tag).toBe('Answering');
expect(reviewing.tag === 'Answering' && reviewing.cursor).toBe(2); // review
});
// Jump back from review to correct the country, without losing later answers.
it('gaNaarStap corrects an earlier answer without losing later ones', () => {
// Given a journey that reached review with a foreign-work answer.
const reviewing = givenIntake(
{ tag: 'SetAnswer', key: 'buitenlandGewerkt', value: 'ja' },
{ tag: 'Next' },
{ tag: 'SetAnswer', key: 'land', value: 'Duitsland' },
{ tag: 'SetAnswer', key: 'buitenlandseUren', value: '300' },
{ tag: 'Next' },
{ tag: 'SetAnswer', key: 'uren', value: '1200' },
{ tag: 'Next' },
);
// When gaNaarStap jumps back to correct the country...
const corrected: IntakeState = given(reduce, reviewing)(
{ tag: 'GaNaarStap', cursor: 0 },
{ tag: 'SetAnswer', key: 'land', value: 'België' },
{ tag: 'Next' }, // buitenland step re-validated
{ tag: 'Next' }, // werk step re-validated (earlier 'uren' answer preserved)
);
// Then the correction lands back on review with the new answer, and the later
// 'uren' answer is preserved rather than lost.
expect(corrected.tag).toBe('Answering');
expect(corrected.tag === 'Answering' && corrected.cursor).toBe(2);
expect(corrected.tag === 'Answering' && corrected.answers.land).toBe('België');
@@ -111,6 +134,7 @@ describe('intake acceptance journeys', () => {
const noForwardJump = reduce(corrected, { tag: 'GaNaarStap', cursor: 2 });
expect(noForwardJump).toBe(corrected);
// And the corrected journey still submits successfully with the corrected data.
const done = given(reduce, corrected)({ tag: 'Submit' }, { tag: 'SubmitConfirmed' });
expect(done.tag).toBe('Submitted');
expect(done.tag === 'Submitted' && done.data).toEqual({
@@ -1,4 +1,5 @@
import { describe, it, expect } from 'vitest';
import { formatDatumNl } from '@shared/kernel/datum';
import { tasksFromProfile } from './tasks';
import { Registration } from './registration';
@@ -13,34 +14,56 @@ const base: Registration = {
describe('tasksFromProfile', () => {
it('offers herregistratie when the server says eligible, with the formatted deadline', () => {
// Given a registration whose deadline is 2026-12-31.
// When the server says the professional is eligible for herregistratie...
const tasks = tasksFromProfile(base, true);
// Then one task is offered, routed to herregistratie, whose copy carries the
// deadline through the same date formatter the rest of the app uses (this test's
// point IS the date formatting, so the expectation is derived from `formatDatumNl`
// rather than a hardcoded Dutch date literal).
expect(tasks).toHaveLength(1);
expect(tasks[0].to).toBe('/herregistratie');
expect(tasks[0].description).toContain('31 december 2026');
expect(tasks[0].description).toContain(formatDatumNl('2026-12-31'));
});
it('offers nothing when the server says not eligible', () => {
// Given the same registration.
// When the server says the professional is not eligible for herregistratie...
// Then no task is offered.
expect(tasksFromProfile(base, false)).toHaveLength(0);
});
it('surfaces a notice for a suspended registration (independent of eligibility)', () => {
// Given a suspended ("Geschorst") registration, ineligible for herregistratie.
const reg: Registration = {
...base,
status: { tag: 'Geschorst', geschorstTot: '2027-01-01', reden: 'Onderzoek' },
};
// When the tasks are derived...
const tasks = tasksFromProfile(reg, false);
// Then exactly one notice is surfaced, routed to the registration page, carrying
// the raw suspension reason through unchanged (not translated copy — `reden` is
// domain data, passed through as-is).
expect(tasks).toHaveLength(1);
expect(tasks[0].title).toContain('geschorst');
expect(tasks[0].to).toBe('/registratie');
expect(tasks[0].description).toBe('Onderzoek');
});
it('surfaces a notice for a struck-off registration', () => {
// Given a struck-off ("Doorgehaald") registration.
const reg: Registration = {
...base,
status: { tag: 'Doorgehaald', doorgehaaldOp: '2025-01-01', reden: 'Op eigen verzoek' },
};
// When the tasks are derived...
const tasks = tasksFromProfile(reg, false);
// Then exactly one notice is surfaced, routed to the registration page.
expect(tasks).toHaveLength(1);
expect(tasks[0].title).toContain('doorgehaald');
expect(tasks[0].to).toBe('/registratie');
});
});
@@ -245,20 +245,33 @@ public class BeoordelingTests(TestWebApplicationFactory factory) : IClassFixture
}
[Fact]
public async Task Unknown_id_404s_and_zorgverlener_is_forbidden()
public async Task Unknown_id_404s()
{
// Given no case exists with this id.
// When a besluit is posted against it...
var notFound = await PostBesluit("does-not-exist", new { besluit = "Goedkeuren" });
Assert.Equal(HttpStatusCode.NotFound, notFound.StatusCode);
// Then the endpoint answers 404, not a decision.
Assert.Equal(HttpStatusCode.NotFound, notFound.StatusCode);
}
[Fact]
public async Task Zorgverlener_is_forbidden_from_deciding()
{
// Given a decidable case.
var (a, _) = await CreateManualCaseWithDocument();
try
{
// When a zorgverlener (no X-Medewerker) posts a besluit against it...
var req = new HttpRequestMessage(HttpMethod.Post, $"/api/v1/beoordeling/{a.Id}/besluit")
{
Content = JsonContent.Create(new { besluit = "Goedkeuren" }),
};
req.Headers.Add("X-Role", "admin"); // zorgverlener, no X-Medewerker
Assert.Equal(HttpStatusCode.Forbidden, (await _client.SendAsync(req)).StatusCode);
var response = await _client.SendAsync(req);
// Then the request is forbidden — deciding is a behandelaar-only capability.
Assert.Equal(HttpStatusCode.Forbidden, response.StatusCode);
}
finally
{
@@ -133,6 +133,17 @@ boundary row is the deliberate exception, not a contradiction: there the entire
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
+81 -9
View File
@@ -12,22 +12,53 @@ test, by layer_); BDD owns _how each test is phrased and scoped_.
## Three rules
### 1. `describe` = the subject, `it` = one observable behaviour
### 1. `describe` = the subject, `it` = one observable behaviour, structured Given → When → Then
The `describe()` block names the unit under test; each `it()` states a single behaviour in
**declarative present tense** — the implicit subject is "it". No `should`, no
Given/When/Then ceremony: present-tense declaration already reads as a spec.
**declarative present tense** — the implicit subject is "it" (no `should`). Present-tense
naming and Given/When/Then structure are not in tension — the _title_ stays a declarative
one-liner; the _body_ is what's organised as Given → When → Then:
```ts
describe('parsePostcode', () => {
it('normalises to "1234 AB" (uppercase, single space, trimmed)', () => { … });
it('rejects malformed input', () => { … });
it('normalises to "1234 AB" (uppercase, single space, trimmed)', () => {
// Given a postcode with mixed case, extra whitespace, and no gap before the letters.
// When it is parsed...
const result = parsePostcode(' 1234ab ');
// Then it comes back normalised.
expect(result).toEqual(ok('1234 AB'));
});
it('rejects malformed input', () => {
// (no Given — the input itself IS the setup) When a non-postcode string is parsed...
// Then it is rejected.
expect(parsePostcode('nope').ok).toBe(false);
});
});
```
Read top-to-bottom it _is_ the spec: "parsePostcode — normalises to 1234 AB; rejects
malformed input."
**A genuinely empty phase is omitted, not faked with an empty comment.** The rejection test
above has no Given worth writing — the malformed literal passed to `parsePostcode` already
is the setup — so it degenerates straight to When/Then. Never write `// Given (nothing)` to
keep three comments lined up; an omitted phase is the correct, honest shape for a test that
doesn't need it. The three phases stay in order (Given before When before Then) whichever
of them are present.
**This reverses this doc's earlier advice** ("No … Given/When/Then ceremony") — the team
decided explicit G/W/T structure earns its keep as the default for every test, not just
acceptance tests. What doesn't change: no `should`, present-tense titles, one behaviour per
test, ubiquitous-language naming (rules 23 below).
**The Elm-machine naming style is a sanctioned form of rule-1 naming, not an exception to
it.** A store/reducer spec titled after the `Msg` tag it drives —
`it('BriefLoaded moves loading to loaded', …)` — names the domain event the same way the
reducer's own `switch (msg.tag)` does; the tag IS ubiquitous language for a state machine,
so this reads as a present-tense behaviour statement exactly like `'rejects malformed
input'` does, not as a violation of rule 3.
### 2. One behaviour per test
A test asserts **one behaviour**, not one `expect()`. Several assertions that pin down the
@@ -66,10 +97,51 @@ it('confirmed dutch proficiency requires taalvaardigheid proof', …);
richest specs; the wire boundary is tested as "rejects malformed input", the UI as
Storybook stories.
## C#/xUnit shape
The three rules above are language-agnostic; xUnit follows them with its own idiom rather
than Vitest's `describe`/`it` nesting:
- **The method name is the title, in `PascalCase_snake_sentence`** — the same present-tense,
ubiquitous-language behaviour statement as a `describe`+`it`, folded into one identifier
because xUnit has no nested-description syntax: `Only_open_statuses_are_decidable`,
`Afwijzen_requires_a_toelichting`, `A_terminal_besluit_is_frozen`.
- **`// Given` / `// When` / `// Then` comments mark the three phases inside the test body** —
the same structure as rule 1, made explicit because C# has no BDD framework layered on
xUnit here (see ADR-0006 — the language's own test framework plus the builder is enough,
deliberately not a Gherkin runner). As in TypeScript, an empty phase is omitted rather than
commented for its own sake.
- **Fixtures go through the `Given` type-state builder** (ADR-0006 §1), never a field-by-field
object initializer — keeping the Given phase itself honest about which states are
reachable.
```csharp
[Fact]
public void A_terminal_besluit_is_frozen()
{
// Given a case already decided Goedgekeurd — terminal, per BeoordelingRules.CanDecide.
var aanvraag = Given.Concept(type: "registratie").Submitted().Decided(Besluit.Goedkeuren).Build();
Persist(aanvraag);
// When a behandelaar tries to record a further besluit on it...
var (outcome, updated) = ApplicationStore.RecordBesluit(aanvraag.Id, Besluit.Afwijzen, "te laat", DateTimeOffset.UtcNow);
// Then the write is refused, and the original decision still stands.
Assert.Equal(ApplicationStore.RecordBesluitOutcome.Conflict, outcome);
Assert.Null(updated);
}
```
See `Acceptance/BesluitLifecycleTests.cs` for the canonical shape (it already does this) and
`AuthzTests.cs` for the truth-table naming convention this predates — a `[Theory]` row set
stays one behaviour (rule 2's "loop asserting one rule over many inputs"), so it doesn't need
per-row G/W/T comments, just one clear method name.
## Where to look
Canonical behaviour specs in the repo: `registratie/domain/value-objects/postcode.spec.ts`
(parser behaviour), `registratie/domain/registratie-wizard.machine.spec.ts` (one transition
per test), and backend `AuthzTests.cs` (rule truth-tables). The
[Testing strategy](?path=/docs/foundations-testing-strategy--docs) page maps which layer
gets which kind of test.
(parser behaviour), `registratie/domain/registratie-wizard.machine.spec.ts` (the
message-driven `describe` block — one reducer transition per test), and backend
`Acceptance/BesluitLifecycleTests.cs` (G/W/T-commented behaviour tests) and `AuthzTests.cs`
(rule truth-tables). The [Testing strategy](?path=/docs/foundations-testing-strategy--docs)
page maps which layer gets which kind of test.