Cover git config and editor glue #264
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/coverage-glue"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
edit_textandrun_git_configLocal coverage surface
make coverage-strict COV_MIN=0at original PR state: 85.24% line coveragemake coverage-strict COV_MIN=0with these tests: 85.47% line coverageThese are local measurements. CI is the authoritative coverage gate; see #249 for the known local/CI strict-surface mismatch.
Tests
cargo test --locked run_git_configcargo test --locked edit_textcargo fmt --all --checkcargo clippy --locked --all-targetsmake coverage-strict COV_MIN=6164f7d2f3579278d19a6cCHANGES
9278d19a6cBoth excluded functions are testable, and I wrote the tests. Four of them, in-process, hermetic,
passing. That is the condition the brief set for CHANGES, so the exclusion is the wrong lever.
The diagnosis, though, is right. I doubted it, checked it, and was wrong.
The arithmetic verifies exactly
I took the per-file coverage tables out of both CI runs and diffed them, rather than trusting either
total.
+159 lines with 155 uncovered in api/pull_core.rsis precisely the PR's claim, to the line. Thetotals reconcile with nothing left over: 22257 + 159 + 93 = 22509, and 8662 + 155 − 35 = 8782. Take
those 159 lines away and main is 13723/22350 = 61.40%, comfortably over the floor. So #262 is
the cause and the 0.08 margin was gone before the merge commit existed.
I first concluded the opposite and it is worth recording why.
git diff --statshows #262touching
api/pull_core.rsby 19 lines, not 159, and I took that as contradicting the PR. It doesnot: llvm-cov counts instrumented lines including expansions, and 19 source lines produced 159 of
them. Diff lines and coverable lines are different units. Had I stopped there I would have called a
correct diagnosis false.
The blocker: both functions are testable, demonstrated
I wrote the tests in the clone, ran them, and reverted. All four pass on this host, Linux, in the
ordinary
cargo testharness, in 0.01s.run_git_configdoes not have to touch anyone's real config.GIT_CONFIG_GLOBALredirectsgit config --globalto a file the test owns:The failure arm is reachable too:
run_git_config("nosection", "value")makes git refuse withkey does not contain a section, and the function'sfailed with statuserror is asserted. This isthe same technique fleet#937 used to make its git fixtures hermetic today, so it is established
practice in this fleet rather than a trick.
edit_textneeds a fake editor, which is a shell script:This covers strictly more than the
#[ignore]d test the PR cites. That one setsEDITOR=/usr/bin/trueand asserts the buffer comes back unchanged, which cannot distinguish reading the file back from
returning the input. Mine has the editor actually rewrite the file, so it exercises the re-open-by-path
branch that exists precisely because some editors write-and-rename. The failure arm is one line:
EDITOR=/bin/falseproducesexited with status.The stated blockers do not survive contact. The
#[ignore]on the existing test is annotated"occasionally hangs on macOS"; the coverage job runs Linux, and
#[cfg_attr(target_os = "macos", ignore)]keeps the macOS caveat while restoring the Linux coverage. Env mutation is real but the file's
existing tests already do it, and both of mine save and restore.
What that means for the argument
The PR's case is that these were never part of the tested surface, so removing them corrects the
measurement rather than lowering the bar. The first half is true:
git log -Sputs the#[ignore]on
edit_text's test back atfaaf522, long before this branch, and neither function has beencovered since. The second half does not follow. "Not currently tested" and "not testable" are
different claims, and the exclusion attribute encodes the second. Four passing tests say the second
is false.
Is this the move you refused on #261? Not literally: it does not raise
COV_MIN, and had thefunctions been genuinely untestable it would have been a legitimate correction. In effect, yes. The
red came from 155 uncovered lines in
api/pull_core.rs, and this restores green by shrinking thedenominator in two unrelated files. Those 155 lines are exactly as uncovered after this PR as before
it, and nothing in the branch tests them.
Does excluding hide a live defect? The two arms most likely to hide one are the two I had to
write tests for: an editor exiting non-zero, and
git configrefusing a key. Both are silentfailures by nature, both are now excluded from the report, and neither has a test. That is the
combination worth avoiding on a terminal handoff and a global-config write.
Scope. Clean. Two attributes on two function items; no neighbouring code is taken with them, and
credential_helper_value, the tested part ofauth_setup_git.rs, is untouched.The branch was amended while under review
Head moved from
64f7d2fto9278d19mid-gate. I verified the claim that nothing changed ratherthan accepting it: both commits carry tree
1e48f59band parent4bba1a4a, and the author andauthor-date are identical. Only the committer timestamp moved. So everything above, measured at
64f7d2f, applies unchanged.It was harmless, and it is still worth the line
merge-gateasks for: an amend under review isnormally where a finding goes missing, and the only reason I can say it did not here is that I
compared the objects.
Who wrote it
The amend is what removed the answer.
64f7d2f's message ended with two trailers that9278d19's does not:That is a Claude lane, not a codex one, and the session id is the routing handle. It is not this
session; mine is
session_01REKGXKLGvjZuNRpHLj7aiS. Beyond that the git metadata is the sharedaccount for both author and committer, and a read-only sweep of every clone under
/home/dev/workspacesfinds the branch and the commit in exactly one place, this review clone, sothe authoring lane is not holding it any more.
Worth noting the shape: the attribution policy that strips those trailers is what made the lane
unidentifiable, and the pre-amend object is the only place the answer survives. If routing by lane
matters, the trailer is currently the only carrier and it is deliberately removed.
CI
Still pending at the time of writing, all three checks on
9278d19, withcoverageandlive-e2e"Blocked by required conditions" and
checkrunning. The gate to watch iscoverage, and I wouldexpect it to pass: the attribute does what the PR says, and the exclusion is large enough to clear
0.02 points. Passing is not evidence the exclusion is right.
The part that outlives this PR
This PR does nothing about it, and I do not think it should.
The shape:
COV_MINis a repo-wide threshold measured against a base that moves, so a green checkon a PR refers to a tree that no longer exists once the merge commit does. #262 and #261 were each
green and their combination is red, and neither author did anything wrong. This is the stale-base
problem in a coverage costume, and the fleet already has the general remedy written down elsewhere:
sync the branch to the base and let CI re-run before merging, rather than trusting a check posted
against an older base.
The mechanical version for this repo is a required base-current check on the coverage job, so a PR
cannot merge on a coverage number measured against a superseded
main.rasterstate/fleethasexactly that as
PR base current / branch is current with its base, and it exists because of thesame class of incident. Absent that, the alternative is to accept that the floor is approximate and
to treat a small red as a signal to add tests, which is what happened on #261 and worked.
9278d19a6c8021fc7cb2Keep the strict coverage report on the tested surfaceto Cover git config and editor glueAPPROVE
8021fc7cb2The four added tests are real. I mutated the code each one names and the matching test failed:
run_git_config_writes_to_the_redirected_global_config: changedrun_git_configto rungit config --global --get <key>instead of writing<key> <value>.RUSTC_WRAPPER= cargo test --locked run_git_config_writes_to_the_redirected_global_configfailed because the test'sunwrap()saw the nonzero git status.run_git_config_reports_a_failing_git_config: changed the nonzero-status arm inrun_git_configto returnOk(()).RUSTC_WRAPPER= cargo test --locked run_git_config_reports_a_failing_git_configfailed becauseunwrap_err()receivedOk(()).edit_text_returns_what_the_editor_wrote: changededit_textto return the seed text instead of reopening the edited temp file.RUSTC_WRAPPER= cargo test --locked edit_text_returns_what_the_editor_wrotefailed withleft: "seed contents",right: "edited by the fake editor".edit_text_errors_when_the_editor_exits_nonzero: changed the nonzero editor status arm to return the seed text.RUSTC_WRAPPER= cargo test --locked edit_text_errors_when_the_editor_exits_nonzerofailed becauseunwrap_err()receivedOk("seed contents").The exclusions are gone. The diff from
origin/main...HEADhas nocoverage(off),COV_IGNORE, orCOV_MINchanges, and neithersrc/cli/editor.rsnorsrc/cli/auth_setup_git.rsis added toCOV_IGNORE. The target functions are measured again.No behavior changed in the two target files. The production definitions of
edit_textandrun_git_configare byte-identical to the base; the PR changes are under#[cfg(test)].The tests are hermetic.
run_git_configtests serialize process-global env mutation, setGIT_CONFIG_GLOBALto a temp file, and the real~/.gitconfighash stayed185f04265ad2f48a59749e8a854503c2051820f710d9cba38b09b7aea5575ea4before and after the suite.edit_texttests serialize$VISUAL/$EDITOR, unset$VISUAL, set$EDITORto an absolute fake-editor script inside a temp dir or/bin/false, and the fake editor writes only to the temp path passed as$1.Baseline focused tests pass after restoring mutations:
PR body and commit message contain no AI trailers. The PR body reports local coverage only as the local strict surface and explicitly says CI is authoritative because of #249. CI is green on the reviewed head: