test(cli): stop the read-only object store fixture from proving nothing - #39
Merged
Conversation
The warn-and-continue test breaks `git write-tree` by chmodding .git/objects to 0500, then asserts the break happened before exercising the branch it cares about. That premise is not guaranteed by anything in the test. Git returns an object it already has without writing it, so if the staged tree had reached the store before the chmod, `write-tree` exits 0 under a read-only store and the fixture assertion fails. Reproduced directly: write the tree once with the store writable, chmod to 0500, run `write-tree` again, exit 0. The staged file now opens with a unique line, so the tree it produces cannot already exist. And the failure is checked for its reason rather than only its exit status: any other breakage would satisfy the old assertion while testing something else. Writing that check is what surfaced git's actual wording here — "insufficient permission for adding an object to repository database". This test failed once in a full local run and passed in isolation and on re-run. The pre-existing-object path is the fragility I could prove; whether it is what fired that time is not established.
…took CI caught the fixture assertion failing on macOS with the unique-tree change already in place, which rules out the pre-existing-object path as the cause there: `git write-tree` succeeded with .git/objects at mode 0500. Mode bits are not the whole permission story. macOS keeps inherited ACLs across chmod, so a directory can read 0500 and still accept a new file. The fixture then builds nothing and the test reports a failure of the code under test when the harness is what did not hold. The test now writes a probe file into the directory it just made read-only. If that succeeds, the environment does not enforce the mode this fixture needs and the test skips with that reason instead of failing. The fixture assertion also carries write-tree's own status and output now, so the next surprise explains itself rather than needing a CI archaeology run.
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.
The premise nothing guaranteed
The warn-and-continue test breaks
git write-treeby chmodding.git/objectsto 0500, then asserts the break happened before exercising the branch it exists
to cover.
Git returns an object it already has without writing it. So if the staged tree
had reached the object store before the chmod,
write-treeexits 0 under aread-only store and the fixture assertion fails — the test reports a failure
that has nothing to do with the behaviour under test.
Reproduced directly: write the tree once with the store writable, chmod to 0500,
run
write-treeagain, exit 0.The change
The staged file opens with a unique line, so the tree it produces cannot already
be in the store.
The failure is also checked for its reason, not only its exit status. Any other
breakage — a missing binary, a corrupt index, a different permission problem —
satisfied the old assertion while testing something else entirely. Writing that
check is what surfaced git's actual wording here:
insufficient permission for adding an object to repository database, which is neither of the phrasings Iwould have guessed.
What this does not claim
This test failed once in a full local run, passed in isolation 25/25, and passed
on re-run. I ran the full suite repeatedly to capture the assertion message and
did not reproduce it in the runs that completed.
So the pre-existing-object path is the fragility I could prove, not a proven
cause of that particular failure. The two other assertions in the test look
robust by inspection:
write-treefailures are caught at both call sites(
bin/aimhooman.mjs:337,src/guard.mjs:91), andhashBlobuseshash-object --stdinwithout-w, so nothing else in the precommit path needsto write an object.
Full suite green,
npm run checkgreen.