Add a mutation helper that refuses when the edit did not land - #371
Merged
Conversation
Rules 7 and 8 ask contributors to break what an assertion guards and to check that a surviving mutation applied. The usual shell loop does that with a silent failure mode: when the edit does not match, it carries on, the tests run against unmodified code, and the green result reads as "the assertion does not catch this". That happened five times across three sessions in one day, and a person caught it every time. This is the check written down. It refuses a missing anchor, an ambiguous one, and a replacement that leaves the file byte-identical, exiting non-zero so a chained test command does not run. Restoring reads its own backup rather than git, which would take uncommitted work in that file with it. The guide points at it as one safe way rather than the way: a mutation that needs a different shape should use one, and a tool that had to be bent around would be worked around instead.
CI runs it and tools/static-checks/run.py did not know about it, which its own contract test refuses: a local runner that reports success without having run a step is worse than one that does not claim to cover it. Found by that test rather than by review, which is the test working.
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
1 participant
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.
Hardening rules 7 and 8 ask contributors to break what an assertion guards and to confirm that a surviving mutation actually applied. The ordinary way to do that is a shell loop that edits a file, runs the tests, and restores it, and that loop has a failure mode with no symptom. When the edit does not match, most spellings carry on, the tests run against unmodified code, and the green result reads as "the assertion does not catch this" when the assertion was never handed anything to catch.
It happened five times across three sessions in one day. Every time somebody noticed, which is the part that does not scale.
The tool edits one file and refuses three things, exiting non-zero so a
&&chain stops before the tests run: an anchor that is missing, an anchor that appears more than once, and a replacement that leaves the file byte-identical. The third is the one worth having on its own. An anchor can match while the edit changes nothing, and that is the same false green by a different route; it is also the case acount == 1assertion in the calling script does not cover.--restorereads a backup the tool wrote. Two sessions lost uncommitted edits togit checkout -- <file>on the same day this was written, so having a restore path that is not that command removes one reason to reach for it.The guide points at it as one safe way and not as the way. Rule 7 says to break what you guard, not how, and a mutation that needs a different shape should use a different shape. A required tool that did not fit would be worked around, and the work-around is the thing with no guard.
selftest.pydrives each refusal and each success, because a tool that refused everything would pass a file of refusal tests and be useless. It runs in the existing lint job for the reasonai-reviewgives there: standard library only, about a second, and a new job is a new required check that docs-only pull requests stall on.Verified by mutating the tool with itself: deleting the unchanged-file check makes the selftest fail on exactly that case and nothing else.