Add requirements to cargo_test. #9892

ehuss · 2021-09-09T06:21:09Z

This extends the #[cargo_test] attribute to support some additional requirements to control whether or not a test can run. The motivation for this is:

Can more clearly see when a test is disabled when it prints "ignored"
Can more easily scan for disabled tests when I do version bumps to see which ones should be enabled on stable (to pass on CI).

The form is a comma separated list of requirements, and if they don't pass the test is ignored. The requirements can be:

nightly — The test only runs on nightly.
>=1.55 — The test only runs on rustc with the given version or newer.
requires_git — Requires the given command to be installed. Can be things like requires_rustfmt or requires_hg, etc.

This also enforces that the author must write a reason why it is ignored (for some of the requirements) so that when I look for tests to update, I know why it is disabled.

This also removes the CARGO_TEST_DISABLE_GIT_CLI option, which appears to no longer be necessary since we have migrated to GitHub Actions.

rust-highfive · 2021-09-09T06:21:12Z

r? @alexcrichton

(rust-highfive has picked a reviewer for you, use r? to override)

ehuss · 2021-09-09T06:21:41Z

I'm opening this up as a draft to get some feedback to see if this is a worthwhile direction to go. This is something I've wanted for a while, as it would help understand which tests are disabled. One issue is that there is some code duplication in cargo-test-macro that is hard to share (would need to create yet another crate to hold that shared code and keep build times down).

Ideally there would be a better way to have runtime test skipping (as described in https://internals.rust-lang.org/t/pre-rfc-skippable-tests/14611).

alexcrichton · 2021-09-09T14:31:02Z

I think this is a fine direction to go in. If possible I'd ideally like to leverage code generation to share code where the checks are in the generated code rather than the macro itself (so the macro doesn't have to do things like spawn rustc and such). This is generating a literal #[ignore] but is that necessary? Presumably you'd search for ignored tests with a regex rather than running tests with --ignored?

bors · 2021-09-09T15:36:20Z

☔ The latest upstream changes (presumably #9889) made this pull request unmergeable. Please resolve the merge conflicts.

weihanglo · 2021-12-22T06:16:16Z

As @ehuss said, the generation of #[ignored] can help us be more confident in which tests have been run and which haven't. I like the explicit, especially when tracing old logs (though I doubt the frequency of this scenario).

Yet, is it a problem that the tests and #[cargo_test] macro generation are run under different arch?

This appears to no longer be necessary since we have migrated to GitHub Actions.

ehuss · 2022-07-31T04:25:49Z

@epage or @weihanglo, I'd like to move forward with this. Would either of you be willing to review this?

I updated it for the latest master. There are also some docs in the contrib guide which explain what has changed.

weihanglo · 2022-07-31T04:52:26Z

I would take a look then. Thank you for updating this!

weihanglo

Looks nice!

I notice that there are two widely used requirement rules are not covered: symlink support and cross compilation. Do we want to integrate them into this proc macro? It could be done by a separate PR tho.

tests/testsuite/git.rs

src/doc/contrib/src/tests/writing.md

tests/testsuite/jobserver.rs

weihanglo · 2022-07-31T17:50:49Z

crates/cargo-test-macro/src/lib.rs

+    let mut ignore = false;
+    let mut requires_reason = false;
+    let mut found_reason = false;
+    let is_not_nightly = || !version().1;


I wonder why is_not_nightly is closure. Is it possible to call version() only once and reuse the return value?

Yea. I think I was thinking of avoiding the overhead of the atomic load for each test, but I suspect it is tiny.

From what I can tell, it is no longer necessary on GitHub Actions. This removes it to help simplify things.

weihanglo

Good to go. Thanks!

weihanglo · 2022-08-01T00:50:50Z

@bors r+

bors · 2022-08-01T00:50:51Z

📌 Commit 8e35e2f has been approved by weihanglo

It is now in the queue for this repository.

bors · 2022-08-01T00:51:04Z

⌛ Testing commit 8e35e2f with merge 2e35678...

bors · 2022-08-01T01:28:15Z

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 2e35678 to master...

Fix formats_source test requiring rustfmt. The requirements added in #9892 that `rustfmt` must be present doesn't work in the `rust-lang/rust` environment. There are two issues: * Cargo is run without using rustup. If you also have rustup installed, the test will fail because the `rustfmt` binary found in `PATH` will fail to choose a toolchain because HOME points to the sandbox home which does not have a rustup configuration. * rust-lang/rust CI uninstalls rustup, and does not have rustfmt in PATH at all. It is not practical to make rustfmt available there. The solution here is to just revert the behavior back to where it was where it checks if it can run `rustfmt` in the sandbox. This should work for anyone who has a normal rustup installation (including Cargo's CI). If running the testsuite without rustup, then the test will be skipped. This also includes a small enhancement to provide better error information when rustfmt fails.

Update cargo, rls 14 commits in 85b500ccad8cd0b63995fd94a03ddd4b83f7905b..4fd148c47e733770c537efac5220744945d572ef 2022-07-24 21:10:46 +0000 to 2022-08-03 15:03:52 +0000 - Revert "Drop check for mingw32-make." (rust-lang/cargo#10934) - Add reasons to all ignored tests. (rust-lang/cargo#10929) - Grammar fixup unused patch message (rust-lang/cargo#10933) - Always allow hg to be missing on CI. (rust-lang/cargo#10931) - Fix formats_source test requiring rustfmt. (rust-lang/cargo#10918) - Disable scrape_examples_complex_reverse_dependencies (rust-lang/cargo#10921) - Contrib: Add docs on the rustbot ready command (rust-lang/cargo#10916) - Support for negative --jobs parameter, counting backwards from max CPUs (rust-lang/cargo#10844) - Add requirements to cargo_test. (rust-lang/cargo#9892) - Contrib: Document submodule update process (rust-lang/cargo#10913) - Contrib: Add docs on how to use crater (rust-lang/cargo#10912) - Contrib: Document new-release process (rust-lang/cargo#10914) - Override to resolver=1 in published package (rust-lang/cargo#10911) - fix(add): Update the lock file (rust-lang/cargo#10902) 1 commits in fcf1f94c9ab2acc18cfd4368a4aeb38e77da9649..4d8b0a19986a4daab37287a5b5fe2da0775d1873 2022-07-14 17:19:11 +0200 to 2022-08-02 22:34:34 -0400 - Update cargo (rust-lang/rls#1782)

rust-highfive assigned alexcrichton Sep 9, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 9, 2021

ehuss marked this pull request as draft September 9, 2021 06:21

ehuss added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 12, 2021

alexcrichton removed their assignment Mar 8, 2022

weihanglo mentioned this pull request May 3, 2022

Migrate tests of cargo-init to snapbox #10620

Merged

ehuss force-pushed the cargo_test-ignore-reason branch 3 times, most recently from 4d26ed8 to aebea40 Compare July 30, 2022 18:42

ehuss added 2 commits July 30, 2022 19:36

Add requirements to cargo_test.

1c3640e

Remove CARGO_TEST_DISABLE_GIT_CLI

9d43ffc

This appears to no longer be necessary since we have migrated to GitHub Actions.

ehuss force-pushed the cargo_test-ignore-reason branch from 5c5b46e to 9d43ffc Compare July 31, 2022 02:37

ehuss marked this pull request as ready for review July 31, 2022 04:24

weihanglo self-requested a review July 31, 2022 04:52

weihanglo added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. labels Jul 31, 2022

weihanglo reviewed Jul 31, 2022

View reviewed changes

ehuss added 4 commits July 31, 2022 15:19

Add missing requires_git requirements.

bcf982c

Fix typos in cargo_test docs.

0264a36

Remove needless is_not_nightly closure.

7858beb

Drop check for mingw32-make.

8e35e2f

From what I can tell, it is no longer necessary on GitHub Actions. This removes it to help simplify things.

ehuss force-pushed the cargo_test-ignore-reason branch from 35a6dee to 8e35e2f Compare July 31, 2022 23:06

weihanglo approved these changes Aug 1, 2022

View reviewed changes

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 Aug 1, 2022

bors merged commit 2e35678 into rust-lang:master Aug 1, 2022

This was referenced Aug 1, 2022

Support per pkg target for -Zbuild-std #10330

Open

Change rustdoc-scrape-examples to be a target-level configuration #10343

Merged

Support inheriting jobserver fd for external subcommands #10511

Merged

ehuss mentioned this pull request Aug 2, 2022

Fix formats_source test requiring rustfmt. #10918

Merged

ehuss mentioned this pull request Aug 2, 2022

Update cargo, rls rust-lang/rust#100065

Merged

ehuss added this to the 1.64.0 milestone Aug 8, 2022

epage mentioned this pull request Sep 1, 2022

Runtime skipping of tests la10736/rstest#162

Open

yuja mentioned this pull request Feb 20, 2024

GPG signing tests fail on my machine (when gpg is not installed) martinvonz/jj#3096

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add requirements to cargo_test. #9892

Add requirements to cargo_test. #9892

ehuss commented Sep 9, 2021 •

edited

Loading

rust-highfive commented Sep 9, 2021

ehuss commented Sep 9, 2021

alexcrichton commented Sep 9, 2021

bors commented Sep 9, 2021

weihanglo commented Dec 22, 2021

ehuss commented Jul 31, 2022

weihanglo commented Jul 31, 2022

weihanglo left a comment

weihanglo Jul 31, 2022 •

edited

Loading

ehuss Jul 31, 2022

weihanglo left a comment

weihanglo commented Aug 1, 2022

bors commented Aug 1, 2022

bors commented Aug 1, 2022

bors commented Aug 1, 2022

Add requirements to cargo_test. #9892

Add requirements to cargo_test. #9892

Conversation

ehuss commented Sep 9, 2021 • edited Loading

rust-highfive commented Sep 9, 2021

ehuss commented Sep 9, 2021

alexcrichton commented Sep 9, 2021

bors commented Sep 9, 2021

weihanglo commented Dec 22, 2021

ehuss commented Jul 31, 2022

weihanglo commented Jul 31, 2022

weihanglo left a comment

Choose a reason for hiding this comment

weihanglo Jul 31, 2022 • edited Loading

Choose a reason for hiding this comment

ehuss Jul 31, 2022

Choose a reason for hiding this comment

weihanglo left a comment

Choose a reason for hiding this comment

weihanglo commented Aug 1, 2022

bors commented Aug 1, 2022

bors commented Aug 1, 2022

bors commented Aug 1, 2022

ehuss commented Sep 9, 2021 •

edited

Loading

weihanglo Jul 31, 2022 •

edited

Loading