Correct injected-floor test explanation (#83) - #84
Merged
Conversation
The comment claimed the injected floor could only sit inside the implemented range once the descriptor-limit bump widened it, and that the preceding range was a point. It was already 3..=4 before that bump, which is what `check_format_version` says: values inside the range have been reachable since manifest format version 4. The test behaviour was right; only its history was wrong. The floor the test injects is the producer's version, which is the top of the window rather than strictly interior, so the contrast the comment draws is with a floor above the whole window — where the accepted set is empty — and not with one at the top. Closes #83 Part of #78
Contributor
Author
|
[Reviewer Round 1] Approved — I found no review findings. The edit at src/verify.rs accurately removes the false descriptor-limit history and explains the meaningful distinction: a floor above the build minimum but no higher than its maximum leaves an accepted set, unlike one above the window. It agrees with The test construction and assertions remain unchanged: it still rejects the manifest at |
Contributor
Author
|
[Review Verdict Round 1: APPROVED] |
5 tasks
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.
Summary
an_injected_floor_inside_the_implemented_range_refuses_only_below_itselfproved the right thing but explained it with the wrong history. Its comment said the injected-floor-inside-the-range case became reachable only once thelimit_nofilebump widened the build's window, and that the preceding accepted range was a point. Neither is true: the range was already3..=4before that bump, which is exactly whatcheck_format_version's rustdoc says — values inside the implemented range have been observable since manifest format version 4.The comment now states that the window has spanned more than one version since manifest format version 4, and draws the contrast the test actually exercises: a floor sitting inside the window (above the bottom, no higher than the top, leaving versions the build still accepts) versus one above the whole window, where the accepted set is empty. The floor this test injects is the producer's version — the top of the window rather than a strictly interior value — so the prose no longer implies otherwise.
The test's package construction, injected floor, and acceptance/refusal assertions are unchanged; the diff is four comment lines in
src/verify.rs. No constant, parser, verifier, or descriptor-limit behavior was touched.Closes #83
Part of #78
Test plan
cargo test --features test-support an_injected_floor_inside_the_implemented_range_refuses_only_below_itselfpassescargo fmt -- --check --config group_imports=StdExternalCratecargo clippy --all-targets -- -D warningscargo clippy --all-targets --features test-support -- -D warningscargo testandcargo test --features test-supportcheck_format_version's rustdoc agree that an injected floor can sit inside the implemented range since manifest format version 4