-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Port from bespoke assertions of snapbox #14039
Comments
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
#14041 highlights a blocker for |
This comment was marked as resolved.
This comment was marked as resolved.
test: migrate build_script_env to snapbox ### What does this PR try to resolve? part of #14039.
test: migrate features_are_quoted to snapbox ### What does this PR try to resolve? Part of #14039. Migrate `tests/testsuite/shell_quoting.rs` to snapbox. ### How should we test and review this PR? N/A ### Additional information N/A
This comment was marked as resolved.
This comment was marked as resolved.
Migrate a few test files to snapbox This migrates the following files to `snapbox` - `artifact_dep` - Has a few `does_not_contain` - `artifact_dir` - `bad_config` - `bad_manifest_path` - Does not use `str!` for all tests - `bench` Note: This also adds auto-redactions for: - `[HOST_TARGET]` - `[ALT_TARGET]` - Only added if cross-compilation is allowed for the target - `[AVG_ELAPSED]` - For `bench` output - `[JITTER]` - For `bench` output Part of #14039
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
test: Migrate tests/testsuite/co*.rs to snapbox Migrating files: - tests/testsuite/collisions.rs - `with_stderr_does_not_contain` in test `collision_doc_host_target_feature_split` - tests/testsuite/concurrent.rs - tests/testsuite/config.rs - tests/testsuite/config_cli.rs - tests/testsuite/config_include.rs - tests/testsuite/corrupt_git.rs Testing with command `SNAPSHOTS=overwrite cargo test collisions::` or so. Part of #14039
This comment was marked as resolved.
This comment was marked as resolved.
test: migrate implicit_features to snapbox ### What does this PR try to resolve? Part of #14039. Migrate following to snapbox: - `tests/testsuite/implicit_features.rs`
feat(test): Add cargo_test to test-support prelude ### What does this PR try to resolve? With where we are at with #14039, I was revisiting our test writing documentation. I was wanting to put more emphasis on rustdoc and found it would be helpful to talk about the concepts if a prelude was used instead of `extern crate`. ### How should we test and review this PR? Yes, this included some `use` clean ups in tangentially-related commit but its already painful enough walking through every test file, I didn't want to do it twice. ### Additional information
test: migrate fetch and list_availables to snapbox ### What does this PR try to resolve? Part of #14039. Migrate following to snapbox: - `tests/testsuite/fetch.rs` - `tests/testsuite/list_availables.rs`
Migrate global_cache_tracker snapbox Part of #14039. Some of these tests may require to assert specific removed file count or size that would be redact by current design. Need feedback to come out solution. Temporary commit to pass the tests and illustrate what need to fix.
I've created assert-rs/snapbox#348 to track what we need from snapbox for jsonlines. See that issue for my proposed solution. |
docs(test): Expand documentation of cargo-test-support ### What does this PR try to resolve? In wanting to document #14039, I felt it would be good to put that documentation in `cargo-test-support`. To do so, I wanted a baseline of existing documentation for it to build on top of. I was tempted to move more of "Writing tests" contrib documentation here, as its more about using `cargo-test-support` but I decided to hold off for now as most of that was long-form documentation and this is mostly focused on reference documentation. ### How should we test and review this PR? ### Additional information
I fixed assert-rs/snapbox#348 but found assert-rs/snapbox#351 and assert-rs/snapbox#352 |
test: Migrate some json tests to snapbox ### What does this PR try to resolve? This builds on assert-rs/snapbox#348 and is part of #14039. Note: this also updates existing `.is_jsonlines()` usage to `.is_json().against_jsonlines()`. ### How should we test and review this PR? ### Additional information
test: migrate messages to snapbox ### What does this PR try to resolve? Part of #14039. Migrate following to snapbox: - `tests/testsuite/messages.rs`
test: Migrate some json tests to snapbox ### What does this PR try to resolve? This is part of #14039 ### How should we test and review this PR? ### Additional information This was unblocked because of assert-rs/snapbox#350
This is part of rust-lang#14039
test: Migrate old_cargos to snapbox This is part of #14039
Looks like we've finished every file-level migration. We have 124 remaining |
This is part of rust-lang#14039
test: Migrate remaining with_stdout/with_stderr calls ### What does this PR try to resolve? This is part of #14039 and is another step towards us not needing our own redaction logic. Along the way, I switched us to using `expect` to make it easier to tell when `allow(deprecated)` should be removed. ### How should we test and review this PR? ### Additional information
Traditionally,
cargo-test-support
has had bespoke assertions, requiring hand implemented diff algorithms. As there is less of a community around this, the knowledge is more specialized, its less likely to be documented, and we are on our own for feature development.In #13980 and #14031, we introduced the use of snapbox as replacement for our own assertions and need to work to migrate to them.
Aside: doc updates related to this transition
Porting Instructions
1. Select a file to port
The files should have
#![allow(deprecated)]
at the top, e.g.Please check the comments on this issue for anyone to have claimed the file and then post that you claim the file
2. Remove
#![allow(deprecated)]
to identify what work is neededIf nothing, congrats, that was easy!
3. Resolve basic deprecations
Replace
Execs::with_stdout("...")
withReplace
Execs::with_stderr("...")
with(prelude is only needed for some steps)
Commit these changes
Run
SNAPSHOTS=overwrite cargo test && cargo check --test testsuite
. In rare cases,SNAPSHOTS=overwrite
may make bad edits. Feel free to create a reproduction case and create an issue. They are usually obvious how to fix, so don't worry.Diff the snapshots. Is there anything machine or run specific in them that previously was elided out with
[..]
?[..]
Once its working, amend your commit with the snapshots
4. Resolve non-literal deprecations
Like Step 3, but not just
with_stdout("...")
but with variables orformat!
.Evaluate whether a literal could be used
format!
for, then keep itSo this can end up with either
with_std*_data(expression)
with_std*_data(str![])
like in Step 35. Repeat with "straight forward" deprecations
with_stdout_unordered(expected)
->with_stdout_data(expected.unordered())
with_stderr_unordered(expected)
->with_stderr_data(expected.unordered())
with_json(expected)
->with_stdout_data(expected.json_lines())
orwith_stdout_data(expected.json())
.unordered()
6. Contains / Does not Contains deprecations
A judgement needs to be made for how to handle these.
A
contains
can be modeled by surrounding a entry with...
(text) or"...",
(json lines). The question is whether we should still do this or use a different method.contains
with an equality check, use multi-line globs (...
) for rustc errors that weren't previously matched to minimize tying Cargo's tests to the exact output of rustccontains
can be merged into a singleeq
with..
interspersed (or one...
and.unordered()
). For example, see 3054936unordered
, likely string literals, rather thanstr![]
should be used, as these can't correctly be updatedA
does_not_contain
/line_without
cannot be modeled at this time. The challenge with these is that they are brittle and you can't tell when they are no longer testing for what you think because the output changed. We should evaluate what to do with these on a case-by-case basis.When in doubt, feel free to put
#[allow(deprecated)]
on a statement and move on. We can come back to these later.The text was updated successfully, but these errors were encountered: