Repository: rust-lang/rust
Pull request: Make sure we run the full suite, even when more specific filters are given

Some test suite's filter code took this into account, while others didn't. 
Imagine this:

```
x test tests/ui/sometest.rs tests/ui
```

This would run only `tests/ui/sometest.rs` since we filtered out `tests/ui` since it matches the name of the test suite.
The reason `x test tests/ui` on its own works, is that if compiletest gets no filter paths, only a suite, it runs the entire suite.
What I've implemented here is that when you pass the suite path, we effectively ignore specific filters and just run the entire suite since that'll include the more specific paths too. This was the case for a few other suites as well, like rustdocjs, but others actually handle this correctly (like clippy, which my code does edit but for which the behavior doesn't change). 

Hope this makes sense!

<!-- homu-ignore:start -->
<!--
If this PR is related to an unstable feature or an otherwise tracked effort,
please link to the relevant tracking issue here. If you don't know of a related
tracking issue or there are none, feel free to ignore this.

This PR will get automatically assigned to a reviewer. In case you would like
a specific user to review your work, you can assign it to them by using

    r? <reviewer name>
-->
<!-- homu-ignore:end -->


Changed files:
filename: src/bootstrap/src/core/build_steps/test.rs
patch: @@ -37,8 +37,9 @@ use crate::core::{android, debuggers};
 use crate::utils::build_stamp::{self, BuildStamp};
 use crate::utils::exec::{BootstrapCommand, command};
 use crate::utils::helpers::{
-    self, LldThreads, add_dylib_path, add_rustdoc_cargo_linker_args, dylib_path, dylib_path_var,
-    linker_args, linker_flags, t, target_supports_cranelift_backend, up_to_date,
+    self, LldThreads, TestFilterCategory, add_dylib_path, add_rustdoc_cargo_linker_args,
+    dylib_path, dylib_path_var, linker_args, linker_flags, t, target_supports_cranelift_backend,
+    up_to_date,
 };
 use crate::utils::render_tests::{add_flags_and_try_run_tests, try_run_tests};
 use crate::{CLang, CodegenBackendKind, GitRepo, Mode, PathSet, TestTarget, envify};
@@ -969,13 +970,15 @@ impl Step for Clippy {
             let paths = &builder.config.paths[..];
             let mut test_names = Vec::new();
             for path in paths {
-                if let Some(path) =
-                    helpers::is_valid_test_suite_arg(path, "src/tools/clippy/tests", builder)
-                {
-                    test_names.push(path);
-                } else if path.ends_with("src/tools/clippy") {
-                    // When src/tools/clippy is called directly, all tests should be run.
-                    break 'partially_test;
+                match helpers::is_valid_test_suite_arg(path, "src/tools/clippy/tests", builder) {
+                    TestFilterCategory::Arg(path) => {
+                        test_names.push(path);
+                    }
+                    TestFilterCategory::Fullsuite => {
+                        // When src/tools/clippy is called directly, all tests should be run.
+                        break 'partially_test;
+                    }
+                    TestFilterCategory::Uninteresting => {}
                 }
             }
             cargo.env("TESTNAME", test_names.join(","));
@@ -1104,16 +1107,30 @@ impl Step for RustdocJSStd {
             .arg(builder.doc_out(self.target))
             .arg("--test-folder")
             .arg(builder.src.join("tests/rustdoc-js-std"));
-        for path in &builder.paths {
-            if let Some(p) = helpers::is_valid_test_suite_arg(path, "tests/rustdoc-js-std", builder)
-            {
-                if !p.ends_with(".js") {
-                    eprintln!("A non-js file was given: `{}`", path.display());
-                    panic!("Cannot run rustdoc-js-std tests");
+
+        let full_suite = builder.paths.iter().any(|path| {
+            matches!(
+                helpers::is_valid_test_suite_arg(path, "tests/rustdoc-js-std", builder),
+                TestFilterCategory::Fullsuite
+            )
+        });
+
+        // If we have to also run the full suite, don't worry about the individual arguments.
+        // They will be covered by running the entire suite
+        if !full_suite {
+            for path in &builder.paths {
+                if let TestFilterCategory::Arg(p) =
+                    helpers::is_valid_test_suite_arg(path, "tests/rustdoc-js-std", builder)
+                {
+                    if !p.ends_with(".js") {
+                        eprintln!("A non-js file was given: `{}`", path.display());
+                        panic!("Cannot run rustdoc-js-std tests");
+                    }
+                    command.arg("--test-file").arg(path);
                 }
-                command.arg("--test-file").arg(path);
             }
         }
+
         builder.ensure(crate::core::build_steps::doc::Std::from_build_compiler(
             self.build_compiler,
             self.target,
@@ -1252,14 +1269,27 @@ impl Step for RustdocGUI {
 
         add_rustdoc_cargo_linker_args(&mut cmd, builder, self.test_compiler.host, LldThreads::No);
 
-        for path in &builder.paths {
-            if let Some(p) = helpers::is_valid_test_suite_arg(path, "tests/rustdoc-gui", builder) {
-                if !p.ends_with(".goml") {
-                    eprintln!("A non-goml file was given: `{}`", path.display());
-                    panic!("Cannot run rustdoc-gui tests");
-                }
-                if let Some(name) = path.file_name().and_then(|f| f.to_str()) {
-                    cmd.arg("--goml-file").arg(name);
+        let full_suite = builder.paths.iter().any(|path| {
+            matches!(
+                helpers::is_valid_test_suite_arg(path, "tests/rustdoc-js-std", builder),
+                TestFilterCategory::Fullsuite
+            )
+        });
+
+        // If we have to also run the full suite, don't worry about the individual arguments.
+        // They will be covered by running the entire suite
+        if !full_suite {
+            for path in &builder.paths {
+                if let TestFilterCategory::Arg(p) =
+                    helpers::is_valid_test_suite_arg(path, "tests/rustdoc-gui", builder)
+                {
+                    if !p.ends_with(".goml") {
+                        eprintln!("A non-goml file was given: `{}`", path.display());
+                        panic!("Cannot run rustdoc-gui tests");
+                    }
+                    if let Some(name) = path.file_name().and_then(|f| f.to_str()) {
+                        cmd.arg("--goml-file").arg(name);
+                    }
                 }
             }
         }
@@ -2270,11 +2300,23 @@ Please disable assertions with `rust.debug-assertions = false`.
             }
             paths = &paths_v;
         }
+
         // Get test-args by striping suite path
-        let mut test_args: Vec<&str> = paths
-            .iter()
-            .filter_map(|p| helpers::is_valid_test_suite_arg(p, suite_path, builder))
-            .collect();
+        let mut test_args = Vec::new();
+        for p in paths {
+            match helpers::is_valid_test_suite_arg(p, suite_path, builder) {
+                TestFilterCategory::Fullsuite => {
+                    // If we also have to run the full suite, don't append _any_ test args here,
+                    // clear the list instead and break out.
+                    // That way none of the more specific paths make it into test_args,
+                    // since running the whole suite will run the specific ones anyway.
+                    test_args.clear();
+                    break;
+                }
+                TestFilterCategory::Arg(a) => test_args.push(a),
+                TestFilterCategory::Uninteresting => {}
+            }
+        }
 
         test_args.append(&mut builder.config.test_args());
 

filename: src/bootstrap/src/utils/helpers.rs
patch: @@ -240,18 +240,30 @@ pub fn target_supports_cranelift_backend(target: TargetSelection) -> bool {
     }
 }
 
+/// Value returned from [`is_valid_test_suite_arg`], which figures out which paths start with the
+/// suite name (and therefore which should be run).
+pub enum TestFilterCategory<'a> {
+    /// If a path is equal to the name of the suite, this is returned.
+    Fullsuite,
+    /// If a path starts with the suite, the suite prefix is stripped and the rest is returned as
+    /// this variant.
+    Arg(&'a str),
+    /// For paths that don't start with the suite.
+    Uninteresting,
+}
+
 pub fn is_valid_test_suite_arg<'a, P: AsRef<Path>>(
     path: &'a Path,
     suite_path: P,
     builder: &Builder<'_>,
-) -> Option<&'a str> {
+) -> TestFilterCategory<'a> {
     let suite_path = suite_path.as_ref();
     let path = match path.strip_prefix(".") {
         Ok(p) => p,
         Err(_) => path,
     };
     if !path.starts_with(suite_path) {
-        return None;
+        return TestFilterCategory::Uninteresting;
     }
     let abs_path = builder.src.join(path);
     let exists = abs_path.is_dir() || abs_path.is_file();
@@ -268,8 +280,8 @@ pub fn is_valid_test_suite_arg<'a, P: AsRef<Path>>(
     // flag is respected, so providing an empty --test-args conflicts with
     // any following it.
     match path.strip_prefix(suite_path).ok().and_then(|p| p.to_str()) {
-        Some(s) if !s.is_empty() => Some(s),
-        _ => None,
+        Some(s) if !s.is_empty() => TestFilterCategory::Arg(s),
+        _ => TestFilterCategory::Fullsuite,
     }
 }