Skip to content

Some test code cleanup #8266

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Jan 12, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 0 additions & 8 deletions .github/workflows/clippy.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
13 changes: 5 additions & 8 deletions .github/workflows/clippy_bors.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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:
Expand Down
1 change: 0 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions clippy_utils/src/paths.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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"];
Expand Down
4 changes: 0 additions & 4 deletions tests/cargo/mod.rs

This file was deleted.

6 changes: 4 additions & 2 deletions tests/compile-test.rs
Original file line number Diff line number Diff line change
@@ -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)]

Expand All @@ -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");
Expand Down Expand Up @@ -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;
}

Expand Down
173 changes: 10 additions & 163 deletions tests/dogfood.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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))]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The windows and mac runners are our slowest runners in CI and also the current bottleneck. This added another 1-2min to the windows runner. So I wonder if we should only run those tests on unix. An argument against that is, that contributors using windows/mac, won't see dogfood errors until they submit a PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for reviewing this.

Maybe I can find a way to get the best of both worlds using a flag or environment variable.

#![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]
Expand All @@ -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) {
Expand All @@ -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))
Expand Down
8 changes: 0 additions & 8 deletions tests/fmt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
13 changes: 13 additions & 0 deletions tests/test_utils/mod.rs
Original file line number Diff line number Diff line change
@@ -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();
Loading