docs(zsh-standard): require a removal's test to be able to fail - #605
Merged
Conversation
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds
zsh/test/cover-removed-behavior. Documentation and policy only.Why
zi#488 was not caused by a missing test. It was caused by a test that could not fail.
zi#477 removed a line as dead and added
tests/plugin-autoload-fpath-scope.zshas evidence the removal was safe. Its commit message said so outright:That is the defect. The test asserted
autoload +Xonly for functions in the plug-in's own directories, which resolve from$PLUGIN_DIRwith or without the removed line. The behaviour the line actually provided, carrying the caller's$fpathso a foreign function could resolve, was never exercised. The test passed on both sides because it was blind to the only thing at issue, andAloxaf/fzf-tabbroke completely.A test whose result is invariant to the change it accompanies is evidence of nothing, and it is worse than no test: it reads as diligence and closes down review.
It recurred immediately
While fixing #488 I inspected the neighbouring
-wbranch of the same function, judged itsfpath+=( $PLUGIN_DIR )an unlocalized leak from its shape alone, and wrote the "fix". It broke-wcompletely:autoload -w <digest>declares functions that resolve lazily at first call, so that append has to outlive the call. The full suite stayed green while-wwas broken end to end, because nothing exercised it either. Corrected in zi#493.Two instances, one function, one release cycle. Both times a line was judged inert on sight, and both times the test set was structurally incapable of disagreeing.
Duplicate search
Per the candidate quality gate in
runbooks/learning-capture.md:zsh/review/report-without-rewritezsh/change/conform-touched-codezsh/test/isolate-environmentzsh/test/declare-negative-fixtureszsh/test/match-production-profileNothing requires that a test cited for a removal be capable of failing without the removed code.
The rule
Scoped to
test-fixture, matching the other rules in that section: the obligation lands on the test artifact, not on the profile of the code being removed. My first draft declared all five profiles, which the validator'sstartup-filemembership check correctly rejected.The check itself is cheap: run the candidate test against the unmodified source. If it passes there, it is not evidence.
Verification
Frozen consumer-parser golden: block count 65 to 66, digest replaced.
startup-filemembership unchanged.Closes #604