From 0ed5ca8e3d6248c5d5597edab3be43ab5ecc2b6f Mon Sep 17 00:00:00 2001 From: "F.N. Claessen" Date: Mon, 3 Aug 2026 17:54:37 +0200 Subject: [PATCH 1/2] Require new tests to be proven capable of failing A test that never fails asserts nothing, and reads exactly like one that works. This adds the convention: break what the test covers, watch it go red, restore. The prompt is a real miss rather than a hypothetical. A balance-group scenario in test_highspy_equivalence.py passed with the constraint it was named after entirely disabled: both devices in the node had a free flow band and no cost incentive, so the optimum was zero flow whether or not the balance was enforced. Only this check caught it, and the failure mode generalises -- optimisation tests over devices with no reason to move are satisfied trivially. Adds a section to the testing instructions with the vacuous/binding contrast, a note that equivalence tests need the compared code path actually reached on each side, and the ask to say in the PR description what was broken to prove it -- reviewers cannot tell a binding test from a vacuous one by reading it. Also surfaces it in CLAUDE.md's most-missed list. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01X2jLjsQzwztDc7Nxz1ozmQ Signed-off-by: F.N. Claessen --- .github/instructions/testing.instructions.md | 36 ++++++++++++++++++++ CLAUDE.md | 4 +++ 2 files changed, 40 insertions(+) diff --git a/.github/instructions/testing.instructions.md b/.github/instructions/testing.instructions.md index 6477db9d7c..955944f17d 100644 --- a/.github/instructions/testing.instructions.md +++ b/.github/instructions/testing.instructions.md @@ -75,6 +75,42 @@ Before changing a test that fails, investigate whether the test is intentionally A failing test often reveals a production bug, not a test bug. +## Prove a new test can fail + +A test that never fails asserts nothing, and it is indistinguishable from a test that works. +Before considering a new test done, break the thing it covers and watch it go red: +comment out the constraint, invert the condition, delete the line — then restore. +If the test still passes, it is not testing what its name says. + +This matters most where the assertion depends on the *problem* having a unique answer. +An optimisation test is the classic trap: +constrain devices that have no incentive to move, and the optimum is "do nothing" with or without the constraint, +so the test passes whether or not the feature works. + +```python +# ❌ Vacuous: with no cost incentive, both devices sit at zero either way, +# so the balance constraint is satisfied trivially and the test proves nothing. +device_constraints = [make_flow_device(0, 1), make_flow_device(-1, 0)] + +# ✅ Binding: the consumer's draw is pinned, so the balance is the only reason +# the producer runs. Remove the constraint and the schedules diverge. +consumer["derivative equals"] = -0.4 +``` + +This was not hypothetical: a balance-group scenario in +`flexmeasures/data/models/planning/tests/test_highspy_equivalence.py` +passed with the constraint it was named after entirely disabled, and only this check caught it. + +Say in the PR description that you did it, and what you broke to prove it. +Reviewers cannot tell a binding test from a vacuous one by reading it. + +## Equivalence tests need both sides exercised + +When two implementations are compared (see `test_highspy_equivalence.py`), +a scenario only has value if each side actually reaches the code under comparison. +Disable the new code path on one side and confirm the scenario fails; +a scenario that passes with the feature removed is comparing two no-ops. + ## Module-scoped fixture state Module-scoped fixtures are shared across tests. When modifying shared objects (e.g. `asset.sensors_to_show`), reset them to the column default — not to `None` — in teardown: diff --git a/CLAUDE.md b/CLAUDE.md index 33fd4af42d..f1124883ff 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -16,6 +16,10 @@ Most-missed rules, called out so they are not forgotten: commit again. See [`pre-commit-hooks.instructions.md`](.github/instructions/pre-commit-hooks.instructions.md). - **Add a changelog entry** for user-facing changes, in the right section, with a PR link. See [`changelog.instructions.md`](.github/instructions/changelog.instructions.md). +- **Prove a new test can fail before calling it done** — break what it covers and watch it go red, then + restore. A test that passes with the feature disabled asserts nothing; optimisation tests are especially + prone to this, because a problem with no incentive to move has the same optimum either way. See + [`testing.instructions.md`](.github/instructions/testing.instructions.md). - **One logical change per commit** ([`atomic-commits.instructions.md`](.github/instructions/atomic-commits.instructions.md)), **timezone-aware datetimes always** ([`timezone-awareness.instructions.md`](.github/instructions/timezone-awareness.instructions.md)), **catch specific exceptions** ([`error-handling.instructions.md`](.github/instructions/error-handling.instructions.md)), From a00facc2394d2af6439811dcf7a3012d9fceb1a8 Mon Sep 17 00:00:00 2001 From: "F.N. Claessen" Date: Mon, 3 Aug 2026 23:16:35 +0200 Subject: [PATCH 2/2] review: tighten the instruction Dropped the code example and the duplicated references, and merged the equivalence-test case into the paragraph on vacuous assertions rather than repeating it as its own point. 36 lines to 11, same asks. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01X2jLjsQzwztDc7Nxz1ozmQ Signed-off-by: F.N. Claessen --- .github/instructions/testing.instructions.md | 38 ++++---------------- CLAUDE.md | 5 ++- 2 files changed, 9 insertions(+), 34 deletions(-) diff --git a/.github/instructions/testing.instructions.md b/.github/instructions/testing.instructions.md index 955944f17d..7df2b70b00 100644 --- a/.github/instructions/testing.instructions.md +++ b/.github/instructions/testing.instructions.md @@ -77,39 +77,15 @@ A failing test often reveals a production bug, not a test bug. ## Prove a new test can fail -A test that never fails asserts nothing, and it is indistinguishable from a test that works. -Before considering a new test done, break the thing it covers and watch it go red: -comment out the constraint, invert the condition, delete the line — then restore. -If the test still passes, it is not testing what its name says. +A test that never fails asserts nothing, and reads exactly like one that works. +Before calling a new test done, break what it covers — comment out the constraint, invert the condition — and confirm it goes red, then restore. -This matters most where the assertion depends on the *problem* having a unique answer. -An optimisation test is the classic trap: -constrain devices that have no incentive to move, and the optimum is "do nothing" with or without the constraint, -so the test passes whether or not the feature works. +Watch for assertions that depend on the problem having a unique answer. +An optimisation test over devices with no incentive to move has the same optimum with or without the constraint, so it passes either way. +In `test_highspy_equivalence.py`, where two backends are compared, also disable the new code path on one side: a scenario that survives that is comparing two no-ops. +Both traps have been hit — a balance-group scenario there passed with the constraint it was named after entirely disabled. -```python -# ❌ Vacuous: with no cost incentive, both devices sit at zero either way, -# so the balance constraint is satisfied trivially and the test proves nothing. -device_constraints = [make_flow_device(0, 1), make_flow_device(-1, 0)] - -# ✅ Binding: the consumer's draw is pinned, so the balance is the only reason -# the producer runs. Remove the constraint and the schedules diverge. -consumer["derivative equals"] = -0.4 -``` - -This was not hypothetical: a balance-group scenario in -`flexmeasures/data/models/planning/tests/test_highspy_equivalence.py` -passed with the constraint it was named after entirely disabled, and only this check caught it. - -Say in the PR description that you did it, and what you broke to prove it. -Reviewers cannot tell a binding test from a vacuous one by reading it. - -## Equivalence tests need both sides exercised - -When two implementations are compared (see `test_highspy_equivalence.py`), -a scenario only has value if each side actually reaches the code under comparison. -Disable the new code path on one side and confirm the scenario fails; -a scenario that passes with the feature removed is comparing two no-ops. +Say in the PR description what you broke to prove it — a reviewer cannot tell a binding test from a vacuous one by reading it. ## Module-scoped fixture state diff --git a/CLAUDE.md b/CLAUDE.md index f1124883ff..0781af5a5f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -16,9 +16,8 @@ Most-missed rules, called out so they are not forgotten: commit again. See [`pre-commit-hooks.instructions.md`](.github/instructions/pre-commit-hooks.instructions.md). - **Add a changelog entry** for user-facing changes, in the right section, with a PR link. See [`changelog.instructions.md`](.github/instructions/changelog.instructions.md). -- **Prove a new test can fail before calling it done** — break what it covers and watch it go red, then - restore. A test that passes with the feature disabled asserts nothing; optimisation tests are especially - prone to this, because a problem with no incentive to move has the same optimum either way. See +- **Prove a new test can fail** — break what it covers, confirm it goes red, restore. A test that passes + with the feature disabled asserts nothing. See [`testing.instructions.md`](.github/instructions/testing.instructions.md). - **One logical change per commit** ([`atomic-commits.instructions.md`](.github/instructions/atomic-commits.instructions.md)), **timezone-aware datetimes always** ([`timezone-awareness.instructions.md`](.github/instructions/timezone-awareness.instructions.md)),