Skip to content

Commit 4c2311e

Browse files
btraversbtravers
committed
docs(spec): design for a mock-free test architecture
Workstream 1 of the production-hardening effort. Establishes the rule "assert effects, never call shapes", the three-tier taxonomy, and the bundle-cache fixture architecture that makes migrating 77 tests to the real time-skipping server affordable. Grounded in measured state: 12 spec files mock the Temporal SDK across 241 tests, while worker unit coverage reports 93.7% statements — largely against a hand-written fake. PR #357's isThenable bug is the precedent for why that number is not trustworthy. Enforcement is a meta-test rather than a lint rule: the repo lints with oxlint only, which cannot express the check, and adding ESLint for a single rule is disproportionate.
1 parent d5ddb13 commit 4c2311e

1 file changed

Lines changed: 211 additions & 0 deletions

File tree

‎docs/superpowers/specs/2026-08-01-mock-free-test-architecture-design.md‎

Lines changed: 211 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,211 @@
1+
# Mock-free test architecture
2+
3+
**Date:** 2026-08-01
4+
**Status:** Approved
5+
**Scope:** Workstream 1 of 4 in the production-hardening effort
6+
7+
## Context
8+
9+
`temporal-contract` is in production use where real money depends on it. The
10+
hardening driver is **preventive** — no incident has occurred.
11+
12+
"Most robust as possible" decomposes into four independent workstreams:
13+
14+
1. **Mock-free test architecture** (this spec)
15+
2. Determinism & money-safety invariants
16+
3. API/type strength — making misuse unrepresentable
17+
4. Pattern enforcement — forcing correct usage on end users
18+
19+
Workstream 1 comes first because the other three are unverifiable without it:
20+
you cannot trust a hardening change when the suite that validates it asserts
21+
against a hand-written fake of the dependency being hardened.
22+
23+
### Measured starting state
24+
25+
| Signal | Value |
26+
| --------------------- | ------------------------------------------- |
27+
| Spec files | 38 |
28+
| Worker unit coverage | 93.7% stmts, **85.9% branch**, 89.3% funcs |
29+
| Files mocking the SDK | **12** (33 `vi.mock` calls, 262 `vi.fn`) |
30+
| Tests in mocked files | 241 |
31+
| Test tiers | `unit`, `integration` (Docker), `inprocess` |
32+
| Coverage thresholds | Configured, **not enforced** |
33+
| Mutation testing | None |
34+
35+
The 93.7% statement coverage is largely achieved _against a fake Temporal_.
36+
This is the failure mode the spec targets: the fake agrees with our
37+
assumptions, so the test passes while real Temporal behaves differently.
38+
39+
**Precedent.** The `isThenable` bug fixed in PR #357 was exactly this shape.
40+
Standard Schema _types_ the async branch as `Promise<Result>`, but an
41+
implementation may return any `PromiseLike`. Such a value slipped past
42+
`instanceof Promise`; the code then read `.issues` off it (`undefined` → "no
43+
issues, valid") and handed the handler an **unvalidated `undefined`**. It
44+
survived a 94%-covered suite because nothing exercised the real path.
45+
46+
## The rule
47+
48+
> **Assert effects, never call shapes.**
49+
>
50+
> A test MAY construct real SDK objects and fake pure transport.
51+
> A test MAY NOT fake SDK _semantics_. If the assertion is "we called Temporal
52+
> with X," it must be replaced by "the workflow behaved correctly."
53+
54+
Worked examples:
55+
56+
```ts
57+
// ALLOWED — real SDK object, faked transport.
58+
const failure = ApplicationFailure.create({ type: "CardDeclined" });
59+
fakeHandle.result = () => Promise.reject(failure);
60+
expect(classify(failure)).toBeErrWithTag(CONTRACT_ERROR_TAG);
61+
62+
// BANNED — call-shape assertion.
63+
vi.mock("@temporalio/workflow", () => ({ proxyActivities: capture }));
64+
expect(proxyCalls[0].startToCloseTimeout).toBe("30s");
65+
66+
// REPLACEMENT — effect assertion on a real server.
67+
// An activity that sleeps past its declared timeout must actually time out.
68+
await expect(run(wf)).resolves.toBeErrWithTag(TIMEOUT_TAG);
69+
```
70+
71+
The distinction is not "mock vs no mock" — it is **whether the test would
72+
catch a divergence from Temporal's real semantics.**
73+
74+
## Architecture
75+
76+
### Three tiers
77+
78+
| Tier | Runs on | Contains |
79+
| ------------- | ---------------------------- | ----------------------------------------------------------- |
80+
| `unit` | Nothing external | Pure logic; classification over **real** SDK error objects |
81+
| `inprocess` | Time-skipping test server | Anything SDK-semantic. **Default for new behavioral tests** |
82+
| `integration` | Docker (Temporal + Postgres) | Full-stack wiring, schedules, multi-worker |
83+
84+
`inprocess` becomes the default home for behavioral tests: it exercises real
85+
Temporal semantics without Docker, so it is both honest and cheap enough to
86+
run per-PR.
87+
88+
### Fixture architecture
89+
90+
The `testEnv` fixture in `packages/testing/src/time-skipping.ts` is already
91+
`{ scope: "worker" }`, so the `TestWorkflowEnvironment` is amortized across
92+
each Vitest worker. **The environment is not the problem.**
93+
94+
The unamortized cost is **workflow bundling**, currently paid inside every
95+
`TypedWorker.create({ workflowsPath })` call — i.e. once per test.
96+
97+
Add a module-level bundle cache to `@temporal-contract/testing`, keyed by
98+
`workflowsPath`:
99+
100+
```
101+
bundleFor(path): Map<string, Promise<{ code: string }>>
102+
→ bundleWorkflowCode() runs at most once per path per Vitest worker
103+
104+
per test:
105+
TypedWorker.create({ workflowBundle, taskQueue: `t-${uniqueId}` })
106+
```
107+
108+
Per-test isolation comes from a unique **task queue** and **workflow ID**,
109+
both nearly free. This preserves independence without re-bundling.
110+
111+
`TypedWorker.create` already forwards `workflowBundle` to Temporal's
112+
`Worker.create`; **no API change is required.**
113+
114+
**Constraint:** prebuilt bundles deliberately skip
115+
`verifyWorkflowRegistration` (see the option's JSDoc). The four
116+
`registration-*.workflows.ts` specs therefore MUST keep using `workflowsPath`
117+
— the registration check is their subject under test.
118+
119+
## Migration map
120+
121+
**77** tests across 6 worker files migrate, **18** split, **146** stay
122+
(77 + 18 + 146 = 241, the full population of mocked-file tests).
123+
124+
### Migrate — highest value first
125+
126+
Ordered by what a fake-vs-real divergence actually costs:
127+
128+
| File | Tests | Why it matters |
129+
| ------------------------- | ----- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
130+
| `handlers.spec.ts` | 28 | The update **validator slot** rejects pre-admission, before a history event is written. A mocked `setHandler` cannot reproduce this — it is the difference between a cleanly rejected update and a corrupted history. |
131+
| `cancellation.spec.ts` | 13 | Real `CancellationScope` propagation, including the swallowed-cancellation hazard where a declared-`errors` activity absorbs the cancel. |
132+
| `continue-as-new.spec.ts` | 10 | Real state carry-over. Losing state here loses money. |
133+
| `workflow-proxy.spec.ts` | 15 | Pure call-shape: asserts `proxyActivities` options rather than that an activity honors them. |
134+
| `wire-format.spec.ts` | 8 | Call-shape over `executeChild` / `startChild`. |
135+
| `workflow-errors.spec.ts` | 3 | Call-shape over `proxyActivities`. |
136+
137+
### Split
138+
139+
`worker.spec.ts` (18) — config validation and option merging are pure and stay
140+
in `unit`; registration-verification behavior moves to `inprocess`.
141+
142+
### Keep as-is
143+
144+
- `client.spec.ts` (108) and `schedule.spec.ts` (25) — these already satisfy
145+
the rule. They import **real** `ApplicationFailure`, `CancelledFailure`,
146+
`TimeoutFailure`, `TerminatedFailure` and fake only transport, so the
147+
error-classification logic is genuinely exercised against true shapes.
148+
- `packages/testing/*` (3 files, 13 tests) — mock `testcontainers` /
149+
`@temporalio/testing` to test this package's own wiring. Low money risk.
150+
151+
## Enforcement
152+
153+
### Mock guard
154+
155+
Fail on `vi.mock("@temporalio/*")` outside an explicit allowlist, where each
156+
entry carries a comment justifying why the path is unreachable on a real
157+
server. Without this the migration decays back within months.
158+
159+
**Mechanism:** the repo lints with **oxlint only** (`"lint": "oxlint ."`), and
160+
oxlint cannot express a custom rule of this shape. Adding ESLint solely for one
161+
rule is disproportionate. Instead this is a **meta-test** —
162+
`packages/testing/src/no-sdk-mocks.spec.ts` — that walks every `*.spec.ts` in
163+
the workspace, greps for `vi.mock("@temporalio/…")`, and fails with the
164+
offending file and line unless the path appears in a documented allowlist
165+
constant.
166+
167+
Rationale: dependency-free, runs per-PR inside the normal suite, and produces a
168+
better diagnostic than a lint rule would (it can name the tier the test should
169+
move to).
170+
171+
Allowlist seeded with the three `packages/testing/*` wiring specs, each with a
172+
one-line justification.
173+
174+
### Mutation testing
175+
176+
Stryker over `packages/contract` and `packages/worker`, **nightly, not
177+
per-PR**. Mutation score is the only mechanism that measures whether a test
178+
would _catch_ a bug rather than merely execute the line.
179+
180+
Scoped to pure modules — `contract-errors`, `wire-format`, `builder`,
181+
`error-tags`, option merging — and explicitly NOT sandboxed workflow paths,
182+
where run cost is prohibitive.
183+
184+
Expect a poor initial score, particularly where coverage is
185+
execution-without-assertion. **That is the finding, not a failure.**
186+
187+
### Explicitly out of scope
188+
189+
- Property-based testing (`fast-check`) — considered, deferred.
190+
- Enforced coverage floor — deferred. A threshold applied before the mocks are
191+
gone would lock in false confidence; revisit after migration.
192+
193+
## Risks
194+
195+
| Risk | Mitigation |
196+
| ------------------------------------ | ------------------------------------------------------------------------------------------- |
197+
| Migrated tests flake on timing | Use time-skipping; no wall-clock waits. Any `sleep` in a test is a review failure. |
198+
| Suite wall-clock grows | Bundle cache; per-test task queues rather than per-test environments. |
199+
| Coverage drops during migration | Migrate file-by-file; each file's replacement lands in the same commit as its deletion. |
200+
| Shared env state bleeds across tests | Unique task queue + workflow ID per test. |
201+
| Stryker runtime unbounded | Nightly only, scoped to pure modules. |
202+
| Registration checks silently skipped | `registration-*` specs pinned to `workflowsPath`; asserted in the lint allowlist rationale. |
203+
204+
## Success criteria
205+
206+
1. Zero `vi.mock("@temporalio/*")` outside the justified allowlist.
207+
2. The 77 identified tests assert effects on a real time-skipping server, and
208+
`worker.spec.ts`'s 18 are split between `unit` and `inprocess`.
209+
3. The `no-sdk-mocks` meta-test blocks reintroduction.
210+
4. Stryker runs nightly over the scoped modules and reports a baseline score.
211+
5. Per-PR suite wall-clock does not regress materially versus today.

0 commit comments

Comments
 (0)