-
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
test: migrate cfg and check to snapbox #14185
Conversation
eabce0d
to
2557a32
Compare
fb8f53b
to
3596042
Compare
.run(); | ||
} | ||
|
||
#[allow(deprecated)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with_stderr_does_not_contain
is being used in this function.
tests/testsuite/check.rs
Outdated
.with_stderr_data( | ||
format!( | ||
"\ | ||
[WARNING] unused manifest key: dependencies.dep.wxz | ||
[WARNING] unused manifest key: dependencies.foo.abc | ||
[WARNING] unused manifest key: dev-dependencies.foo.wxz | ||
[WARNING] unused manifest key: build-dependencies.foo.wxz | ||
[WARNING] unused manifest key: target.bar.build-dependencies.foo.wxz | ||
[WARNING] unused manifest key: target.cfg(windows).dependencies.foo.wxz | ||
[WARNING] unused manifest key: target.x86_64-pc-windows-gnu.dev-dependencies.foo.wxz | ||
[UPDATING] `[..]` index | ||
[WARNING] unused manifest key: target.{}.dev-dependencies.foo.wxz | ||
[UPDATING] `dummy-registry` index | ||
[LOCKING] 3 packages to latest compatible versions | ||
[DOWNLOADING] crates ... | ||
[DOWNLOADED] foo v0.1.0 ([..]) | ||
[DOWNLOADED] dep v0.1.0 ([..]) | ||
[CHECKING] [..] | ||
[CHECKING] [..] | ||
[CHECKING] bar v0.2.0 ([CWD]) | ||
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [..] | ||
[DOWNLOADED] foo v0.1.0 (registry `dummy-registry`) | ||
[DOWNLOADED] dep v0.1.0 (registry `dummy-registry`) | ||
[CHECKING] foo v0.1.0 | ||
[CHECKING] dep v0.1.0 | ||
[CHECKING] bar v0.2.0 ([ROOT]/foo) | ||
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s | ||
", | ||
if cfg!(all(target_os = "windows", target_env = "gnu")) { | ||
"[HOST_TARGET]" | ||
} else { | ||
"x86_64-pc-windows-gnu" | ||
} | ||
) | ||
.unordered(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
x86_64-pc-windows-gnu
is being modified to [HOST_TARGET]
on windows with gnu. 😅
FYI: https://github.com/rust-lang/cargo/actions/runs/9786239334/job/27020737537#step:11:4049
--- Expected
++++ actual: stderr
1 1 | [WARNING] unused manifest key: dependencies.dep.wxz
2 2 | [WARNING] unused manifest key: dependencies.foo.abc
3 3 | [WARNING] unused manifest key: dev-dependencies.foo.wxz
4 4 | [WARNING] unused manifest key: build-dependencies.foo.wxz
5 5 | [WARNING] unused manifest key: target.bar.build-dependencies.foo.wxz
6 6 | [WARNING] unused manifest key: target.cfg(windows).dependencies.foo.wxz
7 - [WARNING] unused manifest key: target.x86_64-pc-windows-gnu.dev-dependencies.foo.wxz
8 7 | [UPDATING] `dummy-registry` index
9 8 | [LOCKING] 3 packages to latest compatible versions
10 9 | [DOWNLOADING] crates ...
11 10 | [DOWNLOADED] foo v0.1.0 (registry `dummy-registry`)
12 11 | [DOWNLOADED] dep v0.1.0 (registry `dummy-registry`)
13 12 | [CHECKING] foo v0.1.0
14 13 | [CHECKING] dep v0.1.0
15 14 | [CHECKING] bar v0.2.0 ([ROOT]/foo)
16 15 | [FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s
16 + [WARNING] unused manifest key: target.[HOST_TARGET].dev-dependencies.foo.wxz
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can just change to something like wasm32-wasip1 that would never conflict?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea!
tests/testsuite/check.rs
Outdated
.with_stderr_data(str![[r#" | ||
[CHECKING] foo v0.0.1 ([ROOT]/foo) | ||
[ERROR] traits in `#[derive(...)]` don't accept arguments | ||
--> src/lib.rs:2:15 | ||
| | ||
2 | #[derive(Debug(x))] | ||
| ^^^ [HELP] remove the arguments | ||
|
||
[WARNING] unused import: `std::io` | ||
--> src/lib.rs:1:5 | ||
| | ||
1 | use std::io; | ||
| ^^^^^^^ | ||
| | ||
= [NOTE] `#[warn(unused_imports)]` on by default | ||
|
||
[WARNING] `foo` (lib) generated 1 warning | ||
[ERROR] could not compile `foo` (lib) due to 1 previous error; 1 warning emitted | ||
|
||
"#]]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: This change removes tests with messages originating from raw_rustc_output
and directly compares them with snapshots. Please let me know if this approach is inappropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like eventually raw_rustc_output can be removed. We should probably omit rustc diagnostics and only asset error code part.
___[EXE] | ||
lib___.rlib |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason we start checking these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, since it's a snapshot, I'm trying to minimize the use of glob redactions.
tests/testsuite/check.rs
Outdated
[LOCKING] 2 packages to latest compatible versions | ||
[CHECKING] bar v0.1.0 ([ROOT]/bar) | ||
[CHECKING] foo v0.0.1 ([ROOT]/foo) | ||
error[E0425]: cannot find function `qux` in crate `bar` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I remember correctly, this changed recently because rustc diagnostic message has changed. I would suggest omitting the actual message instead.
(Got an impression that in one PR I suggested redacting the message and only comparing error codes but it haven't yet done.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you suggesting that we simply check for errors like error[E0425]: [..]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, unless you also want to add a redaction of the main error message
tests/testsuite/check.rs
Outdated
.with_stderr_data( | ||
format!( | ||
"\ | ||
[WARNING] unused manifest key: dependencies.dep.wxz | ||
[WARNING] unused manifest key: dependencies.foo.abc | ||
[WARNING] unused manifest key: dev-dependencies.foo.wxz | ||
[WARNING] unused manifest key: build-dependencies.foo.wxz | ||
[WARNING] unused manifest key: target.bar.build-dependencies.foo.wxz | ||
[WARNING] unused manifest key: target.cfg(windows).dependencies.foo.wxz | ||
[WARNING] unused manifest key: target.x86_64-pc-windows-gnu.dev-dependencies.foo.wxz | ||
[UPDATING] `[..]` index | ||
[WARNING] unused manifest key: target.{}.dev-dependencies.foo.wxz | ||
[UPDATING] `dummy-registry` index | ||
[LOCKING] 3 packages to latest compatible versions | ||
[DOWNLOADING] crates ... | ||
[DOWNLOADED] foo v0.1.0 ([..]) | ||
[DOWNLOADED] dep v0.1.0 ([..]) | ||
[CHECKING] [..] | ||
[CHECKING] [..] | ||
[CHECKING] bar v0.2.0 ([CWD]) | ||
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [..] | ||
[DOWNLOADED] foo v0.1.0 (registry `dummy-registry`) | ||
[DOWNLOADED] dep v0.1.0 (registry `dummy-registry`) | ||
[CHECKING] foo v0.1.0 | ||
[CHECKING] dep v0.1.0 | ||
[CHECKING] bar v0.2.0 ([ROOT]/foo) | ||
[FINISHED] `dev` profile [unoptimized + debuginfo] target(s) in [ELAPSED]s | ||
", | ||
if cfg!(all(target_os = "windows", target_env = "gnu")) { | ||
"[HOST_TARGET]" | ||
} else { | ||
"x86_64-pc-windows-gnu" | ||
} | ||
) | ||
.unordered(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can just change to something like wasm32-wasip1 that would never conflict?
tests/testsuite/check.rs
Outdated
.with_stderr_data(str![[r#" | ||
[CHECKING] foo v0.0.1 ([ROOT]/foo) | ||
[ERROR] traits in `#[derive(...)]` don't accept arguments | ||
--> src/lib.rs:2:15 | ||
| | ||
2 | #[derive(Debug(x))] | ||
| ^^^ [HELP] remove the arguments | ||
|
||
[WARNING] unused import: `std::io` | ||
--> src/lib.rs:1:5 | ||
| | ||
1 | use std::io; | ||
| ^^^^^^^ | ||
| | ||
= [NOTE] `#[warn(unused_imports)]` on by default | ||
|
||
[WARNING] `foo` (lib) generated 1 warning | ||
[ERROR] could not compile `foo` (lib) due to 1 previous error; 1 warning emitted | ||
|
||
"#]]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like eventually raw_rustc_output can be removed. We should probably omit rustc diagnostics and only asset error code part.
All feedback addressed! Thanks :) |
Thank you. @bors r+ |
☀️ Test successful - checks-actions |
This is partial revert of #rust-lang#14185 (fd8a91d) People are unlikely to see these comments when updating snapshots. Although the changes made in that commit are valid, for the same reason mentioned in the comment rust-lang#14181 (comment), "People are unlikely to see these comments when updating snapshots". Therefore, it would be better to revert these parts.
This is partial revert of rust-lang#14185 (2706247) People are unlikely to see these comments when updating snapshots. Although the changes made in that commit are valid, for the same reason mentioned in the comment rust-lang#14181 (comment), "People are unlikely to see these comments when updating snapshots". Therefore, it would be better to revert these parts.
This is partial revert of rust-lang#14185 (2706247) Although the changes made in that commit are valid, for the same reason mentioned in the comment rust-lang#14181 (comment), "People are unlikely to see these comments when updating snapshots". Therefore, it would be better to revert these parts.
…r=weihanglo fix(test): Restore `does_not_contain` for check This is partial revert of #14185 (2706247) Although the changes made in that commit are valid, for the same reason mentioned in the comment #14181 (comment), "People are unlikely to see these comments when updating snapshots". Therefore, it would be better to revert these parts. --- Let me know if anything's missing!
Update cargo 13 commits in a515d463427b3912ec0365d106791f88c1c14e1b..f86ce56396168edf5d0e1c412ddda0b2edba7d46 2024-07-02 20:53:36 +0000 to 2024-07-05 17:52:05 +0000 - test: Migrate jobserver to snapbox (rust-lang/cargo#14191) - chore(deps): update msrv (3 versions) to v1.77 (rust-lang/cargo#14186) - test: migrate build_plan and build_script to snapbox (rust-lang/cargo#14193) - test: migrate cfg and check to snapbox (rust-lang/cargo#14185) - test: migrate install* and inheritable_workspace_fields to snapbox (rust-lang/cargo#14170) - Pass rustflags to artifacts built with implicit targets when using target-applies-to-host (rust-lang/cargo#13900) - test: Migrate network tests to snapbox (rust-lang/cargo#14187) - test: migrate some files to snapbox (rust-lang/cargo#14113) - test: Auto-redact `... after last build at ...`; Migrate `freshness` to Snapbox (rust-lang/cargo#14161) - chore: fix some typos (rust-lang/cargo#14182) - fix: improve message for inactive weak optional feature with edition2024 through unused dep collection (rust-lang/cargo#14026) - test:migrate `doc/directory/docscrape` to snapbox (rust-lang/cargo#14171) - test: Migrate git_auth to snapbox (rust-lang/cargo#14172)
Update cargo 20 commits in a515d463427b3912ec0365d106791f88c1c14e1b..154fdac39ae9629954e19e9986fd2cf2cdd8d964 2024-07-02 20:53:36 +0000 to 2024-07-07 01:28:23 +0000 - test: relax redactions for rust-lang/rust (rust-lang/cargo#14203) - use "bootstrap" instead of "rustbuild" (rust-lang/cargo#14207) - test: migrate serveral files to snapbox (rust-lang/cargo#14180) - Add rustdocflags to Unit's Debug impl (rust-lang/cargo#14201) - Allow enabling `config-include` feature in config (rust-lang/cargo#14196) - fix(test): Restore `does_not_contain` for check (rust-lang/cargo#14198) - test: migrate patch, pkgid, proc_macro and progress to snapbox (rust-lang/cargo#14181) - test: Migrate jobserver to snapbox (rust-lang/cargo#14191) - chore(deps): update msrv (3 versions) to v1.77 (rust-lang/cargo#14186) - test: migrate build_plan and build_script to snapbox (rust-lang/cargo#14193) - test: migrate cfg and check to snapbox (rust-lang/cargo#14185) - test: migrate install* and inheritable_workspace_fields to snapbox (rust-lang/cargo#14170) - Pass rustflags to artifacts built with implicit targets when using target-applies-to-host (rust-lang/cargo#13900) - test: Migrate network tests to snapbox (rust-lang/cargo#14187) - test: migrate some files to snapbox (rust-lang/cargo#14113) - test: Auto-redact `... after last build at ...`; Migrate `freshness` to Snapbox (rust-lang/cargo#14161) - chore: fix some typos (rust-lang/cargo#14182) - fix: improve message for inactive weak optional feature with edition2024 through unused dep collection (rust-lang/cargo#14026) - test:migrate `doc/directory/docscrape` to snapbox (rust-lang/cargo#14171) - test: Migrate git_auth to snapbox (rust-lang/cargo#14172)
What does this PR try to resolve?
Part of #14039.
Migrate following to snapbox:
tests/testsuite/cfg.rs
tests/testsuite/check.rs