Skip to content
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.

Sign up for GitHub

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

Jump to bottom

test: migrate check_cfg to snapbox #14235

Merged
merged 1 commit into from
Jul 12, 2024
Merged

test: migrate check_cfg to snapbox #14235

merged 1 commit into from
Jul 12, 2024

Conversation

eth3lbert
Copy link
Contributor

What does this PR try to resolve?

Part of #14039.

Migrate following to snapbox:

  • tests/testsuite/check_cfg.rs

This commit allow deprecation for most of the tests currently in this file. This is because the tests either use does_not_contain or perform multiple contains assertion for single tool with macro x!.

Here are some potential options to further eliminate the deprecation:

  1. Remove the x! macro and replace the it with str![] for both Windows and non-Windows platforms.
  2. Modify the x! macro to only generate the --check-cfg part for different platforms and manually compose it with format!.

@rustbot
Copy link
Collaborator

rustbot commented Jul 10, 2024

r? @ehuss

rustbot has assigned @ehuss.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 10, 2024
@eth3lbert
Copy link
Contributor Author

r? @epage

@rustbot rustbot assigned epage and unassigned ehuss Jul 10, 2024
@eth3lbert
Copy link
Contributor Author

Failed in CI with the following:

---- expected: tests/testsuite/lto.rs:372:13
++++ actual:   stderr
   1    1 | [COMPILING] foo v0.0.0 ([ROOT]/foo)
   2    2 | [RUNNING] `rustc --crate-name a [..]-C lto [..]`
   3    3 | [RUNNING] `rustc --crate-name b [..]-C lto [..]`
   4    4 | [RUNNING] `rustc --crate-name foo [..]-C lto [..]--test [..]`
   5    5 | [RUNNING] `rustc --crate-name foo [..]--crate-type bin [..]-C lto [..]`
   6      - [FINISHED] `release` profile [optimized] target(s) in [ELAPSED]s
   7    6 | [RUNNING] `[ROOT]/foo/target/release/deps/foo-[HASH][EXE]`
   8    7 | [RUNNING] `[ROOT]/foo/target/release/deps/a-[HASH][EXE]`
   9    8 | [RUNNING] `[ROOT]/foo/target/release/deps/b-[HASH][EXE]`
        9 + [FINISHED] `release` profile [optimized] target(s) in 1m 00s

This should be resolved once #14233 is merged.

@eth3lbert eth3lbert force-pushed the snapbox-check-cfg branch from aae4e9a to 58458cd Compare July 10, 2024 23:00
@epage
Copy link
Contributor

epage commented Jul 11, 2024

#14233 is now merged.

epage
epage reviewed Jul 11, 2024
View reviewed changes
tests/testsuite/check_cfg.rs
@@ -256,6 +269,7 @@ fn features_test() {
.run();
}

#[allow(deprecated)]
#[cargo_test]
fn features_doctest() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the does_not_contains could be removed. If we were passing -Z, it would fail as this test runs on stable

@epage
Copy link
Contributor

epage commented Jul 11, 2024

Remove the x! macro and replace the it with str![] for both Windows and non-Windows platforms.

Overall, not a fan of x!.

@epage
Copy link
Contributor

epage commented Jul 11, 2024

To add though, we can merge this now-ish (maybe remove some of the weirder does_not_contains) and further improve it in follow up PRs, rather than do it all at once.

@eth3lbert eth3lbert force-pushed the snapbox-check-cfg branch from 58458cd to 5a09fcb Compare July 11, 2024 01:39
@eth3lbert
Copy link
Contributor Author

To add though, we can merge this now-ish (maybe remove some of the weirder does_not_contains) and further improve it in follow up PRs, rather than do it all at once.

I remove all the -Z. However, if we only want to remove the one within the features_doctest function, please let me know!

@Urgau
Copy link
Member

Urgau commented Jul 11, 2024

The x! macro was added very early the development of the feature in Cargo and has always been awkward, it was meant as a simple way to create the cmd argument without relying to much on the actual argument; in part because the syntax was still evolving and changing. It also was a convenient way to handle the Windows vs non-Windows escaping.

It has serve us well, but since then many things have evolved, --check-cfg is now stable, Cargo has a new testing engine...

In other words, feel free to remove the macro.

@eth3lbert
Copy link
Contributor Author

The x! macro was added very early the development of the feature in Cargo and has always been awkward, it was meant as a simple way to create the cmd argument without relying to much on the actual argument; in part because the syntax was still evolving and changing. It also was a convenient way to handle the Windows vs non-Windows escaping.

It has serve us well, but since then many things have evolved, --check-cfg is now stable, Cargo has a new testing engine...

In other words, feel free to remove the macro.

Thanks for the explanation and the addition from the author's perspective.

@epage epage mentioned this pull request Jul 12, 2024
Closed
@epage
Copy link
Contributor

epage commented Jul 12, 2024

I'll merge as-is and we can handle x! in a follow up

@bors r+

@bors
Copy link
Contributor

bors commented Jul 12, 2024

📌 Commit 5a09fcb has been approved by epage

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 12, 2024
@bors
Copy link
Contributor

bors commented Jul 12, 2024

⌛ Testing commit 5a09fcb with merge 0e2ef80...

@bors
Copy link
Contributor

bors commented Jul 12, 2024

☀️ Test successful - checks-actions
Approved by: epage
Pushing 0e2ef80 to master...

@bors bors merged commit 0e2ef80 into rust-lang:master Jul 12, 2024
22 checks passed
@eth3lbert eth3lbert deleted the snapbox-check-cfg branch July 12, 2024 01:48
@weihanglo weihanglo mentioned this pull request Jul 17, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 17, 2024
@bors
Auto merge of rust-lang#127838 - weihanglo:update-cargo, r=weihanglo
cb12b52 
Update cargo

31 commits in 154fdac39ae9629954e19e9986fd2cf2cdd8d964..a2b58c3dad4d554ba01ed6c45c41ff85390560f2
2024-07-07 01:28:23 +0000 to 2024-07-16 00:52:02 +0000
- chore(ci): bump CI tools (rust-lang/cargo#14257)
- test: migrate fetch and list_availables to snapbox (rust-lang/cargo#14214)
- chore: downgrade to [email protected] (rust-lang/cargo#14254)
- perf(source): Don't `du` on every git source load (rust-lang/cargo#14252)
- fix(source): Don't warn about unreferenced duplicate packages (rust-lang/cargo#14239)
- feat(test): Add cargo_test to test-support prelude (rust-lang/cargo#14243)
- Add workflow to publish Cargo automatically (rust-lang/cargo#14202)
- test: migrate implicit_features to snapbox (rust-lang/cargo#14245)
- test: migrate build-std/main to snapbox (rust-lang/cargo#14241)
- test: migrate check_cfg to snapbox (rust-lang/cargo#14235)
- refactor(source): More RecursivePathSource clean up (rust-lang/cargo#14231)
- Add more profiling traces (rust-lang/cargo#14238)
- fix(overrides): Don't warn on duplicate packages from using '..' (rust-lang/cargo#14234)
- fix(test): Redact elapsed time in the minutes time frame (rust-lang/cargo#14233)
- test: Migrate lto tests to snapbox (rust-lang/cargo#14209)
- fix: Ensure dep/feature activates the dependency on 2024 (rust-lang/cargo#14221)
- chore(docs): update index of reference (rust-lang/cargo#14228)
- test: migrate test to snapbox (rust-lang/cargo#14226)
- chore: remove duplicate words (rust-lang/cargo#14229)
- docs(contrib): Document things I look for in RFCs (rust-lang/cargo#14222)
- docs(ref): Note MSRV for features in the docs (rust-lang/cargo#14224)
- test(progress): Resolve flakiness (rust-lang/cargo#14223)
- fix(test): Reduce over-prescription to the caller (rust-lang/cargo#14217)
- refactor: move get_source_id out of registry (rust-lang/cargo#14218)
- fix: rename to `rustdoc::broken_intra_doc_links` (rust-lang/cargo#14215)
- test: migrate member_errors, multitarget and new to snapbox (rust-lang/cargo#14210)
- test: migrate generate_lockfile and glob_targets to snapbox (rust-lang/cargo#14200)
- test: Ensure --list test works with cargo-bloat (rust-lang/cargo#14213)
- dont make new constant InternedString in hot path (rust-lang/cargo#14211)
- Fix compatible_with_older_cargo test. (rust-lang/cargo#14212)
- test: migrate metabuild, metadata and net_config to snapbox (rust-lang/cargo#14162)
@rustbot rustbot added this to the 1.81.0 milestone Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@epage epage epage left review comments

Assignees

@epage epage

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.81.0
Development

Successfully merging this pull request may close these issues.
6 participants
@eth3lbert @rustbot @epage @Urgau @bors @ehuss