Auto merge of #9892 - ehuss:cargo_test-ignore-reason, r=weihanglo

Add requirements to cargo_test.

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.

Author: bors

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

@@ -1,19 +1,84 @@
 extern crate proc_macro;
 
 use proc_macro::*;
+use std::process::Command;
+use std::sync::Once;
 
 #[proc_macro_attribute]
 pub fn cargo_test(attr: TokenStream, item: TokenStream) -> TokenStream {
+    // Ideally these options would be embedded in the test itself. However, I
+    // find it very helpful to have the test clearly state whether or not it
+    // is ignored. It would be nice to have some kind of runtime ignore
+    // support (such as
+    // https://internals.rust-lang.org/t/pre-rfc-skippable-tests/14611).
+    //
+    // Unfortunately a big drawback here is that if the environment changes
+    // (such as the existence of the `git` CLI), this will not trigger a
+    // rebuild and the test will still be ignored. In theory, something like
+    // `tracked_env` or `tracked_path`
+    // (https://github.com/rust-lang/rust/issues/99515) could help with this,
+    // but they don't really handle the absence of files well.
+    let mut ignore = false;
+    let mut requires_reason = false;
+    let mut found_reason = false;
+    let is_not_nightly = !version().1;
+    for rule in split_rules(attr) {
+        match rule.as_str() {
+            "build_std_real" => {
+                // Only run the "real" build-std tests on nightly and with an
+                // explicit opt-in (these generally only work on linux, and
+                // have some extra requirements, and are slow, and can pollute
+                // the environment since it downloads dependencies).
+                ignore |= is_not_nightly;
+                ignore |= option_env!("CARGO_RUN_BUILD_STD_TESTS").is_none();
+            }
+            "build_std_mock" => {
+                // Only run the "mock" build-std tests on nightly and disable
+                // for windows-gnu which is missing object files (see
+                // https://github.com/rust-lang/wg-cargo-std-aware/issues/46).
+                ignore |= is_not_nightly;
+                ignore |= cfg!(all(target_os = "windows", target_env = "gnu"));
+            }
+            "nightly" => {
+                requires_reason = true;
+                ignore |= is_not_nightly;
+            }
+            s if s.starts_with("requires_") => {
+                let command = &s[9..];
+                ignore |= !has_command(command);
+            }
+            s if s.starts_with(">=1.") => {
+                requires_reason = true;
+                let min_minor = s[4..].parse().unwrap();
+                ignore |= version().0 < min_minor;
+            }
+            s if s.starts_with("reason=") => {
+                found_reason = true;
+            }
+            _ => panic!("unknown rule {:?}", rule),
+        }
+    }
+    if requires_reason && !found_reason {
+        panic!(
+            "#[cargo_test] with a rule also requires a reason, \
+            such as #[cargo_test(nightly, reason = \"needs -Z unstable-thing\")]"
+        );
+    }
+
     let span = Span::call_site();
     let mut ret = TokenStream::new();
-    ret.extend(Some(TokenTree::from(Punct::new('#', Spacing::Alone))));
-    let test = TokenTree::from(Ident::new("test", span));
-    ret.extend(Some(TokenTree::from(Group::new(
-        Delimiter::Bracket,
-        test.into(),
-    ))));
-
-    let build_std = contains_ident(&attr, "build_std");
+    let add_attr = |ret: &mut TokenStream, attr_name| {
+        ret.extend(Some(TokenTree::from(Punct::new('#', Spacing::Alone))));
+        let attr = TokenTree::from(Ident::new(attr_name, span));
+        ret.extend(Some(TokenTree::from(Group::new(
+            Delimiter::Bracket,
+            attr.into(),
+        ))));
+    };
+    add_attr(&mut ret, "test");
+    if ignore {
+        add_attr(&mut ret, "ignore");
+    }
 
     for token in item {
         let group = match token {
@@ -38,17 +103,6 @@ pub fn cargo_test(attr: TokenStream, item: TokenStream) -> TokenStream {
             };"#,
         );
 
-        // If this is a `build_std` test (aka `tests/build-std/*.rs`) then they
-        // only run on nightly and they only run when specifically instructed to
-        // on CI.
-        if build_std {
-            let ts = to_token_stream("if !cargo_test_support::is_nightly() { return }");
-            new_body.extend(ts);
-            let ts = to_token_stream(
-                "if std::env::var(\"CARGO_RUN_BUILD_STD_TESTS\").is_err() { return }",
-            );
-            new_body.extend(ts);
-        }
         new_body.extend(group.stream());
         ret.extend(Some(TokenTree::from(Group::new(
             group.delimiter(),
@@ -59,13 +113,79 @@ pub fn cargo_test(attr: TokenStream, item: TokenStream) -> TokenStream {
     ret
 }
 
-fn contains_ident(t: &TokenStream, ident: &str) -> bool {
-    t.clone().into_iter().any(|t| match t {
-        TokenTree::Ident(i) => i.to_string() == ident,
+fn split_rules(t: TokenStream) -> Vec<String> {
+    let tts: Vec<_> = t.into_iter().collect();
+    tts.split(|tt| match tt {
+        TokenTree::Punct(p) => p.as_char() == ',',
         _ => false,
     })
+    .filter(|parts| !parts.is_empty())
+    .map(|parts| {
+        parts
+            .into_iter()
+            .map(|part| part.to_string())
+            .collect::<String>()
+    })
+    .collect()
 }
 
 fn to_token_stream(code: &str) -> TokenStream {
     code.parse().unwrap()
 }
+
+static mut VERSION: (u32, bool) = (0, false);
+
+fn version() -> &'static (u32, bool) {
+    static INIT: Once = Once::new();
+    INIT.call_once(|| {
+        let output = Command::new("rustc")
+            .arg("-V")
+            .output()
+            .expect("rustc should run");
+        let stdout = std::str::from_utf8(&output.stdout).expect("utf8");
+        let vers = stdout.split_whitespace().skip(1).next().unwrap();
+        let is_nightly = option_env!("CARGO_TEST_DISABLE_NIGHTLY").is_none()
+            && (vers.contains("-nightly") || vers.contains("-dev"));
+        let minor = vers.split('.').skip(1).next().unwrap().parse().unwrap();
+        unsafe { VERSION = (minor, is_nightly) }
+    });
+    unsafe { &VERSION }
+}
+
+fn has_command(command: &str) -> bool {
+    let output = match Command::new(command).arg("--version").output() {
+        Ok(output) => output,
+        Err(e) => {
+            // hg is not installed on GitHub macos.
+            // Consider installing it if Cargo gains more hg support, but
+            // otherwise it isn't critical.
+            if is_ci() && !(cfg!(target_os = "macos") && command == "hg") {
+                panic!(
+                    "expected command `{}` to be somewhere in PATH: {}",
+                    command, e
+                );
+            }
+            return false;
+        }
+    };
+    if !output.status.success() {
+        panic!(
+            "expected command `{}` to be runnable, got error {}:\n\
+            stderr:{}\n\
+            stdout:{}\n",
+            command,
+            output.status,
+            String::from_utf8_lossy(&output.stderr),
+            String::from_utf8_lossy(&output.stdout)
+        );
+    }
+    true
+}
+
+/// Whether or not this running in a Continuous Integration environment.
+fn is_ci() -> bool {
+    // Consider using `tracked_env` instead of option_env! when it is stabilized.
+    // `tracked_env` will handle changes, but not require rebuilding the macro
+    // itself like option_env does.
+    option_env!("CI").is_some() || option_env!("TF_BUILD").is_some()
+}

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

@@ -1128,6 +1128,10 @@ pub fn rustc_host_env() -> String {
 
 pub fn is_nightly() -> bool {
     let vv = &RUSTC_INFO.verbose_version;
+    // CARGO_TEST_DISABLE_NIGHTLY is set in rust-lang/rust's CI so that all
+    // nightly-only tests are disabled there. Otherwise, it could make it
+    // difficult to land changes which would need to be made simultaneously in
+    // rust-lang/cargo and rust-lan/rust, which isn't possible.
     env::var("CARGO_TEST_DISABLE_NIGHTLY").is_err()
         && (vv.contains("-nightly") || vv.contains("-dev"))
 }
@@ -1350,16 +1354,6 @@ pub fn slow_cpu_multiplier(main: u64) -> Duration {
     Duration::from_secs(*SLOW_CPU_MULTIPLIER * main)
 }
 
-pub fn command_is_available(cmd: &str) -> bool {
-    if let Err(e) = process(cmd).arg("-V").exec_with_output() {
-        eprintln!("{} not available, skipping tests", cmd);
-        eprintln!("{:?}", e);
-        false
-    } else {
-        true
-    }
-}
-
 #[cfg(windows)]
 pub fn symlink_supported() -> bool {
     if is_ci() {

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

@@ -50,13 +50,7 @@ fn <description>() {
 }
 ```
 
-`#[cargo_test]`:
-- This is used in place of `#[test]`
-- This attribute injects code which does some setup before starting the
-  test, creating a filesystem "sandbox" under the "cargo integration test"
-  directory for each test such as
-  `/path/to/cargo/target/cit/t123/`
-- The sandbox will contain a `home` directory that will be used instead of your normal home directory
+The [`#[cargo_test]` attribute](#cargo_test-attribute) is used in place of `#[test]` to inject some setup code.
 
 [`ProjectBuilder`] via `project()`:
 - Each project is in a separate directory in the sandbox
@@ -68,6 +62,37 @@ fn <description>() {
   - See [`support::compare`] for an explanation of the string pattern matching.
     Patterns are used to make it easier to match against the expected output.
 
+#### `#[cargo_test]` attribute
+
+The `#[cargo_test]` attribute injects code which does some setup before starting the test.
+It will create a filesystem "sandbox" under the "cargo integration test" directory for each test, such as `/path/to/cargo/target/tmp/cit/t123/`.
+The sandbox will contain a `home` directory that will be used instead of your normal home directory.
+
+The `#[cargo_test]` attribute takes several options that will affect how the test is generated.
+They are listed in parentheses separated with commas, such as:
+
+```rust,ignore
+#[cargo_test(nightly, reason = "-Zfoo is unstable")]
+```
+
+The options it supports are:
+
+* `nightly` — This will cause the test to be ignored if not running on the nightly toolchain.
+  This is useful for tests that use unstable options in `rustc` or `rustdoc`.
+  These tests are run in Cargo's CI, but are disabled in rust-lang/rust's CI due to the difficulty of updating both repos simultaneously.
+  A `reason` field is required to explain why it is nightly-only.
+* `build_std_real` — This is a "real" `-Zbuild-std` test (in the `build_std` integration test).
+  This only runs on nightly, and only if the environment variable `CARGO_RUN_BUILD_STD_TESTS` is set (these tests on run on Linux).
+* `build_std_mock` — This is a "mock" `-Zbuild-std` test (which uses a mock standard library).
+  This only runs on nightly, and is disabled for windows-gnu.
+* `requires_` — This indicates a command that is required to be installed to be run.
+  For example, `requires_rustfmt` means the test will only run if the executable `rustfmt` is installed.
+  These tests are *always* run on CI.
+  This is mainly used to avoid requiring contributors from having every dependency installed.
+* `>=1.64` — This indicates that the test will only run with the given version of `rustc` or newer.
+  This can be used when a new `rustc` feature has been stabilized that the test depends on.
+  If this is specified, a `reason` is required to explain why it is being checked.
+
 #### Testing Nightly Features
 
 If you are testing a Cargo feature that only works on "nightly" Cargo, then
@@ -79,16 +104,15 @@ p.cargo("build").masquerade_as_nightly_cargo(&["print-im-a-teapot"])
 ```
 
 If you are testing a feature that only works on *nightly rustc* (such as
-benchmarks), then you should exit the test if it is not running with nightly
-rust, like this:
+benchmarks), then you should use the `nightly` option of the `cargo_test`
+attribute, like this:
 
 ```rust,ignore
-if !is_nightly() {
-    // Add a comment here explaining why this is necessary.
-    return;
-}
+#[cargo_test(nightly, reason = "-Zfoo is unstable")]
 ```
 
+This will cause the test to be ignored if not running on the nightly toolchain.
+
 #### Specifying Dependencies
 
 You should not write any tests that use the network such as contacting
@@ -201,16 +225,15 @@ the name of the feature as the reason, like this:
 ```
 
 If you are testing a feature that only works on *nightly rustc* (such as
-benchmarks), then you should exit the test if it is not running with nightly
-rust, like this:
+benchmarks), then you should use the `nightly` option of the `cargo_test`
+attribute, like this:
 
 ```rust,ignore
-if !is_nightly() {
-    // Add a comment here explaining why this is necessary.
-    return;
-}
+#[cargo_test(nightly, reason = "-Zfoo is unstable")]
 ```
 
+This will cause the test to be ignored if not running on the nightly toolchain.
+
 ### Platform-specific Notes
 
 When checking output, use `/` for paths even on Windows: the actual output

tests/build-std/main.rs

@@ -13,7 +13,7 @@
 //! not catching any regressions that `tests/testsuite/standard_lib.rs` isn't
 //! already catching.
 //!
-//! All tests here should use `#[cargo_test(build_std)]` to indicate that
+//! All tests here should use `#[cargo_test(build_std_real)]` to indicate that
 //! boilerplate should be generated to require the nightly toolchain and the
 //! `CARGO_RUN_BUILD_STD_TESTS` env var to be set to actually run these tests.
 //! Otherwise the tests are skipped.
@@ -59,7 +59,7 @@ impl BuildStd for Execs {
     }
 }
 
-#[cargo_test(build_std)]
+#[cargo_test(build_std_real)]
 fn basic() {
     let p = project()
         .file(
@@ -127,7 +127,7 @@ fn basic() {
     assert_eq!(p.glob(deps_dir.join("*.dylib")).count(), 0);
 }
 
-#[cargo_test(build_std)]
+#[cargo_test(build_std_real)]
 fn cross_custom() {
     let p = project()
         .file(
@@ -170,7 +170,7 @@ fn cross_custom() {
         .run();
 }
 
-#[cargo_test(build_std)]
+#[cargo_test(build_std_real)]
 fn custom_test_framework() {
     let p = project()
         .file(