Skip to content

docs: refresh diff_collect.rs module summary to match its coverage - #49

Open
cargo-affected-bot wants to merge 3 commits into
mainfrom
docs/diff-collect-module-doc-coverage
Open

docs: refresh diff_collect.rs module summary to match its coverage#49
cargo-affected-bot wants to merge 3 commits into
mainfrom
docs/diff-collect-module-doc-coverage

Conversation

@cargo-affected-bot

Copy link
Copy Markdown
Collaborator

The tests/functional/diff_collect.rs module doc was stale in two ways, both surfaced by the nightly rolling survey.

The summary claimed the file exercises "the two error paths the project's 'fail loudly' stance demands: no prior collect, and a stored sha no longer reachable from HEAD." Neither half holds anymore:

  • A single unreachable sha is not an error. plan_diff_collect emits a notice and proceeds as long as one sha is reachable (src/collect.rs:586); the bail! fires only when every stored sha is unreachable — a case this file doesn't cover. Commit Treat sibling collect_shas as reachable; only missing-from-repo bails #9 ("Treat sibling collect_shas as reachable; only missing-from-repo bails") made sibling shas reachable rather than fatal, and diff_collect_succeeds_when_sha_is_sibling now asserts exactly that success.
  • The file covers far more than two error paths. It has grown to ten tests covering re-anchoring, multi-sha accumulation, deleted/renamed-test pruning, ignored-test retention, all-phantom recovery, clean-tree short-circuit, and the run-side behaviors that ride on the multi-sha state (run_uses_reachable_shas_when_one_sha_diverges, run_unions_affected_and_stranded_when_partially_diverged). The only hard error actually exercised here is no-prior-collect.

This rewrites the summary to describe what the file tests today.

No test change is included — the edit is a doc comment in the test module itself, so the existing ten tests are the regression coverage for the behaviors it now describes.

The module doc claimed the file exercises "the two error paths the
project's 'fail loudly' stance demands: no prior collect, and a stored
sha no longer reachable from HEAD." Both halves are stale:

- A single unreachable sha is not an error. plan_diff_collect only emits
  a notice and proceeds as long as one sha is reachable (collect.rs:586);
  the bail fires only when *every* sha is unreachable — a case this file
  doesn't cover. Commit #9 made sibling shas reachable rather than fatal,
  and diff_collect_succeeds_when_sha_is_sibling now asserts exactly that.
- The file has grown to ten tests covering pruning, ignored-test
  retention, all-phantom recovery, clean-tree short-circuit, and the
  run-side multi-sha behaviors — far more than "headline guarantee + two
  error paths." Only the no-prior-collect path is an actual hard error
  exercised here.

Rewrite the summary to describe what the file actually tests.
@cargo-affected-bot

Copy link
Copy Markdown
Collaborator Author

Pushed 7a44b44 to reword one clause. The summary this PR added said "a single diverged sha only strands its own tests", which isn't right: reachability is git cat-file -e, not ancestry, so a diverged-but-still-present commit is Reachable and its tests come back as ordinary affected hits. Only a sha that's actually gone from the object database strands anything.

The clause now names the missing case and notes the distinction. #75 fixes the same confusion in the two run scenarios' own doc comments further down the file — disjoint hunks, so the two merge cleanly in either order.

@cargo-affected-bot cargo-affected-bot left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One claim in the new summary isn't backed by a test in this file, which matters given the PR's premise is that the summary should describe today's coverage.

The clause "a sha that has dropped out of the object database strands only the tests anchored there instead of widening the run" — no scenario here produces a Missing sha. Both run scenarios orphan sha1 with git reset --hard, which leaves the commit in the object database, and relation_to_head classifies Missing purely on git cat-file -e failing (src/project.rs, the cat-file -e {sha}^{{commit}} probe). So sha1 stays Reachable in both, nothing is stranded, and run_unions_affected_and_stranded_when_partially_diverged's 2 tests to run passes with both tests classified affected. #75 exists precisely to make that scenario strand for real (it adds a reflog expire + git gc --prune=now) — so this sentence is accurate only after #75 lands, and if #49 merges first it reintroduces the same class of stale claim it set out to remove.

The inline suggestion drops the unsupported half and keeps the sibling case, which run_uses_reachable_shas_when_one_sha_diverges does assert (!contains("not in the repo"), !contains("running all tests")). It's phrased so it doesn't go stale in the other direction either — no negative claim about the missing case, so #75 can add that sentence back as part of its own change without contradicting this one. The hunks stay disjoint either way.

Everything else checks out against the file: ten tests, the six --diff behaviors listed all have a matching scenario, diff_collect_errors_with_no_prior_collect is indeed the only hard-error path, and the git cat-file -e/ancestry parenthetical matches relation_to_head.

Comment thread tests/functional/diff_collect.rs Outdated
@cargo-affected-bot

Copy link
Copy Markdown
Collaborator Author

Fair — that clause described behavior, not coverage, and this PR's whole premise is that the summary should track what the file actually tests. Both run scenarios here orphan sha1 with git reset --hard, which leaves the commit in the object database, so nothing on this branch produces a Missing sha.

a3cb292 drops it. The clause is now "a selection spanning several shas stays narrow instead of widening to the whole suite", which both scenarios do assert (!combined.contains("running all tests")).

The reachable-vs-missing distinction now lives entirely in #75, which adds the scenario that earns it. That also keeps the two PRs' hunks disjoint, so neither needs a rewording pass depending on merge order.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant