Auto merge of #8266 - camsteffen:test-tweaks, r=flip1995

Some test code cleanup

changelog: none

Mainly moves /clippy_workspace_tests into /tests and combines the two dogfood tests which can't run concurrently.

.github/workflows/clippy.yml

@@ -70,14 +70,6 @@ jobs:
       run: cargo test --features deny-warnings
       working-directory: clippy_dev
 
-    - name: Test cargo-clippy
-      run: ../target/debug/cargo-clippy
-      working-directory: clippy_workspace_tests
-
-    - name: Test cargo-clippy --fix
-      run: ../target/debug/cargo-clippy clippy --fix
-      working-directory: clippy_workspace_tests
-
     - name: Test clippy-driver
       run: bash .github/driver.sh
       env:

.github/workflows/clippy_bors.yml

@@ -115,8 +115,13 @@ jobs:
       run: cargo build --features deny-warnings,internal
 
     - name: Test
+      if: runner.os == 'Linux'
       run: cargo test --features deny-warnings,internal
 
+    - name: Test
+      if: runner.os != 'Linux'
+      run: cargo test --features deny-warnings,internal -- --skip dogfood
+
     - name: Test clippy_lints
       run: cargo test --features deny-warnings,internal
       working-directory: clippy_lints
@@ -133,14 +138,6 @@ jobs:
       run: cargo test --features deny-warnings
       working-directory: clippy_dev
 
-    - name: Test cargo-clippy
-      run: ../target/debug/cargo-clippy
-      working-directory: clippy_workspace_tests
-
-    - name: Test cargo-clippy --fix
-      run: ../target/debug/cargo-clippy clippy --fix
-      working-directory: clippy_workspace_tests
-
     - name: Test clippy-driver
       run: bash .github/driver.sh
       env:

.gitignore

@@ -19,7 +19,6 @@ out
 /target
 /clippy_lints/target
 /clippy_utils/target
-/clippy_workspace_tests/target
 /clippy_dev/target
 /lintcheck/target
 /rustc_tools_util/target

clippy_utils/src/paths.rs

@@ -117,6 +117,7 @@ pub const PARKING_LOT_RWLOCK_WRITE_GUARD: [&str; 2] = ["parking_lot", "RwLockWri
 pub const PATH_BUF_AS_PATH: [&str; 4] = ["std", "path", "PathBuf", "as_path"];
 pub const PATH_TO_PATH_BUF: [&str; 4] = ["std", "path", "Path", "to_path_buf"];
 pub const PERMISSIONS: [&str; 3] = ["std", "fs", "Permissions"];
+#[cfg_attr(not(unix), allow(clippy::invalid_paths))]
 pub const PERMISSIONS_FROM_MODE: [&str; 6] = ["std", "os", "unix", "fs", "PermissionsExt", "from_mode"];
 pub const POLL: [&str; 4] = ["core", "task", "poll", "Poll"];
 pub const POLL_PENDING: [&str; 5] = ["core", "task", "poll", "Poll", "Pending"];

tests/cargo/mod.rs

@@ -1,4 +0,0 @@
-#[must_use]
-pub fn is_rustc_test_suite() -> bool {
-    option_env!("RUSTC_TEST_SUITE").is_some()
-}

tests/compile-test.rs

@@ -1,4 +1,5 @@
 #![feature(test)] // compiletest_rs requires this attribute
+#![feature(once_cell)]
 #![cfg_attr(feature = "deny-warnings", deny(warnings))]
 #![warn(rust_2018_idioms, unused_lifetimes)]
 
@@ -11,8 +12,9 @@ use std::ffi::{OsStr, OsString};
 use std::fs;
 use std::io;
 use std::path::{Path, PathBuf};
+use test_utils::IS_RUSTC_TEST_SUITE;
 
-mod cargo;
+mod test_utils;
 
 // whether to run internal tests or not
 const RUN_INTERNAL_TESTS: bool = cfg!(feature = "internal");
@@ -304,7 +306,7 @@ fn run_ui_cargo(config: &mut compiletest::Config) {
         Ok(result)
     }
 
-    if cargo::is_rustc_test_suite() {
+    if IS_RUSTC_TEST_SUITE {
         return;
     }
 

tests/dogfood.rs

@@ -3,179 +3,26 @@
 //!
 //! See [Eating your own dog food](https://en.wikipedia.org/wiki/Eating_your_own_dog_food) for context
 
-// Dogfood cannot run on Windows
-#![cfg(not(windows))]
 #![feature(once_cell)]
 #![cfg_attr(feature = "deny-warnings", deny(warnings))]
 #![warn(rust_2018_idioms, unused_lifetimes)]
 
-use std::lazy::SyncLazy;
 use std::path::PathBuf;
 use std::process::Command;
+use test_utils::IS_RUSTC_TEST_SUITE;
 
-mod cargo;
-
-static CLIPPY_PATH: SyncLazy<PathBuf> = SyncLazy::new(|| {
-    let mut path = std::env::current_exe().unwrap();
-    assert!(path.pop()); // deps
-    path.set_file_name("cargo-clippy");
-    path
-});
+mod test_utils;
 
 #[test]
 fn dogfood_clippy() {
-    // run clippy on itself and fail the test if lint warnings are reported
-    if cargo::is_rustc_test_suite() {
-        return;
-    }
-    let root_dir = PathBuf::from(env!("CARGO_MANIFEST_DIR"));
-
-    let mut command = Command::new(&*CLIPPY_PATH);
-    command
-        .current_dir(root_dir)
-        .env("CARGO_INCREMENTAL", "0")
-        .arg("clippy")
-        .arg("--all-targets")
-        .arg("--all-features")
-        .arg("--")
-        .args(&["-D", "clippy::all"])
-        .args(&["-D", "clippy::pedantic"])
-        .arg("-Cdebuginfo=0"); // disable debuginfo to generate less data in the target dir
-
-    // internal lints only exist if we build with the internal feature
-    if cfg!(feature = "internal") {
-        command.args(&["-D", "clippy::internal"]);
-    }
-
-    let output = command.output().unwrap();
-
-    println!("status: {}", output.status);
-    println!("stdout: {}", String::from_utf8_lossy(&output.stdout));
-    println!("stderr: {}", String::from_utf8_lossy(&output.stderr));
-
-    assert!(output.status.success());
-}
-
-fn test_no_deps_ignores_path_deps_in_workspaces() {
-    if cargo::is_rustc_test_suite() {
-        return;
-    }
-    let root = PathBuf::from(env!("CARGO_MANIFEST_DIR"));
-    let target_dir = root.join("target").join("dogfood");
-    let cwd = root.join("clippy_workspace_tests");
-
-    // Make sure we start with a clean state
-    Command::new("cargo")
-        .current_dir(&cwd)
-        .env("CARGO_TARGET_DIR", &target_dir)
-        .arg("clean")
-        .args(&["-p", "subcrate"])
-        .args(&["-p", "path_dep"])
-        .output()
-        .unwrap();
-
-    // `path_dep` is a path dependency of `subcrate` that would trigger a denied lint.
-    // Make sure that with the `--no-deps` argument Clippy does not run on `path_dep`.
-    let output = Command::new(&*CLIPPY_PATH)
-        .current_dir(&cwd)
-        .env("CARGO_INCREMENTAL", "0")
-        .arg("clippy")
-        .args(&["-p", "subcrate"])
-        .arg("--no-deps")
-        .arg("--")
-        .arg("-Cdebuginfo=0") // disable debuginfo to generate less data in the target dir
-        .args(&["--cfg", r#"feature="primary_package_test""#])
-        .output()
-        .unwrap();
-    println!("status: {}", output.status);
-    println!("stdout: {}", String::from_utf8_lossy(&output.stdout));
-    println!("stderr: {}", String::from_utf8_lossy(&output.stderr));
-
-    assert!(output.status.success());
-
-    let lint_path_dep = || {
-        // Test that without the `--no-deps` argument, `path_dep` is linted.
-        let output = Command::new(&*CLIPPY_PATH)
-            .current_dir(&cwd)
-            .env("CARGO_INCREMENTAL", "0")
-            .arg("clippy")
-            .args(&["-p", "subcrate"])
-            .arg("--")
-            .arg("-Cdebuginfo=0") // disable debuginfo to generate less data in the target dir
-            .args(&["--cfg", r#"feature="primary_package_test""#])
-            .output()
-            .unwrap();
-        println!("status: {}", output.status);
-        println!("stdout: {}", String::from_utf8_lossy(&output.stdout));
-        println!("stderr: {}", String::from_utf8_lossy(&output.stderr));
-
-        assert!(!output.status.success());
-        assert!(
-            String::from_utf8(output.stderr)
-                .unwrap()
-                .contains("error: empty `loop {}` wastes CPU cycles")
-        );
-    };
-
-    // Make sure Cargo is aware of the removal of `--no-deps`.
-    lint_path_dep();
-
-    let successful_build = || {
-        let output = Command::new(&*CLIPPY_PATH)
-            .current_dir(&cwd)
-            .env("CARGO_INCREMENTAL", "0")
-            .arg("clippy")
-            .args(&["-p", "subcrate"])
-            .arg("--")
-            .arg("-Cdebuginfo=0") // disable debuginfo to generate less data in the target dir
-            .output()
-            .unwrap();
-        println!("status: {}", output.status);
-        println!("stdout: {}", String::from_utf8_lossy(&output.stdout));
-        println!("stderr: {}", String::from_utf8_lossy(&output.stderr));
-
-        assert!(output.status.success());
-
-        output
-    };
-
-    // Trigger a sucessful build, so Cargo would like to cache the build result.
-    successful_build();
-
-    // Make sure there's no spurious rebuild when nothing changes.
-    let stderr = String::from_utf8(successful_build().stderr).unwrap();
-    assert!(!stderr.contains("Compiling"));
-    assert!(!stderr.contains("Checking"));
-    assert!(stderr.contains("Finished"));
-
-    // Make sure Cargo is aware of the new `--cfg` flag.
-    lint_path_dep();
-}
-
-#[test]
-fn dogfood_subprojects() {
-    // run clippy on remaining subprojects and fail the test if lint warnings are reported
-    if cargo::is_rustc_test_suite() {
+    if IS_RUSTC_TEST_SUITE {
         return;
     }
 
-    // NOTE: `path_dep` crate is omitted on purpose here
-    for project in &[
-        "clippy_workspace_tests",
-        "clippy_workspace_tests/src",
-        "clippy_workspace_tests/subcrate",
-        "clippy_workspace_tests/subcrate/src",
-        "clippy_dev",
-        "clippy_lints",
-        "clippy_utils",
-        "rustc_tools_util",
-    ] {
-        run_clippy_for_project(project);
+    // "" is the root package
+    for package in &["", "clippy_dev", "clippy_lints", "clippy_utils", "rustc_tools_util"] {
+        run_clippy_for_package(package);
     }
-
-    // NOTE: Since tests run in parallel we can't run cargo commands on the same workspace at the
-    // same time, so we test this immediately after the dogfood for workspaces.
-    test_no_deps_ignores_path_deps_in_workspaces();
 }
 
 #[test]
@@ -191,7 +38,7 @@ fn run_metadata_collection_lint() {
 
     // Run collection as is
     std::env::set_var("ENABLE_METADATA_COLLECTION", "1");
-    run_clippy_for_project("clippy_lints");
+    run_clippy_for_package("clippy_lints");
 
     // Check if cargo caching got in the way
     if let Ok(file) = File::open(metadata_output_path) {
@@ -214,13 +61,13 @@ fn run_metadata_collection_lint() {
     .unwrap();
 
     // Running the collection again
-    run_clippy_for_project("clippy_lints");
+    run_clippy_for_package("clippy_lints");
 }
 
-fn run_clippy_for_project(project: &str) {
+fn run_clippy_for_package(project: &str) {
     let root_dir = PathBuf::from(env!("CARGO_MANIFEST_DIR"));
 
-    let mut command = Command::new(&*CLIPPY_PATH);
+    let mut command = Command::new(&*test_utils::CARGO_CLIPPY_PATH);
 
     command
         .current_dir(root_dir.join(project))

tests/fmt.rs

@@ -10,14 +10,6 @@ fn fmt() {
         return;
     }
 
-    // Skip this test if nightly rustfmt is unavailable
-    let rustup_output = Command::new("rustup").args(&["component", "list"]).output().unwrap();
-    assert!(rustup_output.status.success());
-    let component_output = String::from_utf8_lossy(&rustup_output.stdout);
-    if !component_output.contains("rustfmt") {
-        return;
-    }
-
     let root_dir = PathBuf::from(env!("CARGO_MANIFEST_DIR"));
     let output = Command::new("cargo")
         .current_dir(root_dir)

tests/test_utils/mod.rs

@@ -0,0 +1,13 @@
+#![allow(dead_code)] // see https://github.com/rust-lang/rust/issues/46379
+
+use std::lazy::SyncLazy;
+use std::path::PathBuf;
+
+pub static CARGO_CLIPPY_PATH: SyncLazy<PathBuf> = SyncLazy::new(|| {
+    let mut path = std::env::current_exe().unwrap();
+    assert!(path.pop()); // deps
+    path.set_file_name("cargo-clippy");
+    path
+});
+
+pub const IS_RUSTC_TEST_SUITE: bool = option_env!("RUSTC_TEST_SUITE").is_some();

tests/workspace.rs

@@ -0,0 +1,107 @@
+#![feature(once_cell)]
+
+use std::path::PathBuf;
+use std::process::Command;
+use test_utils::{CARGO_CLIPPY_PATH, IS_RUSTC_TEST_SUITE};
+
+mod test_utils;
+
+#[test]
+fn test_no_deps_ignores_path_deps_in_workspaces() {
+    if IS_RUSTC_TEST_SUITE {
+        return;
+    }
+    let root = PathBuf::from(env!("CARGO_MANIFEST_DIR"));
+    let target_dir = root.join("target").join("workspace_test");
+    let cwd = root.join("tests/workspace_test");
+
+    // Make sure we start with a clean state
+    Command::new("cargo")
+        .current_dir(&cwd)
+        .env("CARGO_TARGET_DIR", &target_dir)
+        .arg("clean")
+        .args(&["-p", "subcrate"])
+        .args(&["-p", "path_dep"])
+        .output()
+        .unwrap();
+
+    // `path_dep` is a path dependency of `subcrate` that would trigger a denied lint.
+    // Make sure that with the `--no-deps` argument Clippy does not run on `path_dep`.
+    let output = Command::new(&*CARGO_CLIPPY_PATH)
+        .current_dir(&cwd)
+        .env("CARGO_INCREMENTAL", "0")
+        .env("CARGO_TARGET_DIR", &target_dir)
+        .arg("clippy")
+        .args(&["-p", "subcrate"])
+        .arg("--no-deps")
+        .arg("--")
+        .arg("-Cdebuginfo=0") // disable debuginfo to generate less data in the target dir
+        .args(&["--cfg", r#"feature="primary_package_test""#])
+        .output()
+        .unwrap();
+    println!("status: {}", output.status);
+    println!("stdout: {}", String::from_utf8_lossy(&output.stdout));
+    println!("stderr: {}", String::from_utf8_lossy(&output.stderr));
+
+    assert!(output.status.success());
+
+    let lint_path_dep = || {
+        // Test that without the `--no-deps` argument, `path_dep` is linted.
+        let output = Command::new(&*CARGO_CLIPPY_PATH)
+            .current_dir(&cwd)
+            .env("CARGO_INCREMENTAL", "0")
+            .env("CARGO_TARGET_DIR", &target_dir)
+            .arg("clippy")
+            .args(&["-p", "subcrate"])
+            .arg("--")
+            .arg("-Cdebuginfo=0") // disable debuginfo to generate less data in the target dir
+            .args(&["--cfg", r#"feature="primary_package_test""#])
+            .output()
+            .unwrap();
+        println!("status: {}", output.status);
+        println!("stdout: {}", String::from_utf8_lossy(&output.stdout));
+        println!("stderr: {}", String::from_utf8_lossy(&output.stderr));
+
+        assert!(!output.status.success());
+        assert!(
+            String::from_utf8(output.stderr)
+                .unwrap()
+                .contains("error: empty `loop {}` wastes CPU cycles")
+        );
+    };
+
+    // Make sure Cargo is aware of the removal of `--no-deps`.
+    lint_path_dep();
+
+    let successful_build = || {
+        let output = Command::new(&*CARGO_CLIPPY_PATH)
+            .current_dir(&cwd)
+            .env("CARGO_INCREMENTAL", "0")
+            .env("CARGO_TARGET_DIR", &target_dir)
+            .arg("clippy")
+            .args(&["-p", "subcrate"])
+            .arg("--")
+            .arg("-Cdebuginfo=0") // disable debuginfo to generate less data in the target dir
+            .output()
+            .unwrap();
+        println!("status: {}", output.status);
+        println!("stdout: {}", String::from_utf8_lossy(&output.stdout));
+        println!("stderr: {}", String::from_utf8_lossy(&output.stderr));
+
+        assert!(output.status.success());
+
+        output
+    };
+
+    // Trigger a sucessful build, so Cargo would like to cache the build result.
+    successful_build();
+
+    // Make sure there's no spurious rebuild when nothing changes.
+    let stderr = String::from_utf8(successful_build().stderr).unwrap();
+    assert!(!stderr.contains("Compiling"));
+    assert!(!stderr.contains("Checking"));
+    assert!(stderr.contains("Finished"));
+
+    // Make sure Cargo is aware of the new `--cfg` flag.
+    lint_path_dep();
+}

tests/workspace_test/Cargo.toml

@@ -1,5 +1,5 @@
 [package]
-name = "clippy_workspace_tests"
+name = "workspace_test"
 version = "0.1.0"
 edition = "2018"