From f17db4a0ac738f4e4b0f7a312dbf38557aa2eeff Mon Sep 17 00:00:00 2001 From: Sal Date: Thu, 3 Sep 2026 00:30:11 +0100 Subject: [PATCH] docs(zsh-standard): require a removal's test to be able to fail z-shell/zi#488 was not caused by a missing test. It was caused by a test that could not fail. z-shell/zi#477 removed a line as dead and added a test as evidence the removal was safe. Its commit message said the test "passes before and after, which is the point". That is the defect. The test asserted `autoload +X' only for functions in the plug-in's own directories, which resolve from $PLUGIN_DIR either way. The behaviour the removed line provided, carrying the caller's $fpath so a foreign function could resolve, was never exercised, so the test was structurally incapable of objecting. fzf-tab broke completely. Not a one-off. While fixing that, the neighbouring `-w' branch was judged an unlocalized leak by inspection and "fixed"; the whole suite stayed green while -w was broken end to end, because nothing exercised it either. Corrected in z-shell/zi#493. Two instances, one function, one release cycle, both a line judged inert on sight against a test set that could not disagree. Add zsh/test/cover-removed-behavior: a test offered as evidence for a removal must exercise the behaviour the removed code provided, observed failing without it and passing with it. Running the candidate against the unmodified source is the whole check; if it passes there, it is not evidence. Scoped to test-fixture, matching the other rules in the section: the obligation lands on the test artifact, not on the profile of the code being removed. Searched for duplicates first. zsh/review/report-without-rewrite governs read-only work, zsh/change/conform-touched-code governs cleanup scope, and the three existing zsh/test rules govern how a test is built, not what it must be able to detect. The frozen consumer-parser golden moves accordingly: block count 65 to 66 and the digest replaced. startup-file membership is unchanged, as the rule is test-fixture only. Closes #604 --- .../zsh-scripting.instructions.md | 25 +++++++++++++++++++ lib/zsh-standard-policy.json | 9 +++++++ scripts/test_validate_zsh_standard_policy.py | 4 +-- scripts/validate-zsh-standard-policy.py | 1 + 4 files changed, 37 insertions(+), 2 deletions(-) diff --git a/.github/instructions/zsh-scripting.instructions.md b/.github/instructions/zsh-scripting.instructions.md index 637299dcb..a99ac23f6 100644 --- a/.github/instructions/zsh-scripting.instructions.md +++ b/.github/instructions/zsh-scripting.instructions.md @@ -315,6 +315,31 @@ and remember that a system `zshenv` may still execute. Mark intentionally invalid, legacy, or lint-negative fixtures in repository metadata instead of excluding an entire test directory. +### `zsh/test/cover-removed-behavior` + +- Level: `required` +- Profiles: `test-fixture` +- Minimum Zsh: `null` +- Basis: `organization-policy` +- Evidence: `manual-index` +- Enforcement: `human-review` + +When a change removes code on the grounds that it is unnecessary, a test +offered as evidence for that removal must exercise the behaviour the removed +code provided. Observe it failing with the code removed and passing with it +present. A test that passes in both states supports nothing and must not be +cited as though it does. + +Run the candidate test against the unmodified source before removing anything. +If it passes there, it is not evidence, and either the test or the premise of +the removal is wrong. + +This applies with particular force where a line looks inert: a declaration that +appears redundant, an assignment whose value seems unused, a mutation that +resembles a leak. Such a line is exactly the kind that gets removed on +inspection, and exactly the kind whose loss a nearby test will not notice +because the test covers the path the line does not serve. + ### `zsh/test/match-production-profile` - Level: `required` diff --git a/lib/zsh-standard-policy.json b/lib/zsh-standard-policy.json index 1b800846b..f67954729 100644 --- a/lib/zsh-standard-policy.json +++ b/lib/zsh-standard-policy.json @@ -310,6 +310,15 @@ "evidence": ["shell-grammar"], "enforcement": ["classifier"] }, + { + "id": "zsh/test/cover-removed-behavior", + "level": "required", + "profiles": ["test-fixture"], + "minimum_zsh": null, + "basis": "organization-policy", + "evidence": ["manual-index"], + "enforcement": ["human-review"] + }, { "id": "zsh/test/match-production-profile", "level": "required", diff --git a/scripts/test_validate_zsh_standard_policy.py b/scripts/test_validate_zsh_standard_policy.py index 655d75bfe..d281a8cdd 100644 --- a/scripts/test_validate_zsh_standard_policy.py +++ b/scripts/test_validate_zsh_standard_policy.py @@ -1979,7 +1979,7 @@ def test_repair_2_consumer_parser_outputs_match_frozen_golden(self) -> None: }, "parsed_rules": validator._markdown_rules(instruction), } - self.assertEqual(len(snapshot["rule_blocks"]), 65) + self.assertEqual(len(snapshot["rule_blocks"]), 66) digest = hashlib.sha256( json.dumps( snapshot, @@ -1991,7 +1991,7 @@ def test_repair_2_consumer_parser_outputs_match_frozen_golden(self) -> None: self.assertEqual( digest, - "d841ec864632352bc399bb035780e6805200425a4f14b0b700d3e7a9e8f7c59e", + "c2faea55b03018e947b6e1b5fef023c4d50a9474cfc9f4c4f372f68cabf6ba58", msg=( "The frozen golden covers the parsed output of every path in " f"{paths}. Editing any of them changes this digest, which is " diff --git a/scripts/validate-zsh-standard-policy.py b/scripts/validate-zsh-standard-policy.py index e80555694..c3b528f6d 100644 --- a/scripts/validate-zsh-standard-policy.py +++ b/scripts/validate-zsh-standard-policy.py @@ -144,6 +144,7 @@ "zsh/completion/preserve-trust-boundaries", "zsh/test/isolate-environment", "zsh/test/declare-negative-fixtures", + "zsh/test/cover-removed-behavior", "zsh/test/match-production-profile", "zsh/options/canonical-spelling", "zsh/options/declare-correctness-state",