From 0b92c5e7d39c416288b4b84a7288d9d4b03e96e6 Mon Sep 17 00:00:00 2001 From: Fernando de Oliveira <5161098+fedeoliv@users.noreply.github.com> Date: Fri, 16 Oct 2026 14:09:27 -0400 Subject: [PATCH] chore(testing): guard against tautological tests (#72) Require independent behavioral expectations and reuse existing tests during behavior-preserving refactors to avoid circular assertions and unnecessary test churn. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 40bf5936-fdf2-408a-afe7-dd07b66ed7da --- .../devsquad.implement.execute.agent.md | 2 +- .../agents/devsquad.review.tests.agent.md | 6 ++- .../quality-gate/references/rubrica-code.md | 2 +- .../devsquad/skills/test-discipline/SKILL.md | 44 +++++++++++++++++-- CHANGELOG.md | 4 ++ docs/src/content/docs/skills/quality.mdx | 23 +++++++++- 6 files changed, 73 insertions(+), 8 deletions(-) diff --git a/.github/plugins/devsquad/agents/devsquad.implement.execute.agent.md b/.github/plugins/devsquad/agents/devsquad.implement.execute.agent.md index caa1dd3..9693d93 100644 --- a/.github/plugins/devsquad/agents/devsquad.implement.execute.agent.md +++ b/.github/plugins/devsquad/agents/devsquad.implement.execute.agent.md @@ -45,7 +45,7 @@ The test must fail BEFORE the fix and pass AFTER. If the test passes before the - **Anchor to existing patterns**: Before writing new code, search the codebase for a similar implementation (e.g., an existing endpoint, component, or test of the same shape). Use it as the structural exemplar so new code stays consistent with project conventions. - **Phase by phase**: Complete each phase before moving to the next - **Respect dependencies**: Sequential tasks in order, parallel [P] tasks can run together -- **Test discipline**: Choose test-first or design-first-then-test as appropriate for each task. Consult the `test-discipline` skill for guidance on when each approach fits, quality criteria, and test doubles hierarchy. +- **Test discipline**: Choose test-first or design-first-then-test as appropriate for each task. Consult the `test-discipline` skill for quality criteria, independent behavioral oracles, tautological assertions, and test doubles hierarchy. For behavior-preserving refactors, run and reuse existing behavioral tests, adding tests only for concrete coverage gaps rather than newly extracted helpers or delegation details. - **ADR compliance**: Follow documented architectural decisions - **Traceability**: Add comment referencing spec/task in generated code - **Consult harness learnings**: Before coding, read `.memory/harness-learnings.md` (if it exists) and check for entries whose Scope overlaps with the task's files. Apply high-confidence Guidance proactively to avoid known pitfalls. diff --git a/.github/plugins/devsquad/agents/devsquad.review.tests.agent.md b/.github/plugins/devsquad/agents/devsquad.review.tests.agent.md index 5d183f4..48e53ff 100644 --- a/.github/plugins/devsquad/agents/devsquad.review.tests.agent.md +++ b/.github/plugins/devsquad/agents/devsquad.review.tests.agent.md @@ -28,7 +28,11 @@ Execute the detected commands and capture results: - Does the build compile without errors? - Do existing tests continue to pass? - Do new tests cover the spec scenarios? -- Do tests meet quality criteria? (Consult the `test-discipline` skill: behavior-focused, public-interface-only, refactor-resistant, correct test doubles hierarchy) +- Do tests meet quality criteria? (Consult the `test-discipline` skill: behavior-focused, public-interface-only, refactor-resistant, independent behavioral oracle, correct test doubles hierarchy) +- Do assertions derive expected outcomes from a contract or independently justified property rather than the result under test, a copied production algorithm, or a shared helper that could hide the same bug? +- For behavior-preserving refactors, do existing tests already protect the affected contract, and do added tests address concrete coverage gaps rather than mirror new internal structure? + +For test-quality findings, identify the assertion, explain what makes its expectation circular or coupled to implementation, and name the contract violation left undetected. Assess shared helpers, computed expectations, snapshots, properties, and mocks by what they prove rather than rejecting the technique itself. ### 3. Analyze Failures diff --git a/.github/plugins/devsquad/skills/quality-gate/references/rubrica-code.md b/.github/plugins/devsquad/skills/quality-gate/references/rubrica-code.md index d14fc8d..787d72b 100644 --- a/.github/plugins/devsquad/skills/quality-gate/references/rubrica-code.md +++ b/.github/plugins/devsquad/skills/quality-gate/references/rubrica-code.md @@ -9,7 +9,7 @@ Applicable after `devsquad.implement` completes a medium or high impact task. Do | C1 | Conformance with spec | Code implements the functional requirements (RF-XXX) mapped in the task. Relevant conformance criteria (CC-XXX) are met. | Cross-check with spec.md | | C2 | Conformance with ADR | Technologies, patterns, and versions used match the referenced ADRs. | Cross-check with ADRs | | C3 | No regression | Existing tests continue passing (if the project has tests). | Run test suite | -| C4 | Behavior coverage | New code with medium/high impact has tests that validate implemented behavior (relevant success and error scenarios). Bug fixes have a test that reproduces the bug. Exemptions: infrastructure tasks, configuration, or when there is no test framework in the project. | Check new/modified tests | +| C4 | Behavior coverage | New code with medium/high impact has tests that validate implemented behavior (relevant success and error scenarios) using independent behavioral oracles as defined in `test-discipline`. Expectations do not merely repeat the implementation. Bug fixes have a test that reproduces the bug. Existing behavioral tests can satisfy coverage for behavior-preserving refactors, with new tests required only for concrete coverage gaps. Exemptions: infrastructure tasks, configuration, or when there is no test framework in the project. | Check relevant existing and new/modified tests against the contract and identify a plausible violation their assertions would catch | ## Quality Criteria diff --git a/.github/plugins/devsquad/skills/test-discipline/SKILL.md b/.github/plugins/devsquad/skills/test-discipline/SKILL.md index ac185a0..e48aa20 100644 --- a/.github/plugins/devsquad/skills/test-discipline/SKILL.md +++ b/.github/plugins/devsquad/skills/test-discipline/SKILL.md @@ -1,6 +1,6 @@ --- name: test-discipline -description: "Testing practices for behavior-focused, refactor-resistant tests. Use when implementing features or fixing bugs to ensure tests verify intent through public interfaces. Also use when adding regression tests after bug fixes. Consulted by the implement agent during coding and by the review agent when validating test quality. Do not use for test strategy decisions (use complexity-analysis), for failure triage and root-cause analysis (use debugging-recovery; after root cause is localized, use test-discipline to add the reproducing regression test and fix), or for integration test infrastructure setup." +description: "Testing practices for behavior-focused, refactor-resistant tests. Use when implementing features, fixing bugs, refactoring, or adding or reviewing tests to ensure they verify intent through public interfaces and avoid tautological assertions. Also use when adding regression tests after bug fixes. Consulted by the implement agent during coding and by the review agent when validating test quality. Do not use for test strategy decisions (use complexity-analysis), for failure triage and root-cause analysis (use debugging-recovery, then after root cause is localized use test-discipline to add the reproducing regression test and fix), or for integration test infrastructure setup." --- # Test Discipline @@ -55,7 +55,7 @@ The first slice should be a **tracer bullet**: a thin end-to-end path that prove 1. **Choose the next vertical slice** (tracer bullet first) 2. **Design**: Sketch the interface and responsibilities for this slice 3. **Implement + Test**: Either test-first (RED → GREEN) or implement-then-test, based on the context guidance above -4. **Verify**: Run the full test suite — new test passes, no regressions +4. **Verify**: Run the full test suite to confirm relevant existing tests and any justified new tests pass without regressions 5. **Refactor**: Clean up while all tests are green. Never refactor with failing tests. 6. **Repeat**: Next slice @@ -68,6 +68,7 @@ Every test must satisfy these criteria regardless of when it was written: | **Behavior-focused** | Tests what the system does, not how it does it | | **Public interface only** | Uses the same API that callers use | | **Refactor-resistant** | Would survive internal restructuring without changes | +| **Independent oracle** | Expected outcomes follow a requirement, concrete example, or independently justified property rather than repeating the implementation | | **Independent** | Does not depend on other tests or test execution order | | **Deterministic** | Same result every run, no timing dependencies | | **Readable** | Test name describes the behavior being verified | @@ -86,13 +87,45 @@ For unit test rules, integration test rules, and test doubles hierarchy, follow **Warning sign**: a test breaks when you refactor, but behavior has not changed. That test was testing implementation, not behavior. +## Avoid Tautological Tests + +A tautological test derives its expectation from the implementation it is meant to verify, so the same mistake can appear on both sides of the assertion. Passing results then provide false confidence. + +1. **Establish the oracle**: Derive expected outcomes from requirements, concrete examples, externally visible contracts, or independently justified properties. Do not use the result under test to generate its own expectation or copy the production algorithm into the assertion. +2. **Name a regression the assertion would catch**: Identify a plausible contract violation and check that the assertion would fail for it. This is a reasoning check and does not require mutation-testing tooling. +3. **Keep helpers independent of the behavior under test**: Shared fixture builders are fine. Avoid using the same formatting or calculation helper on both sides when correctness of that operation is part of the asserted contract. +4. **Assess what the assertion proves**: Computed expectations, property-based tests, independent reference implementations, snapshots, and mocks can be valid. A round-trip property alone can miss matching encoder and decoder bugs, and an unreviewed snapshot can preserve a defect. Asserting a stubbed value is meaningful when forwarding that value is the public contract, but does not verify the stubbed dependency's calculation. + +For a contract where a 20 percent discount on 1,000 cents leaves 800 cents payable, avoid copying a buggy production calculation that returns the discount amount instead: + +```python +assert discounted_total(1000, 20) == 1000 * 20 // 100 +``` + +Use the independently established payable amount: + +```python +assert discounted_total(1000, 20) == 800 +``` + +The second assertion catches returning the discount amount. Additional cases should address relevant contract gaps, such as rounding, rather than repeat the same formula across many inputs. + +Reference: [Tautological Tests by Randy Coulman](https://randycoulman.com/blog/2016/12/20/tautological-tests/). + +## Behavior-Preserving Refactoring + +1. Run the existing behavioral tests before changing code to establish a baseline, then run them after the refactor. +2. Reuse existing tests when they already protect the affected contract. Add tests only for a concrete coverage gap in behavior that must remain stable, preferably before restructuring that code. +3. Do not add tests solely for extracted private helpers, delegation details, renamed classes, or structural equivalence between old and new implementations. Comparing two implementations can support a refactor but does not independently establish that either meets the requirements. +4. Preserve behavioral assertions when adapting setup or call sites to an intentional interface change. Do not rewrite expected outcomes merely to match the refactored output. + ## Integration with SDD Lifecycle | Agent | How test discipline is used | |-------|----------------------------| | `devsquad.implement.execute` | Follows per-task procedure for each task | | `devsquad.review.tests` | Uses test quality criteria as review rubric | -| `quality-gate` (code rubric) | Verifies test coverage and behavior focus | +| `quality-gate` (code rubric) | Verifies behavior coverage and independent oracles, including existing tests for refactors | | `debugging-recovery` | Adds regression test (test that reproduces bug, then fix) | ## Anti-patterns @@ -102,6 +135,8 @@ For unit test rules, integration test rules, and test doubles hierarchy, follow | All tests first, then all code | Tests verify imagined behavior | Work in vertical slices | | Mocking what you own | Tests break on refactor | Test through public interface with real implementations | | Testing private methods | Coupled to implementation | Test the behavior those methods enable | +| Tautological assertions | The same bug can appear in implementation and expectation | Derive the expectation from an independent behavioral oracle | +| New tests for every refactor | Tests mirror new structure without protecting additional behavior | Reuse behavioral tests and add only concrete coverage gaps | | 100% coverage as a goal | Coverage without behavior focus is noise | Cover critical paths and edge cases | | Copy-paste test setup | Fragile, hard to maintain | Extract test helpers and fixtures | | Testing the framework | Verifying framework behavior, not yours | Trust the framework, test your logic | @@ -114,6 +149,9 @@ For unit test rules, integration test rules, and test doubles hierarchy, follow [ ] Test describes behavior, not implementation [ ] Test uses public interface only [ ] Test would survive internal refactor +[ ] Expected outcome has an independent behavioral oracle +[ ] Assertion would catch a plausible contract violation +[ ] New tests address concrete behavior gaps rather than refactoring structure [ ] Test doubles follow coding-guidelines hierarchy [ ] No speculative features added [ ] Refactoring done only while all tests pass diff --git a/CHANGELOG.md b/CHANGELOG.md index 9e92029..c301570 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,10 @@ See `CONTRIBUTING.md` for full conventions. - Documentation style instructions and the `documentation-style` skill now explicitly forbid introducing the em-dash character (Unicode U+2014) in newly written or rewritten Markdown and discourage semicolons in prose. Recast sentences for natural flow while preserving their meaning, without mechanically replacing punctuation with abrupt sentence breaks. Semicolons required by code, technical syntax, or verbatim quotations remain allowed. Preserve existing text outside the requested scope. Existing documents do not require a cleanup. - **Consumer action:** Update the DevSquad plugin for the skill change. Apply the same scoped rules to existing `.github/copilot-instructions.md` and `.github/instructions/documentation-style.instructions.md` files, or preview `sdd-init.sh update-all --dry-run` and review all planned overwrites before applying `sdd-init.sh update-all` to refresh managed files. +### Changed + +- The `test-discipline` skill now explicitly covers tautological assertions, independent behavioral oracles, and behavior-preserving refactoring. Implementation and review workers and the code quality rubric reinforce these checks. Reuse existing behavioral tests and add tests only for concrete coverage gaps, without requiring mutation-testing tooling or banning legitimate test helpers and computed expectations. + ### Added (Agent Intent Governance — selective Architecture of Intent adoption) Closes three structural gaps in the framework: framework agents had no explicit behavioral envelope (a reviewer reading `devsquad.implement.agent.md` could not determine in under a minute what the agent was authorized to do, must never do, or how it composes sub-agents); consumer specs describing AI capabilities had no canonical fragment for operational cost commitments; the failure-diagnosis surface had no taxonomy mapping a failure to the upstream artifact that owns the fix. Vocabulary and discipline drawn from "The Architecture of Intent" by Marcel Aldecoa (`https://marcelaldecoa.github.io/TheArchitectureOfIntent/`). The framework adopts the load-bearing principles and deliberately omits parts of the source vocabulary that add no behavior (see "AoI constructs considered and not adopted" in ADR 0014). diff --git a/docs/src/content/docs/skills/quality.mdx b/docs/src/content/docs/skills/quality.mdx index d91f2f7..53e5e9d 100644 --- a/docs/src/content/docs/skills/quality.mdx +++ b/docs/src/content/docs/skills/quality.mdx @@ -1,6 +1,6 @@ --- title: Quality and Security Skills -description: Skills for artifact validation, security assessment, and documentation formatting. +description: Skills for artifact validation, behavior-focused testing, security assessment, and documentation formatting. banner: content: | This project is under active development and subject to breaking changes. See the changelog for release notes. @@ -8,7 +8,7 @@ banner: import { Aside, Badge } from '@astrojs/starlight/components'; -These three skills enforce quality standards: validating artifacts at the point of creation, assessing security in two modes, and enforcing documentation formatting rules. +These skills enforce quality standards through artifact validation, behavior-focused testing, security assessment, and documentation formatting. ## quality-gate @@ -57,6 +57,25 @@ flowchart TD --- +## test-discipline + +Guides behavior-focused tests through public interfaces, with independent expected outcomes and tests that survive internal refactoring. + +| Scenario | Guidance | +|----------|----------| +| Feature or bug fix | Derive expectations from requirements, concrete examples, or independently justified properties | +| Tautological assertion | Avoid copying production calculations or deriving the expectation from the result being tested | +| Behavior-preserving refactor | Run and reuse existing behavioral tests, adding tests only for concrete coverage gaps | +| Test review | Identify a plausible contract violation the assertion would catch and report concrete gaps | + +For example, a contract specifying a 20 percent discount on 1,000 cents establishes an expected payable amount of 800 cents. An assertion that copies a buggy calculation returning the discount amount can pass while violating that contract. + +Computed expectations, shared fixture builders, snapshots, property-based tests, independent reference implementations, and mocks remain valid when they provide meaningful contract evidence. Review what they prove and whether a shared mistake could pass unnoticed. + +**Activation triggers:** Feature implementation, bug fixes, refactoring, test additions, and test-quality review. These checks use the existing test workflow and do not require mutation-testing tooling. + +--- + ## security-review Security assessment in two modes: architectural (during design) and code (during implementation). Covers STRIDE threat modeling, OWASP guidelines, dependency scanning, and compliance checks.