Delete TestMatchesGlobs and TestMatchesAction private function tests#83
Merged
Delete TestMatchesGlobs and TestMatchesAction private function tests#83
Conversation
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.
Fixes #76
Our
docs/testing.mdliterally calls outmatchesGlobsby name as the canonical example of what not to test: "ifmatchesGlobsbreaks, aResolvetest will catch it. Testing both just means two tests to update when the signature changes." And yet there it was, sitting in the test suite alongsideTestMatchesAction, doing exactly the thing the docs tell contributors not to do.This matters because new contributors read the tests before they read the docs. Having private function tests in the codebase signals "this is how we do things here" regardless of what the guidelines say. It undermines the whole testing philosophy we spent time documenting.
Every scenario these two tests covered is already exercised through the public
Resolve()API:TestResolve_NonPythonFile,TestResolve_ExcludePattern,TestResolve_EditBeforeTestResolve_ExcludePattern+ theExcludeOverridesMatchproperty testTestResolve_ContextAboveRootIsIgnoredTestResolve_ReadDoesNotMatchEditOnly,TestResolve_CreateAction, and theEditEntriesFilteredForActionReadproperty testNo coverage gaps. Just removed the duplication.
Why not keep them as "extra safety"? Because that's exactly the trap the docs warn about. Two tests covering the same behavior means two things to update when internals change, which makes refactoring harder and trains people to see private function tests as normal. The 19 remaining
TestResolve_*tests (including 5 property-based ones with randomized inputs) give us better coverage than the deleted unit tests ever did.Dev Ghost metrics
Total duration: 4m 32s
Turns: 60
Tool calls: 48
Tokens: 368,976 input / 5,084 output
Resolves #76