test(xtask): make skipped fidelity cells a counted, reason-carrying outcome - #7248
Draft
oferchen wants to merge 2 commits into
Draft
test(xtask): make skipped fidelity cells a counted, reason-carrying outcome#7248oferchen wants to merge 2 commits into
oferchen wants to merge 2 commits into
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.
Problem
cargo xtask validatecounted skipped cells but let them pass. A run thatexercised 5 of 239 cells printed
and exited 0. Nothing in the output or the exit status distinguished that
from a healthy run; the totals line simply got smaller.
The model
Upstream does not ban skipping - it makes a skip impossible to mistake for a
pass.
testsuite/rsync.fns:448:Three properties carry over: the reason reaches the operator, the reason is left
behind in a machine-readable artifact, and the outcome is distinct from both
pass and fail. What upstream leaves to the caller - noticing the skip count has
grown - is made a hard contract here, in the spirit of
tools/ci/known_failures.conf: a declared number, exceeding it fails the run.What landed
SkipLedgercollects every skipped cell with its reason, groups reasons byfrequency, and prints the count against its declared budget under a
=== skipped cells ===heading.whyskippedartifact is written to the work dir, one tab-separatedcheck<TAB>cell<TAB>reasonrecord per skip, named after upstream's file. Itis written even when nothing skipped - an empty file says "accounted, none",
which a missing file does not.
ExpectedSkips::DEFAULTis the declared budget. Exceeding it fails the runwith a diagnostic naming the count, the budget, and every reason with its
cell count.
The measurement that changed the design
Re-deriving the numbers from the raw logs corrected an earlier claim of mine
and exposed a hole in the first version of this gate.
A skip budget alone cannot catch the macOS defect (#371). Measured:
touch(stock macOS)touchon PATHThe stock-macOS run does not skip 184 cells - those cells stop existing.
When a check's fixture setup fails, some checks collapse the transports they
would have run into a single aggregate skip, so the cells are counted nowhere:
not passed, not failed, not skipped. A skip ledger never sees them.
Separately, the 127 skips in the GNU-touch run are not the touch problem at
all - 118 of them are
no sshd on localhost:22. The touch defect and the skipvolume are two different things, and conflating them was an error in my earlier
report.
So the accounting contract is two gates, not one:
ExpectedSkips::DEFAULT- the skip budget.EXPECTED_CELLS- a floor on cells accounted for, checked separately. Afloor rather than an equality because
--flagsand--edge-caseslegitimately add cells; nothing legitimately removes them. Not applied to a
narrowed run (
--transport), which produces fewer cells by design.Without the second gate this change would have shipped a check that appears to
validate and does not - the exact class it exists to close.
Proof
All three scenarios surface as counted, reason-carrying outcomes and trip a
gate. Under
cargo nextest run --workspace --all-features -E 'test(skips::)':12 tests run: 12 passed.
Both directions. Neutering exactly the two production gates (
exceeds_budgetalways false,
cell_shortfallalwaysNone) turns exactly the three tests thatassert them red:
Restored: 12/12.
End-to-end on the real macOS shapes - the run that previously exited 0 now
exits 1:
Verification
cargo fmt --all -- --checkclean.cargo clippy --locked --workspace --all-targets --all-features --no-deps -- -D warningsclean. No CI (outage);local only, on macOS/aarch64.
Stated limits
EXPECTED_CELLS = 239is measured on macOS/aarch64 only. If Linuxlegitimately produces a different cell count the floor will fire there and the
constant needs re-measuring, possibly per-platform. This is called out in the
doc comment on the constant.
xtask validateruns in no CI workflow. These gates fire when the matrixis run, which today is a local pre-push step only. The nextest tests keep the
gate logic honest in CI, but nothing runs the matrix itself in CI. That is a
separate gap worth its own task.
the gate. It is now loudly red instead of silently green. Sizing for whoever
picks it up: the
touchshell-out is not one helper -"touch"appearsacross 42 files under
xtask/src/commands/validate/, so moving to thefiletimecrate is a 42-file change, not a one-liner.no sshd on localhost:22vsno sshd), which splits one cause across two rows in the grouping. Cosmetic,left alone.