Auto merge of #10305 - weihanglo:issue-10268, r=alexcrichton

do not compile test for bins flagged as `test = false`

### What does this PR try to resolve?

Fixes #10268

#6683 introduced a behavior that compiles all bin targets, but for bins with `test = false` they shouldn't be compiled with `--test` as testbins.

### How should we test and review this PR?

In the first commit of this PR, I refines the test `test_filtered_excludes_compiling_examples` to reflect the current wrong behavior (test passed). The following two commits correct the behavior and the test accordingly. The last few commits encapsulate scattered target selection logic into functions on `CompileFilter`.

src/bin/cargo/commands/fix.rs

@@ -1,6 +1,6 @@
 use crate::command_prelude::*;
 
-use cargo::ops::{self, CompileFilter, FilterRule, LibRule};
+use cargo::ops;
 
 pub fn cli() -> App {
     subcommand("fix")
@@ -76,15 +76,9 @@ pub fn exec(config: &mut Config, args: &ArgMatches) -> CliResult {
     let mut opts =
         args.compile_options(config, mode, Some(&ws), ProfileChecking::LegacyTestOnly)?;
 
-    if let CompileFilter::Default { .. } = opts.filter {
-        opts.filter = CompileFilter::Only {
-            all_targets: true,
-            lib: LibRule::Default,
-            bins: FilterRule::All,
-            examples: FilterRule::All,
-            benches: FilterRule::All,
-            tests: FilterRule::All,
-        }
+    if !opts.filter.is_specific() {
+        // cargo fix with no target selection implies `--all-targets`.
+        opts.filter = ops::CompileFilter::new_all_targets();
     }
 
     ops::fix(

src/bin/cargo/commands/run.rs

@@ -62,19 +62,8 @@ pub fn exec(config: &mut Config, args: &ArgMatches) -> CliResult {
             .iter()
             .filter_map(|pkg| pkg.manifest().default_run())
             .collect();
-        if default_runs.len() == 1 {
-            compile_opts.filter = CompileFilter::from_raw_arguments(
-                false,
-                vec![default_runs[0].to_owned()],
-                false,
-                vec![],
-                false,
-                vec![],
-                false,
-                vec![],
-                false,
-                false,
-            );
+        if let [bin] = &default_runs[..] {
+            compile_opts.filter = CompileFilter::single_bin(bin.to_string());
         } else {
             // ops::run will take care of errors if len pkgs != 1.
             compile_opts.filter = CompileFilter::Default {

src/bin/cargo/commands/test.rs

@@ -1,6 +1,6 @@
 use crate::command_prelude::*;
 use anyhow::Error;
-use cargo::ops::{self, CompileFilter, FilterRule, LibRule};
+use cargo::ops;
 
 pub fn cli() -> App {
     subcommand("test")
@@ -77,44 +77,30 @@ pub fn exec(config: &mut Config, args: &ArgMatches) -> CliResult {
 
     // `TESTNAME` is actually an argument of the test binary, but it's
     // important, so we explicitly mention it and reconfigure.
-    let test_name: Option<&str> = args.value_of("TESTNAME");
+    let test_name = args.value_of("TESTNAME");
     let test_args = args.value_of("TESTNAME").into_iter();
     let test_args = test_args.chain(args.values_of("args").unwrap_or_default());
     let test_args = test_args.collect::<Vec<_>>();
 
     let no_run = args.is_present("no-run");
     let doc = args.is_present("doc");
     if doc {
-        if let CompileFilter::Only { .. } = compile_opts.filter {
-            return Err(CliError::new(
-                anyhow::format_err!("Can't mix --doc with other target selecting options"),
-                101,
-            ));
+        if compile_opts.filter.is_specific() {
+            return Err(
+                anyhow::format_err!("Can't mix --doc with other target selecting options").into(),
+            );
         }
         if no_run {
-            return Err(CliError::new(
-                anyhow::format_err!("Can't skip running doc tests with --no-run"),
-                101,
-            ));
+            return Err(anyhow::format_err!("Can't skip running doc tests with --no-run").into());
         }
         compile_opts.build_config.mode = CompileMode::Doctest;
-        compile_opts.filter = ops::CompileFilter::new(
-            LibRule::True,
-            FilterRule::none(),
-            FilterRule::none(),
-            FilterRule::none(),
-            FilterRule::none(),
-        );
-    } else if test_name.is_some() {
-        if let CompileFilter::Default { .. } = compile_opts.filter {
-            compile_opts.filter = ops::CompileFilter::new(
-                LibRule::Default,   // compile the library, so the unit tests can be run filtered
-                FilterRule::All, // compile the binaries, so the unit tests in binaries can be run filtered
-                FilterRule::All, // compile the tests, so the integration tests can be run filtered
-                FilterRule::none(), // specify --examples to unit test binaries filtered
-                FilterRule::none(), // specify --benches to unit test benchmarks filtered
-            ); // also, specify --doc to run doc tests filtered
-        }
+        compile_opts.filter = ops::CompileFilter::lib_only();
+    } else if test_name.is_some() && !compile_opts.filter.is_specific() {
+        // If arg `TESTNAME` is provided, assumed that the user knows what
+        // exactly they wants to test, so we use `all_test_targets` to
+        // avoid compiling unnecessary targets such as examples, which are
+        // included by the logic of default target filter.
+        compile_opts.filter = ops::CompileFilter::all_test_targets();
     }
 
     let ops = ops::TestOptions {

src/cargo/ops/cargo_compile.rs

@@ -759,7 +759,7 @@ impl FilterRule {
 }
 
 impl CompileFilter {
-    /// Construct a CompileFilter from raw command line arguments.
+    /// Constructs a filter from raw command line arguments.
     pub fn from_raw_arguments(
         lib_only: bool,
         bins: Vec<String>,
@@ -788,7 +788,7 @@ impl CompileFilter {
         CompileFilter::new(rule_lib, rule_bins, rule_tsts, rule_exms, rule_bens)
     }
 
-    /// Construct a CompileFilter from underlying primitives.
+    /// Constructs a filter from underlying primitives.
     pub fn new(
         rule_lib: LibRule,
         rule_bins: FilterRule,
@@ -817,6 +817,7 @@ impl CompileFilter {
         }
     }
 
+    /// Constructs a filter that includes all targets.
     pub fn new_all_targets() -> CompileFilter {
         CompileFilter::Only {
             all_targets: true,
@@ -828,6 +829,51 @@ impl CompileFilter {
         }
     }
 
+    /// Constructs a filter that includes all test targets.
+    ///
+    /// Being different from the behavior of [`CompileFilter::Default`], this
+    /// function only recongnizes test targets, which means cargo might compile
+    /// all targets with `tested` flag on, whereas [`CompileFilter::Default`]
+    /// may include additional example targets to ensure they can be compiled.
+    ///
+    /// Note that the actual behavior is subject to `filter_default_targets`
+    /// and `generate_targets` though.
+    pub fn all_test_targets() -> Self {
+        Self::Only {
+            all_targets: false,
+            lib: LibRule::Default,
+            bins: FilterRule::none(),
+            examples: FilterRule::none(),
+            tests: FilterRule::All,
+            benches: FilterRule::none(),
+        }
+    }
+
+    /// Constructs a filter that includes lib target only.
+    pub fn lib_only() -> Self {
+        Self::Only {
+            all_targets: false,
+            lib: LibRule::True,
+            bins: FilterRule::none(),
+            examples: FilterRule::none(),
+            tests: FilterRule::none(),
+            benches: FilterRule::none(),
+        }
+    }
+
+    /// Constructs a filter that includes the given binary. No more. No less.
+    pub fn single_bin(bin: String) -> Self {
+        Self::Only {
+            all_targets: false,
+            lib: LibRule::False,
+            bins: FilterRule::new(vec![bin], false),
+            examples: FilterRule::none(),
+            tests: FilterRule::none(),
+            benches: FilterRule::none(),
+        }
+    }
+
+    /// Indicates if Cargo needs to build any dev dependency.
     pub fn need_dev_deps(&self, mode: CompileMode) -> bool {
         match mode {
             CompileMode::Test | CompileMode::Doctest | CompileMode::Bench => true,
@@ -848,8 +894,8 @@ impl CompileFilter {
         }
     }
 
-    // this selects targets for "cargo run". for logic to select targets for
-    // other subcommands, see generate_targets and filter_default_targets
+    /// Selects targets for "cargo run". for logic to select targets for other
+    /// subcommands, see `generate_targets` and `filter_default_targets`.
     pub fn target_run(&self, target: &Target) -> bool {
         match *self {
             CompileFilter::Default { .. } => true,

tests/testsuite/test.rs

@@ -1606,32 +1606,70 @@ fn test_run_implicit_example_target() {
 #[cargo_test]
 fn test_filtered_excludes_compiling_examples() {
     let p = project()
+        .file(
+            "Cargo.toml",
+            r#"
+                [package]
+                name = "foo"
+                version = "0.0.1"
+                authors = []
+
+                [[bin]]
+                name = "mybin"
+                test = false
+            "#,
+        )
         .file(
             "src/lib.rs",
-            "#[cfg(test)] mod tests { #[test] fn foo() { assert!(true); } }",
+            "#[cfg(test)] mod tests { #[test] fn test_in_lib() { } }",
+        )
+        .file(
+            "src/bin/mybin.rs",
+            "#[test] fn test_in_bin() { }
+               fn main() { panic!(\"Don't execute me!\"); }",
+        )
+        .file("tests/mytest.rs", "#[test] fn test_in_test() { }")
+        .file(
+            "benches/mybench.rs",
+            "#[test] fn test_in_bench() { assert!(false) }",
+        )
+        .file(
+            "examples/myexm1.rs",
+            "#[test] fn test_in_exm() { assert!(false) }
+               fn main() { panic!(\"Don't execute me!\"); }",
         )
-        .file("examples/ex1.rs", "fn main() {}")
         .build();
 
-    p.cargo("test -v foo")
+    p.cargo("test -v test_in_")
         .with_stdout(
             "
 running 1 test
-test tests::foo ... ok
+test tests::test_in_lib ... ok
+
+test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out[..]
+
+
+running 1 test
+test test_in_test ... ok
 
 test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out[..]
 
 ",
         )
-        .with_stderr(
+        .with_stderr_unordered(
             "\
 [COMPILING] foo v0.0.1 ([CWD])
+[RUNNING] `rustc --crate-name foo src/lib.rs [..] --crate-type lib [..]`
 [RUNNING] `rustc --crate-name foo src/lib.rs [..] --test [..]`
+[RUNNING] `rustc --crate-name mybin src/bin/mybin.rs [..] --crate-type bin [..]`
+[RUNNING] `rustc --crate-name mytest tests/mytest.rs [..] --test [..]`
 [FINISHED] test [unoptimized + debuginfo] target(s) in [..]
-[RUNNING] `[CWD]/target/debug/deps/foo-[..] foo`
+[RUNNING] `[CWD]/target/debug/deps/foo-[..] test_in_`
+[RUNNING] `[CWD]/target/debug/deps/mytest-[..] test_in_`
 ",
         )
-        .with_stderr_does_not_contain("[RUNNING][..]rustc[..]ex1[..]")
+        .with_stderr_does_not_contain("[RUNNING][..]rustc[..]myexm1[..]")
+        .with_stderr_does_not_contain("[RUNNING][..]deps/mybin-[..] test_in_")
         .run();
 }