Auto merge of #6039 - ehuss:fix-all-targets, r=alexcrichton

--all-targets fixes

- Fix: `cargo test --all-targets` was running lib tests three times.
- `--all-targets` help strings were wrong or misleading.
- Minor cleanup to add `Proposal` type to maybe make the code more readable.

Closes #5178.

Author: bors

src/bin/cargo/commands/bench.rs

@@ -26,7 +26,7 @@ pub fn cli() -> App {
             "Benchmark all tests",
             "Benchmark only the specified bench target",
             "Benchmark all benches",
-            "Benchmark all targets (default)",
+            "Benchmark all targets",
         )
         .arg(opt("no-run", "Compile, but don't run benchmarks"))
         .arg_package_spec(
@@ -78,7 +78,6 @@ pub fn exec(config: &mut Config, args: &ArgMatches) -> CliResult {
     let ops = TestOptions {
         no_run: args.is_present("no-run"),
         no_fail_fast: args.is_present("no-fail-fast"),
-        only_doc: false,
         compile_opts,
     };
 

src/bin/cargo/commands/build.rs

@@ -22,7 +22,7 @@ pub fn cli() -> App {
             "Build all tests",
             "Build only the specified bench target",
             "Build all benches",
-            "Build all targets (lib and bin targets by default)",
+            "Build all targets",
         )
         .arg_release("Build artifacts in release mode, with optimizations")
         .arg_features()

src/bin/cargo/commands/check.rs

@@ -21,7 +21,7 @@ pub fn cli() -> App {
             "Check all tests",
             "Check only the specified bench target",
             "Check all benches",
-            "Check all targets (lib and bin targets by default)",
+            "Check all targets",
         )
         .arg_release("Check artifacts in release mode, with optimizations")
         .arg(opt("profile", "Profile to build the selected target for").value_name("PROFILE"))

src/bin/cargo/commands/fix.rs

@@ -21,7 +21,7 @@ pub fn cli() -> App {
             "Fix all tests",
             "Fix only the specified bench target",
             "Fix all benches",
-            "Fix all targets (lib and bin targets by default)",
+            "Fix all targets (default)",
         )
         .arg_release("Fix artifacts in release mode, with optimizations")
         .arg(opt("profile", "Profile to build the selected target for").value_name("PROFILE"))

src/bin/cargo/commands/rustc.rs

@@ -19,7 +19,7 @@ pub fn cli() -> App {
             "Build all tests",
             "Build only the specified bench target",
             "Build all benches",
-            "Build all targets (lib and bin targets by default)",
+            "Build all targets",
         )
         .arg_release("Build artifacts in release mode, with optimizations")
         .arg(opt("profile", "Profile to build the selected target for").value_name("PROFILE"))

src/bin/cargo/commands/rustdoc.rs

@@ -23,7 +23,7 @@ pub fn cli() -> App {
             "Build all tests",
             "Build only the specified bench target",
             "Build all benches",
-            "Build all targets (default)",
+            "Build all targets",
         )
         .arg_release("Build artifacts in release mode, with optimizations")
         .arg_features()

src/bin/cargo/commands/test.rs

@@ -27,7 +27,7 @@ pub fn cli() -> App {
             "Test all tests",
             "Test only the specified bench target",
             "Test all benches",
-            "Test all targets (default)",
+            "Test all targets",
         )
         .arg(opt("doc", "Test only this library's documentation"))
         .arg(opt("no-run", "Compile, but don't run tests"))
@@ -116,7 +116,6 @@ pub fn exec(config: &mut Config, args: &ArgMatches) -> CliResult {
     let ops = ops::TestOptions {
         no_run: args.is_present("no-run"),
         no_fail_fast: args.is_present("no-fail-fast"),
-        only_doc: doc,
         compile_opts,
     };
 

src/cargo/ops/cargo_compile.rs

@@ -444,6 +444,22 @@ impl CompileFilter {
     }
 }
 
+/// A proposed target.
+///
+/// Proposed targets are later filtered into actual Units based on whether or
+/// not the target requires its features to be present.
+#[derive(Debug)]
+struct Proposal<'a> {
+    pkg: &'a Package,
+    target: &'a Target,
+    /// Indicates whether or not all required features *must* be present. If
+    /// false, and the features are not available, then it will be silently
+    /// skipped. Generally, targets specified by name (`--bin foo`) are
+    /// required, all others can be silently skipped if features are missing.
+    requires_features: bool,
+    mode: CompileMode,
+}
+
 /// Generates all the base targets for the packages the user has requested to
 /// compile. Dependencies for these targets are computed later in
 /// `unit_dependencies`.
@@ -531,36 +547,33 @@ fn generate_targets<'a>(
         }
     };
 
-    // Create a list of proposed targets.  The `bool` value indicates
-    // whether or not all required features *must* be present. If false,
-    // and the features are not available, then it will be silently
-    // skipped.  Generally, targets specified by name (`--bin foo`) are
-    // required, all others can be silently skipped if features are
-    // missing.
-    let mut proposals: Vec<(&Package, &Target, bool, CompileMode)> = Vec::new();
+    // Create a list of proposed targets.
+    let mut proposals: Vec<Proposal> = Vec::new();
 
     match *filter {
         CompileFilter::Default {
             required_features_filterable,
         } => {
             for pkg in packages {
                 let default = filter_default_targets(pkg.targets(), build_config.mode);
-                proposals.extend(default.into_iter().map(|target| {
-                    (
-                        *pkg,
-                        target,
-                        !required_features_filterable,
-                        build_config.mode,
-                    )
+                proposals.extend(default.into_iter().map(|target| Proposal {
+                    pkg,
+                    target,
+                    requires_features: !required_features_filterable,
+                    mode: build_config.mode,
                 }));
                 if build_config.mode == CompileMode::Test {
-                    // Include doctest for lib.
                     if let Some(t) = pkg
                         .targets()
                         .iter()
                         .find(|t| t.is_lib() && t.doctested() && t.doctestable())
                     {
-                        proposals.push((pkg, t, false, CompileMode::Doctest));
+                        proposals.push(Proposal {
+                            pkg,
+                            target: t,
+                            requires_features: false,
+                            mode: CompileMode::Doctest,
+                        });
                     }
                 }
             }
@@ -586,7 +599,12 @@ fn generate_targets<'a>(
                                 pkg.name()
                             ))?;
                         } else {
-                            libs.push((*pkg, target, false, build_config.mode));
+                            libs.push(Proposal {
+                                pkg,
+                                target,
+                                requires_features: false,
+                                mode: build_config.mode,
+                            });
                         }
                     }
                 }
@@ -600,6 +618,7 @@ fn generate_targets<'a>(
                 }
                 proposals.extend(libs);
             }
+
             // If --tests was specified, add all targets that would be
             // generated by `cargo test`.
             let test_filter = match *tests {
@@ -657,8 +676,8 @@ fn generate_targets<'a>(
     // Only include targets that are libraries or have all required
     // features available.
     let mut features_map = HashMap::new();
-    let mut units = Vec::new();
-    for (pkg, target, required, mode) in proposals {
+    let mut units = HashSet::new();
+    for Proposal { pkg, target, requires_features, mode} in proposals {
         let unavailable_features = match target.required_features() {
             Some(rf) => {
                 let features = features_map
@@ -670,8 +689,8 @@ fn generate_targets<'a>(
         };
         if target.is_lib() || unavailable_features.is_empty() {
             let unit = new_unit(pkg, target, mode);
-            units.push(unit);
-        } else if required {
+            units.insert(unit);
+        } else if requires_features {
             let required_features = target.required_features().unwrap();
             let quoted_required_features: Vec<String> = required_features
                 .iter()
@@ -688,7 +707,7 @@ fn generate_targets<'a>(
         }
         // else, silently skip target.
     }
-    Ok(units)
+    Ok(units.into_iter().collect())
 }
 
 fn resolve_all_features(
@@ -746,14 +765,19 @@ fn list_rule_targets<'a>(
     target_desc: &'static str,
     is_expected_kind: fn(&Target) -> bool,
     mode: CompileMode,
-) -> CargoResult<Vec<(&'a Package, &'a Target, bool, CompileMode)>> {
+) -> CargoResult<Vec<Proposal<'a>>> {
     let mut result = Vec::new();
     match *rule {
         FilterRule::All => {
             for pkg in packages {
                 for target in pkg.targets() {
                     if is_expected_kind(target) {
-                        result.push((*pkg, target, false, mode));
+                        result.push(Proposal {
+                            pkg,
+                            target,
+                            requires_features: false,
+                            mode,
+                        });
                     }
                 }
             }
@@ -780,12 +804,17 @@ fn find_named_targets<'a>(
     target_desc: &'static str,
     is_expected_kind: fn(&Target) -> bool,
     mode: CompileMode,
-) -> CargoResult<Vec<(&'a Package, &'a Target, bool, CompileMode)>> {
+) -> CargoResult<Vec<Proposal<'a>>> {
     let mut result = Vec::new();
     for pkg in packages {
         for target in pkg.targets() {
             if target.name() == target_name && is_expected_kind(target) {
-                result.push((*pkg, target, true, mode));
+                result.push(Proposal {
+                    pkg,
+                    target,
+                    requires_features: true,
+                    mode,
+                });
             }
         }
     }

src/cargo/ops/cargo_test.rs

@@ -10,7 +10,6 @@ pub struct TestOptions<'a> {
     pub compile_opts: ops::CompileOptions<'a>,
     pub no_run: bool,
     pub no_fail_fast: bool,
-    pub only_doc: bool,
 }
 
 pub fn run_tests(
@@ -23,27 +22,13 @@ pub fn run_tests(
     if options.no_run {
         return Ok(None);
     }
-    let (test, mut errors) = if options.only_doc {
-        assert!(options.compile_opts.filter.is_specific());
-        run_doc_tests(options, test_args, &compilation)?
-    } else {
-        run_unit_tests(options, test_args, &compilation)?
-    };
+    let (test, mut errors) = run_unit_tests(options, test_args, &compilation)?;
 
     // If we have an error and want to fail fast, return
     if !errors.is_empty() && !options.no_fail_fast {
         return Ok(Some(CargoTestError::new(test, errors)));
     }
 
-    // If a specific test was requested or we're not running any tests at all,
-    // don't run any doc tests.
-    if options.compile_opts.filter.is_specific() {
-        match errors.len() {
-            0 => return Ok(None),
-            _ => return Ok(Some(CargoTestError::new(test, errors))),
-        }
-    }
-
     let (doctest, docerrors) = run_doc_tests(options, test_args, &compilation)?;
     let test = if docerrors.is_empty() { test } else { doctest };
     errors.extend(docerrors);

tests/testsuite/test.rs

@@ -3168,3 +3168,17 @@ test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out
         .with_stderr("[ERROR] Can't mix --doc with other target selecting options\n")
         .run();
 }
+
+#[test]
+fn test_all_targets_lib() {
+    let p = project().file("src/lib.rs", "").build();
+
+    p.cargo("test --all-targets")
+        .with_stderr(
+            "\
+[COMPILING] foo [..]
+[FINISHED] dev [..]
+[RUNNING] [..]foo[..]
+",
+        ).run();
+}