-
Notifications
You must be signed in to change notification settings - Fork 14k
bootstrap: Add snapshot tests for path-to-step handling #148424
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
It’s worrying that CI sees different snapshots. I wonder if the “real” host/target is bleeding through somehow? |
|
Oh gross, Still not sure what's going on with the rustdoc JS tests; perhaps something is detecting that I don't have the requisite paths set on my machine? |
|
Yeah, looks like those steps auto-skip themselves when |
This comment has been minimized.
This comment has been minimized.
|
Thanks for working on this! This looks great. I wonder if we should just use out-of-band snap files also for the "normal" snapshot tests? It seems a bit weird to have two systems (in-band and out-of-band) in bootstrap.
That's a problem, right? Because if someone does/doesn't have |
|
Right, that’s why the test overwrites (E.g. I currently don’t have node on my path, which is why I had to add that override, so that my machine would generate the same snapshots as a CI machine that does have node installed.) |
|
Sorry, I didn't realize that you overwrite it after parsing the config, that makes sense. The paths will probably have to be blessed from time to time when e.g. rustc crates change, but that's not that common, I suppose. In general, I like this approach. You can r=me, unless you want to e.g. add more tests in this PR, or something. |
|
Adding more tests later is pretty easy, so I'm happy to get the plumbing and current tests merged for now. @bors r=Kobzol |
|
Very cool 👍 |
bootstrap: Add snapshot tests for path-to-step handling This PR adds a suite of snapshot tests for how bootstrap translates command-line “paths” (which are not necessarily actual paths) into a set of top-level steps to execute. Unlike the existing suite of snapshot tests (for transitive step logging), I decided to use `.snap` files over inline snapshots, because I find that inline snapshots quickly make test files impossible to navigate. Instead, I set up a macro that makes it relatively easy to add or modify test cases. Using `.snap` files also means that the tests can be blessed by setting `INSTA_UPDATE=always`, without necessarily needing the `cargo insta` tool installed, though the tool can still be used instead if desired. These snapshot tests capture _current_ behavior, to prevent unintended changes or regressions. If the current behavior is wrong or undersirable, then any fix will necessarily have to re-bless the affected tests! r? Kobzol
bootstrap: Add snapshot tests for path-to-step handling This PR adds a suite of snapshot tests for how bootstrap translates command-line “paths” (which are not necessarily actual paths) into a set of top-level steps to execute. Unlike the existing suite of snapshot tests (for transitive step logging), I decided to use `.snap` files over inline snapshots, because I find that inline snapshots quickly make test files impossible to navigate. Instead, I set up a macro that makes it relatively easy to add or modify test cases. Using `.snap` files also means that the tests can be blessed by setting `INSTA_UPDATE=always`, without necessarily needing the `cargo insta` tool installed, though the tool can still be used instead if desired. These snapshot tests capture _current_ behavior, to prevent unintended changes or regressions. If the current behavior is wrong or undersirable, then any fix will necessarily have to re-bless the affected tests! r? Kobzol
Rollup of 12 pull requests Successful merges: - #146627 (Simplify `jemalloc` setup) - #147753 (Suggest add bounding value for RangeTo) - #147974 (Improve diagnostics for buffer reuse with borrowed references) - #148080 ([rustdoc] Fix invalid jump to def macro link generation) - #148424 (bootstrap: Add snapshot tests for path-to-step handling) - #148500 (Update git index before running diff-index) - #148536 (cmse: add test for `async` and `const` functions) - #148770 (implement `feature(c_variadic_naked_functions)`) - #148819 (Remove specialized warning for removed target) - #148830 (miri subtree update) - #148833 (Update rustbook dependencies) - #148841 (Remove more `#[must_use]` from portable-simd) r? `@ghost` `@rustbot` modify labels: rollup
|
Failed in rollup: #148845 (comment) @bors r- |
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
I added some quick-and-dirty backslash normalization to avoid failures on Windows, and also added a handful of extra tests related to @bors try jobs=x86_64-msvc-2,test-various |
This comment has been minimized.
This comment has been minimized.
bootstrap: Add snapshot tests for path-to-step handling try-job: x86_64-msvc-2 try-job: test-various
|
@rustbot ready |
|
@bors r+ |
|
☀️ Test successful - checks-actions |
What is this?This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.Comparing 0b329f8 (parent) -> 95aeb46 (this PR) Test differencesShow 39 test diffsStage 0
Additionally, 1 doctest diff were found. These are ignored, as they are noisy. Job group index Test dashboardRun cargo run --manifest-path src/ci/citool/Cargo.toml -- \
test-dashboard 95aeb4696545eb4c9cbb68516f2912770e3846ea --output-dir test-dashboardAnd then open Job duration changes
How to interpret the job duration changes?Job durations can vary a lot, based on the actual runner instance |
This PR adds a suite of snapshot tests for how bootstrap translates command-line “paths” (which are not necessarily actual paths) into a set of top-level steps to execute.
Unlike the existing suite of snapshot tests (for transitive step logging), I decided to use
.snapfiles over inline snapshots, because I find that inline snapshots quickly make test files impossible to navigate. Instead, I set up a macro that makes it relatively easy to add or modify test cases.Using
.snapfiles also means that the tests can be blessed by settingINSTA_UPDATE=always, without necessarily needing thecargo instatool installed, though the tool can still be used instead if desired.These snapshot tests capture current behavior, to prevent unintended changes or regressions. If the current behavior is wrong or undersirable, then any fix will necessarily have to re-bless the affected tests!
r? Kobzol