Strengthens TESTING-STANDARDS.md contract 6 (counter-tests for negative space): a test on an error/fallback branch must assert the specific degraded verdict the branch produces, not merely that the call did not throw. Generalizes the liveness-vs-correctness finding from #3050 and epic #3051 (closed, all 14 enumerated modules drained) into a standing review expectation, now that the one-time enumeration is done. Worked examples are the real pre-fix and post-fix shapes of tests/worktree-safety.test.cjs's resolveWorktreeContext timeout counter-test, verified directly against source. Deliberately not lint-enforced: a pattern scan for fail-open shapes scored 1 true positive against 3 false positives during #3051's own measurement (source: epic #3051 body, Phase 3). Cross-linked from CONTRIBUTING.md's QA Matrix Requirements so reviewers see it where they already apply the matrix. H4 of epic #3053. Co-authored-by: sim <sim@local>
This commit is contained in:
@@ -612,6 +612,8 @@ Happy-path tests are not enough for code that accepts user input, reads project
|
||||
|
||||
See [`TEST-EXAMPLES.md`](TEST-EXAMPLES.md) for concrete demo tests that show these requirements in practice.
|
||||
|
||||
**Standing rule for error/fallback branches:** feeding an adversarial input is not sufficient on its own — if the code degrades permissively instead of throwing, the test must assert the *specific* degraded verdict, not just that the call survived. See [`TESTING-STANDARDS.md` — "Standing rule: assert the degraded verdict"](TESTING-STANDARDS.md#standing-rule-assert-the-degraded-verdict-not-just-did-not-throw).
|
||||
|
||||
Use this matrix when it applies to the changed surface:
|
||||
|
||||
1. Happy path
|
||||
|
||||
@@ -83,6 +83,44 @@ See [`CONTRIBUTING.md` — "QA Matrix Requirements"](CONTRIBUTING.md#qa-matrix-r
|
||||
|
||||
**Enforcement:** Code review, `no-only-tests/no-only-tests` ESLint rule (prevents happy-path-only merges via `test.only`).
|
||||
|
||||
#### Standing rule: assert the degraded verdict, not just "did not throw"
|
||||
|
||||
A counter-test that feeds a hostile or failing input satisfies the letter of contract 6 above and can still be worthless. If a function has an error or fallback branch — a guard that degrades permissively on bad input, a resolver that falls back to a default root, a lock that expires — the test for that branch must assert the **specific degraded verdict** the branch produces, not merely that the call completed without throwing or returned *some* value of the right type.
|
||||
|
||||
**Non-compliant (the actual pre-fix shape of `tests/worktree-safety.test.cjs`'s `resolveWorktreeContext` timeout counter-test, per [#3050](https://github.com/open-gsd/gsd-core/issues/3050) finding 2 and epic [#3051](https://github.com/open-gsd/gsd-core/issues/3051) Phase 3, generalized here as [#3053](https://github.com/open-gsd/gsd-core/issues/3053) H4):**
|
||||
|
||||
```javascript
|
||||
// A liveness test wearing a correctness test's name — it proves the call
|
||||
// survived a timeout, not that it degraded to the RIGHT shape.
|
||||
test('resolveWorktreeContext handles a timeout', () => {
|
||||
const result = resolveWorktreeContext('/repo/wt', { execGit: makeTimeoutStub() });
|
||||
assert.doesNotThrow(() => resolveWorktreeContext('/repo/wt', { execGit: makeTimeoutStub() }));
|
||||
assert.strictEqual(typeof result.effectiveRoot, 'string'); // true of ANY string, including the wrong one
|
||||
});
|
||||
```
|
||||
|
||||
**Compliant (the actual fixed test, `tests/worktree-safety.test.cjs`):**
|
||||
|
||||
```javascript
|
||||
test('returns effectiveRoot=cwd, mode=current_directory, reason=git_timed_out on timeout, not throw', () => {
|
||||
const result = resolveWorktreeContext('/tmp', { execGit: makeTimeoutStub() });
|
||||
assert.deepStrictEqual(result, {
|
||||
effectiveRoot: '/tmp', // the specific degraded value, not just "a string"
|
||||
mode: 'current_directory',
|
||||
reason: 'git_timed_out',
|
||||
});
|
||||
});
|
||||
```
|
||||
|
||||
This is not the same requirement as the input-rejection rule above it. A test can already satisfy "feed the SUT a hostile input" while still failing this one, if it never checks *what the SUT did in response*. Two shapes are explicitly out of scope for this rule — they are not fail-open guards and adding this counter-test to them would be noise, not signal:
|
||||
|
||||
- A branch that re-throws or propagates the error rather than producing a degraded verdict — contract 4 above ("test the claimed path") already covers it via `assert.throws`.
|
||||
- A branch that returns a documented, structured error signal that the test already asserts on directly (a three-state policy that fails closed on malformed input, a dispatch convention, an `{isError: true}` return shape) — the structured field *is* the verdict; asserting on it already satisfies this rule.
|
||||
|
||||
**Why this isn't a lint rule.** A pattern scan for fail-open shapes was measured directly against this repo's `src/*.cts` during epic #3051: it scored 1 true positive against 3 false positives (a documented three-state policy, a dispatch convention, and a structured `isError` return each looked like a fail-open guard and were not). The permissive-verdict shape is module-specific, not mechanically enumerable, so a lint rule here would be both incomplete and noisy. This is a code-review expectation, not a CI gate — it will not fail a build on its own; it fails when a reviewer (human or `/code-review`) lets a "did not throw" test stand in for a correctness test.
|
||||
|
||||
**Enforcement:** Code review only — deliberately not lint-enforced (see above). Cross-linked from [`CONTRIBUTING.md` — "QA Matrix Requirements"](CONTRIBUTING.md#qa-matrix-requirements) so reviewers see it at the point they already apply the negative-space matrix.
|
||||
|
||||
---
|
||||
|
||||
## New policies (ADR 456)
|
||||
|
||||
Reference in New Issue
Block a user