From 3c29c3e6fc6715ee749c75b4205e98de95fca10a Mon Sep 17 00:00:00 2001 From: Caleb Cartwright Date: Wed, 4 Sep 2019 18:26:10 -0500 Subject: [PATCH 1/7] feat: support files in `tests` subdirs --- src/cargo-fmt/main.rs | 272 +++++++++++++++++++++++++++++++++++------- 1 file changed, 226 insertions(+), 46 deletions(-) diff --git a/src/cargo-fmt/main.rs b/src/cargo-fmt/main.rs index 96d1a23b47c..ea2193f7b39 100644 --- a/src/cargo-fmt/main.rs +++ b/src/cargo-fmt/main.rs @@ -3,6 +3,7 @@ #![deny(warnings)] use cargo_metadata; +use ignore::{self, gitignore}; use std::cmp::Ordering; use std::collections::{BTreeMap, BTreeSet}; @@ -60,6 +61,10 @@ pub struct Opts { /// Run rustfmt in check mode #[structopt(long = "check")] check: bool, + + /// Format files located within subdirectories of `tests` directory + #[structopt(long = "include-nested-test-files")] + include_nested_test_files: bool, } fn main() { @@ -122,6 +127,13 @@ fn execute() -> i32 { } } + let include_nested_test_files = opts.include_nested_test_files; + let ignored_nested_test_files = if include_nested_test_files { + opts.ignored_nested_test_files + } else { + vec![] + }; + if let Some(specified_manifest_path) = opts.manifest_path { if !specified_manifest_path.ends_with("Cargo.toml") { print_usage_to_stderr("the manifest-path must be a path to a Cargo.toml file"); @@ -133,9 +145,18 @@ fn execute() -> i32 { &strategy, rustfmt_args, Some(&manifest_path), + include_nested_test_files, + &ignored_nested_test_files, )) } else { - handle_command_status(format_crate(verbosity, &strategy, rustfmt_args, None)) + handle_command_status(format_crate( + verbosity, + &strategy, + rustfmt_args, + None, + include_nested_test_files, + &ignored_nested_test_files, + )) } } @@ -239,8 +260,15 @@ fn format_crate( strategy: &CargoFmtStrategy, rustfmt_args: Vec, manifest_path: Option<&Path>, + include_nested_test_files: bool, + ignored_nested_test_files: &Vec, ) -> Result { - let targets = get_targets(strategy, manifest_path)?; + let targets = get_targets( + strategy, + manifest_path, + include_nested_test_files, + &ignored_nested_test_files, + )?; // Currently only bin and lib files get formatted. run_rustfmt(&targets, &rustfmt_args, verbosity) @@ -255,17 +283,27 @@ pub struct Target { kind: String, /// Rust edition for this target. edition: String, + // Rust files residing within subdirectories of the tests directory. + nested_int_test_files: Vec, } impl Target { - pub fn from_target(target: &cargo_metadata::Target) -> Self { + pub fn from_target( + target: &cargo_metadata::Target, + nested_int_test_files: Option>, + ) -> Self { let path = PathBuf::from(&target.src_path); let canonicalized = fs::canonicalize(&path).unwrap_or(path); + let test_files = match nested_int_test_files { + Some(files) => files, + None => vec![], + }; Target { path: canonicalized, kind: target.kind[0].clone(), edition: target.edition.clone(), + nested_int_test_files: test_files, } } } @@ -320,17 +358,32 @@ impl CargoFmtStrategy { fn get_targets( strategy: &CargoFmtStrategy, manifest_path: Option<&Path>, + include_nested_test_files: bool, + ignored_nested_test_files: &Vec, ) -> Result, io::Error> { let mut targets = BTreeSet::new(); match *strategy { - CargoFmtStrategy::Root => get_targets_root_only(manifest_path, &mut targets)?, - CargoFmtStrategy::All => { - get_targets_recursive(manifest_path, &mut targets, &mut BTreeSet::new())? - } - CargoFmtStrategy::Some(ref hitlist) => { - get_targets_with_hitlist(manifest_path, hitlist, &mut targets)? - } + CargoFmtStrategy::Root => get_targets_root_only( + manifest_path, + &mut targets, + include_nested_test_files, + &ignored_nested_test_files, + )?, + CargoFmtStrategy::All => get_targets_recursive( + manifest_path, + &mut targets, + &mut BTreeSet::new(), + include_nested_test_files, + &ignored_nested_test_files, + )?, + CargoFmtStrategy::Some(ref hitlist) => get_targets_with_hitlist( + manifest_path, + hitlist, + &mut targets, + include_nested_test_files, + &ignored_nested_test_files, + )?, } if targets.is_empty() { @@ -345,7 +398,9 @@ fn get_targets( fn get_targets_root_only( manifest_path: Option<&Path>, - targets: &mut BTreeSet, + mut targets: &mut BTreeSet, + include_nested_test_files: bool, + ignored_nested_test_files: &Vec, ) -> Result<(), io::Error> { let metadata = get_cargo_metadata(manifest_path, false)?; let workspace_root_path = PathBuf::from(&metadata.workspace_root).canonicalize()?; @@ -362,26 +417,36 @@ fn get_targets_root_only( ) }; - let package_targets = match metadata.packages.len() { - 1 => metadata.packages.into_iter().next().unwrap().targets, - _ => metadata - .packages - .into_iter() - .filter(|p| { - in_workspace_root - || PathBuf::from(&p.manifest_path) - .canonicalize() - .unwrap_or_default() - == current_dir_manifest - }) - .map(|p| p.targets) - .flatten() - .collect(), + let (package_targets, package_manifest_path) = match metadata.packages.len() { + 1 => { + let package = metadata.packages.into_iter().next().unwrap(); + (package.targets, PathBuf::from(&package.manifest_path)) + } + _ => ( + metadata + .packages + .into_iter() + .filter(|p| { + in_workspace_root + || PathBuf::from(&p.manifest_path) + .canonicalize() + .unwrap_or_default() + == current_dir_manifest + }) + .map(|p| p.targets) + .flatten() + .collect(), + PathBuf::from(current_dir_manifest), + ), }; - for target in package_targets { - targets.insert(Target::from_target(&target)); - } + add_targets( + &package_manifest_path, + &package_targets, + &mut targets, + include_nested_test_files, + &ignored_nested_test_files, + ); Ok(()) } @@ -390,12 +455,20 @@ fn get_targets_recursive( manifest_path: Option<&Path>, mut targets: &mut BTreeSet, visited: &mut BTreeSet, + include_nested_test_files: bool, + ignored_nested_test_files: &Vec, ) -> Result<(), io::Error> { let metadata = get_cargo_metadata(manifest_path, false)?; let metadata_with_deps = get_cargo_metadata(manifest_path, true)?; for package in metadata.packages { - add_targets(&package.targets, &mut targets); + add_targets( + &package.manifest_path, + &package.targets, + &mut targets, + include_nested_test_files, + &ignored_nested_test_files, + ); // Look for local dependencies. for dependency in package.dependencies { @@ -407,19 +480,27 @@ fn get_targets_recursive( .packages .iter() .find(|p| p.name == dependency.name && p.source.is_none()); - let manifest_path = if dependency_package.is_some() { - PathBuf::from(&dependency_package.unwrap().manifest_path) - } else { - let mut package_manifest_path = PathBuf::from(&package.manifest_path); - package_manifest_path.pop(); - package_manifest_path.push(&dependency.name); - package_manifest_path.push("Cargo.toml"); - package_manifest_path + + let manifest_path = match dependency_package { + Some(p) => PathBuf::from(&p.manifest_path), + None => { + let mut package_manifest_path = PathBuf::from(&package.manifest_path); + package_manifest_path.pop(); + package_manifest_path.push(&dependency.name); + package_manifest_path.push("Cargo.toml"); + package_manifest_path + } }; if manifest_path.exists() { visited.insert(dependency.name); - get_targets_recursive(Some(&manifest_path), &mut targets, visited)?; + get_targets_recursive( + Some(&manifest_path), + &mut targets, + visited, + include_nested_test_files, + &ignored_nested_test_files, + )?; } } } @@ -430,7 +511,9 @@ fn get_targets_recursive( fn get_targets_with_hitlist( manifest_path: Option<&Path>, hitlist: &[String], - targets: &mut BTreeSet, + mut targets: &mut BTreeSet, + include_nested_test_files: bool, + ignored_nested_test_files: &Vec, ) -> Result<(), io::Error> { let metadata = get_cargo_metadata(manifest_path, false)?; @@ -438,9 +521,13 @@ fn get_targets_with_hitlist( for package in metadata.packages { if workspace_hitlist.remove(&package.name) { - for target in package.targets { - targets.insert(Target::from_target(&target)); - } + add_targets( + &package.manifest_path, + &package.targets, + &mut targets, + include_nested_test_files, + &ignored_nested_test_files, + ); } } @@ -455,10 +542,99 @@ fn get_targets_with_hitlist( } } -fn add_targets(target_paths: &[cargo_metadata::Target], targets: &mut BTreeSet) { +fn add_targets( + manifest_path: &PathBuf, + target_paths: &[cargo_metadata::Target], + targets: &mut BTreeSet, + include_nested_test_files: bool, + ignore_list: &Vec, +) { + let mut test_files_added = false; for target in target_paths { - targets.insert(Target::from_target(target)); + // Packages often have more than one `test` target, + // so only add the nested files for the first one. + let test_files = if include_nested_test_files + && !test_files_added + && target.kind.iter().any(|t| t == "test") + { + // If any errors are encountered, just fallback to ignoring + // nested files in subdirectories of `tests` + match manifest_path.parent() { + None => None, + Some(package_dir) => { + let target_dir = package_dir.join("tests"); + let mut ignore_builder = gitignore::GitignoreBuilder::new(package_dir); + let mut had_ignore_add_error = false; + + for ignore_path in ignore_list { + if ignore_builder.add_line(None, &ignore_path).is_err() { + had_ignore_add_error = true; + break; + }; + } + + if had_ignore_add_error { + None + } else { + match ignore_builder.build() { + Err(_) => None, + Ok(ignore_set) => { + test_files_added = true; + Some(get_nested_integration_test_files( + &target_dir, + &target_dir, + &ignore_set, + )) + } + } + } + } + } + } else { + None + }; + targets.insert(Target::from_target(target, test_files)); + } +} + +// Returns a `Vec` containing `PathBuf`s of files nested .rs files within a subdirectory +// under the `tests` directory for a given package. +// https://github.com/rust-lang/rustfmt/issues/1820 +fn get_nested_integration_test_files( + path: &Path, + root_dir: &Path, + ignore_set: &gitignore::Gitignore, +) -> Vec { + let mut files = vec![]; + if path.is_dir() + && !ignore_set + .matched_path_or_any_parents(&path, true) + .is_ignore() + { + for entry in fs::read_dir(path).expect(&format!( + "couldn't read directory {}", + path.to_str().unwrap() + )) { + let entry = entry.expect("couldn't get `DirEntry`"); + let path = entry.path(); + if !ignore_set + .matched_path_or_any_parents(&path, false) + .is_ignore() + { + let parent = path.parent().expect("couldn't get parent directory"); + if path.is_dir() { + files.append(&mut get_nested_integration_test_files( + &path, + &root_dir, + &ignore_set, + )); + } else if path.extension().map_or(false, |f| f == "rs") && parent != root_dir { + files.push(path); + } + } + } } + files } fn run_rustfmt( @@ -474,7 +650,11 @@ fn run_rustfmt( } }) .fold(BTreeMap::new(), |mut h, t| { - h.entry(&t.edition).or_insert_with(Vec::new).push(&t.path); + let entry = h.entry(&t.edition).or_insert_with(Vec::new); + for test_file in &t.nested_int_test_files { + entry.push(test_file) + } + entry.push(&t.path); h }); From bd7da4c6278769f5fb94f3b9367e4b06fdc235fe Mon Sep 17 00:00:00 2001 From: Caleb Cartwright Date: Thu, 12 Sep 2019 18:44:06 -0500 Subject: [PATCH 2/7] refactor: remove nested test file ignore cli option --- src/cargo-fmt/main.rs | 94 ++++++------------------------------------- 1 file changed, 13 insertions(+), 81 deletions(-) diff --git a/src/cargo-fmt/main.rs b/src/cargo-fmt/main.rs index ea2193f7b39..89f23491daa 100644 --- a/src/cargo-fmt/main.rs +++ b/src/cargo-fmt/main.rs @@ -3,7 +3,6 @@ #![deny(warnings)] use cargo_metadata; -use ignore::{self, gitignore}; use std::cmp::Ordering; use std::collections::{BTreeMap, BTreeSet}; @@ -128,11 +127,6 @@ fn execute() -> i32 { } let include_nested_test_files = opts.include_nested_test_files; - let ignored_nested_test_files = if include_nested_test_files { - opts.ignored_nested_test_files - } else { - vec![] - }; if let Some(specified_manifest_path) = opts.manifest_path { if !specified_manifest_path.ends_with("Cargo.toml") { @@ -146,7 +140,6 @@ fn execute() -> i32 { rustfmt_args, Some(&manifest_path), include_nested_test_files, - &ignored_nested_test_files, )) } else { handle_command_status(format_crate( @@ -155,7 +148,6 @@ fn execute() -> i32 { rustfmt_args, None, include_nested_test_files, - &ignored_nested_test_files, )) } } @@ -261,14 +253,8 @@ fn format_crate( rustfmt_args: Vec, manifest_path: Option<&Path>, include_nested_test_files: bool, - ignored_nested_test_files: &Vec, ) -> Result { - let targets = get_targets( - strategy, - manifest_path, - include_nested_test_files, - &ignored_nested_test_files, - )?; + let targets = get_targets(strategy, manifest_path, include_nested_test_files)?; // Currently only bin and lib files get formatted. run_rustfmt(&targets, &rustfmt_args, verbosity) @@ -359,30 +345,24 @@ fn get_targets( strategy: &CargoFmtStrategy, manifest_path: Option<&Path>, include_nested_test_files: bool, - ignored_nested_test_files: &Vec, ) -> Result, io::Error> { let mut targets = BTreeSet::new(); match *strategy { - CargoFmtStrategy::Root => get_targets_root_only( - manifest_path, - &mut targets, - include_nested_test_files, - &ignored_nested_test_files, - )?, + CargoFmtStrategy::Root => { + get_targets_root_only(manifest_path, &mut targets, include_nested_test_files)? + } CargoFmtStrategy::All => get_targets_recursive( manifest_path, &mut targets, &mut BTreeSet::new(), include_nested_test_files, - &ignored_nested_test_files, )?, CargoFmtStrategy::Some(ref hitlist) => get_targets_with_hitlist( manifest_path, hitlist, &mut targets, include_nested_test_files, - &ignored_nested_test_files, )?, } @@ -400,7 +380,6 @@ fn get_targets_root_only( manifest_path: Option<&Path>, mut targets: &mut BTreeSet, include_nested_test_files: bool, - ignored_nested_test_files: &Vec, ) -> Result<(), io::Error> { let metadata = get_cargo_metadata(manifest_path, false)?; let workspace_root_path = PathBuf::from(&metadata.workspace_root).canonicalize()?; @@ -445,7 +424,6 @@ fn get_targets_root_only( &package_targets, &mut targets, include_nested_test_files, - &ignored_nested_test_files, ); Ok(()) @@ -456,7 +434,6 @@ fn get_targets_recursive( mut targets: &mut BTreeSet, visited: &mut BTreeSet, include_nested_test_files: bool, - ignored_nested_test_files: &Vec, ) -> Result<(), io::Error> { let metadata = get_cargo_metadata(manifest_path, false)?; let metadata_with_deps = get_cargo_metadata(manifest_path, true)?; @@ -467,7 +444,6 @@ fn get_targets_recursive( &package.targets, &mut targets, include_nested_test_files, - &ignored_nested_test_files, ); // Look for local dependencies. @@ -499,7 +475,6 @@ fn get_targets_recursive( &mut targets, visited, include_nested_test_files, - &ignored_nested_test_files, )?; } } @@ -513,7 +488,6 @@ fn get_targets_with_hitlist( hitlist: &[String], mut targets: &mut BTreeSet, include_nested_test_files: bool, - ignored_nested_test_files: &Vec, ) -> Result<(), io::Error> { let metadata = get_cargo_metadata(manifest_path, false)?; @@ -526,7 +500,6 @@ fn get_targets_with_hitlist( &package.targets, &mut targets, include_nested_test_files, - &ignored_nested_test_files, ); } } @@ -547,7 +520,6 @@ fn add_targets( target_paths: &[cargo_metadata::Target], targets: &mut BTreeSet, include_nested_test_files: bool, - ignore_list: &Vec, ) { let mut test_files_added = false; for target in target_paths { @@ -563,31 +535,8 @@ fn add_targets( None => None, Some(package_dir) => { let target_dir = package_dir.join("tests"); - let mut ignore_builder = gitignore::GitignoreBuilder::new(package_dir); - let mut had_ignore_add_error = false; - - for ignore_path in ignore_list { - if ignore_builder.add_line(None, &ignore_path).is_err() { - had_ignore_add_error = true; - break; - }; - } - - if had_ignore_add_error { - None - } else { - match ignore_builder.build() { - Err(_) => None, - Ok(ignore_set) => { - test_files_added = true; - Some(get_nested_integration_test_files( - &target_dir, - &target_dir, - &ignore_set, - )) - } - } - } + test_files_added = true; + Some(get_nested_integration_test_files(&target_dir, &target_dir)) } } } else { @@ -600,37 +549,20 @@ fn add_targets( // Returns a `Vec` containing `PathBuf`s of files nested .rs files within a subdirectory // under the `tests` directory for a given package. // https://github.com/rust-lang/rustfmt/issues/1820 -fn get_nested_integration_test_files( - path: &Path, - root_dir: &Path, - ignore_set: &gitignore::Gitignore, -) -> Vec { +fn get_nested_integration_test_files(path: &Path, root_dir: &Path) -> Vec { let mut files = vec![]; - if path.is_dir() - && !ignore_set - .matched_path_or_any_parents(&path, true) - .is_ignore() - { + if path.is_dir() { for entry in fs::read_dir(path).expect(&format!( "couldn't read directory {}", path.to_str().unwrap() )) { let entry = entry.expect("couldn't get `DirEntry`"); let path = entry.path(); - if !ignore_set - .matched_path_or_any_parents(&path, false) - .is_ignore() - { - let parent = path.parent().expect("couldn't get parent directory"); - if path.is_dir() { - files.append(&mut get_nested_integration_test_files( - &path, - &root_dir, - &ignore_set, - )); - } else if path.extension().map_or(false, |f| f == "rs") && parent != root_dir { - files.push(path); - } + let parent = path.parent().expect("couldn't get parent directory"); + if path.is_dir() { + files.append(&mut get_nested_integration_test_files(&path, &root_dir)); + } else if path.extension().map_or(false, |f| f == "rs") && parent != root_dir { + files.push(path); } } } From ec62d72e8c36247f67932999f2120a088ff620e2 Mon Sep 17 00:00:00 2001 From: Caleb Cartwright Date: Mon, 7 Oct 2019 20:11:46 -0500 Subject: [PATCH 3/7] tests: add tests for nested int test files --- src/cargo-fmt/main.rs | 54 +++++++++++++++---- .../empty-tests-dir/Cargo.toml | 6 +++ .../empty-tests-dir/src/lib.rs | 1 + .../nested-test-files/no-tests-dir/Cargo.toml | 6 +++ .../nested-test-files/no-tests-dir/src/lib.rs | 1 + .../only-root-level-tests/Cargo.toml | 6 +++ .../only-root-level-tests/src/lib.rs | 1 + .../only-root-level-tests/tests/bar.rs | 4 ++ .../only-root-level-tests/tests/foo.rs | 4 ++ .../root-and-nested-tests/Cargo.toml | 6 +++ .../root-and-nested-tests/src/lib.rs | 1 + .../root-and-nested-tests/tests/bar.rs | 4 ++ .../root-and-nested-tests/tests/foo.rs | 4 ++ .../tests/nested/foo_bar.rs | 4 ++ 14 files changed, 93 insertions(+), 9 deletions(-) create mode 100644 tests/nested-test-files/empty-tests-dir/Cargo.toml create mode 100644 tests/nested-test-files/empty-tests-dir/src/lib.rs create mode 100644 tests/nested-test-files/no-tests-dir/Cargo.toml create mode 100644 tests/nested-test-files/no-tests-dir/src/lib.rs create mode 100644 tests/nested-test-files/only-root-level-tests/Cargo.toml create mode 100644 tests/nested-test-files/only-root-level-tests/src/lib.rs create mode 100644 tests/nested-test-files/only-root-level-tests/tests/bar.rs create mode 100644 tests/nested-test-files/only-root-level-tests/tests/foo.rs create mode 100644 tests/nested-test-files/root-and-nested-tests/Cargo.toml create mode 100644 tests/nested-test-files/root-and-nested-tests/src/lib.rs create mode 100644 tests/nested-test-files/root-and-nested-tests/tests/bar.rs create mode 100644 tests/nested-test-files/root-and-nested-tests/tests/foo.rs create mode 100644 tests/nested-test-files/root-and-nested-tests/tests/nested/foo_bar.rs diff --git a/src/cargo-fmt/main.rs b/src/cargo-fmt/main.rs index 89f23491daa..72b434c32ce 100644 --- a/src/cargo-fmt/main.rs +++ b/src/cargo-fmt/main.rs @@ -546,8 +546,8 @@ fn add_targets( } } -// Returns a `Vec` containing `PathBuf`s of files nested .rs files within a subdirectory -// under the `tests` directory for a given package. +// Returns a `Vec` containing `PathBuf`s of nested .rs files within subdirectories +// of the `tests` directory for a given package. // https://github.com/rust-lang/rustfmt/issues/1820 fn get_nested_integration_test_files(path: &Path, root_dir: &Path) -> Vec { let mut files = vec![]; @@ -556,13 +556,16 @@ fn get_nested_integration_test_files(path: &Path, root_dir: &Path) -> Vec, + get_nested_integration_test_files(&target_dir, &target_dir), + ) + } + + #[test] + fn returns_no_files_if_tests_has_no_nested_files() { + let target_dir = Path::new("tests/nested-test-files/only-root-level-tests/tests"); + assert_eq!( + Vec::new() as Vec, + get_nested_integration_test_files(&target_dir, &target_dir), + ) + } + + #[test] + fn returns_nested_files() { + let target_dir = Path::new("tests/nested-test-files/root-and-nested-tests/tests"); + assert_eq!( + vec![PathBuf::from( + "tests/nested-test-files/root-and-nested-tests/tests/nested/foo_bar.rs" + )], + get_nested_integration_test_files(&target_dir, &target_dir), + ) + } + } } diff --git a/tests/nested-test-files/empty-tests-dir/Cargo.toml b/tests/nested-test-files/empty-tests-dir/Cargo.toml new file mode 100644 index 00000000000..27c0eeb1ba4 --- /dev/null +++ b/tests/nested-test-files/empty-tests-dir/Cargo.toml @@ -0,0 +1,6 @@ +[package] +name = "empty-tests-dir" +version = "0.1.0" +authors = ["rustfmt devs "] + +[dependencies] diff --git a/tests/nested-test-files/empty-tests-dir/src/lib.rs b/tests/nested-test-files/empty-tests-dir/src/lib.rs new file mode 100644 index 00000000000..8f3b7ef112a --- /dev/null +++ b/tests/nested-test-files/empty-tests-dir/src/lib.rs @@ -0,0 +1 @@ +fn foo() {} diff --git a/tests/nested-test-files/no-tests-dir/Cargo.toml b/tests/nested-test-files/no-tests-dir/Cargo.toml new file mode 100644 index 00000000000..c48c1588c8f --- /dev/null +++ b/tests/nested-test-files/no-tests-dir/Cargo.toml @@ -0,0 +1,6 @@ +[package] +name = "no-tests" +version = "0.1.0" +authors = ["rustfmt devs "] + +[dependencies] diff --git a/tests/nested-test-files/no-tests-dir/src/lib.rs b/tests/nested-test-files/no-tests-dir/src/lib.rs new file mode 100644 index 00000000000..8f3b7ef112a --- /dev/null +++ b/tests/nested-test-files/no-tests-dir/src/lib.rs @@ -0,0 +1 @@ +fn foo() {} diff --git a/tests/nested-test-files/only-root-level-tests/Cargo.toml b/tests/nested-test-files/only-root-level-tests/Cargo.toml new file mode 100644 index 00000000000..29ea5aa6254 --- /dev/null +++ b/tests/nested-test-files/only-root-level-tests/Cargo.toml @@ -0,0 +1,6 @@ +[package] +name = "foo" +version = "0.1.0" +authors = ["rustfmt devs "] + +[dependencies] diff --git a/tests/nested-test-files/only-root-level-tests/src/lib.rs b/tests/nested-test-files/only-root-level-tests/src/lib.rs new file mode 100644 index 00000000000..8f3b7ef112a --- /dev/null +++ b/tests/nested-test-files/only-root-level-tests/src/lib.rs @@ -0,0 +1 @@ +fn foo() {} diff --git a/tests/nested-test-files/only-root-level-tests/tests/bar.rs b/tests/nested-test-files/only-root-level-tests/tests/bar.rs new file mode 100644 index 00000000000..2b536c616e1 --- /dev/null +++ b/tests/nested-test-files/only-root-level-tests/tests/bar.rs @@ -0,0 +1,4 @@ +#[test] +fn test2() { + assert!(true); +} diff --git a/tests/nested-test-files/only-root-level-tests/tests/foo.rs b/tests/nested-test-files/only-root-level-tests/tests/foo.rs new file mode 100644 index 00000000000..8629b8e804b --- /dev/null +++ b/tests/nested-test-files/only-root-level-tests/tests/foo.rs @@ -0,0 +1,4 @@ +#[test] +fn test1() { + assert_eq!(1, 1); +} diff --git a/tests/nested-test-files/root-and-nested-tests/Cargo.toml b/tests/nested-test-files/root-and-nested-tests/Cargo.toml new file mode 100644 index 00000000000..27c0eeb1ba4 --- /dev/null +++ b/tests/nested-test-files/root-and-nested-tests/Cargo.toml @@ -0,0 +1,6 @@ +[package] +name = "empty-tests-dir" +version = "0.1.0" +authors = ["rustfmt devs "] + +[dependencies] diff --git a/tests/nested-test-files/root-and-nested-tests/src/lib.rs b/tests/nested-test-files/root-and-nested-tests/src/lib.rs new file mode 100644 index 00000000000..8f3b7ef112a --- /dev/null +++ b/tests/nested-test-files/root-and-nested-tests/src/lib.rs @@ -0,0 +1 @@ +fn foo() {} diff --git a/tests/nested-test-files/root-and-nested-tests/tests/bar.rs b/tests/nested-test-files/root-and-nested-tests/tests/bar.rs new file mode 100644 index 00000000000..2b536c616e1 --- /dev/null +++ b/tests/nested-test-files/root-and-nested-tests/tests/bar.rs @@ -0,0 +1,4 @@ +#[test] +fn test2() { + assert!(true); +} diff --git a/tests/nested-test-files/root-and-nested-tests/tests/foo.rs b/tests/nested-test-files/root-and-nested-tests/tests/foo.rs new file mode 100644 index 00000000000..8629b8e804b --- /dev/null +++ b/tests/nested-test-files/root-and-nested-tests/tests/foo.rs @@ -0,0 +1,4 @@ +#[test] +fn test1() { + assert_eq!(1, 1); +} diff --git a/tests/nested-test-files/root-and-nested-tests/tests/nested/foo_bar.rs b/tests/nested-test-files/root-and-nested-tests/tests/nested/foo_bar.rs new file mode 100644 index 00000000000..f965be72004 --- /dev/null +++ b/tests/nested-test-files/root-and-nested-tests/tests/nested/foo_bar.rs @@ -0,0 +1,4 @@ +#[test] +fn test_false() { + assert_ne!(false, true); +} From 424f7ae77ccd35cf70ddfa678a3f77df2fc088c7 Mon Sep 17 00:00:00 2001 From: Caleb Cartwright Date: Mon, 14 Oct 2019 18:53:49 -0500 Subject: [PATCH 4/7] tests: more cargo fmt tests for nested test files --- src/cargo-fmt/main.rs | 168 +++++++++++++++++- .../Cargo.toml | 0 .../src/lib.rs | 0 .../tests/bar.rs | 0 .../tests/foo.rs | 0 .../root-and-nested-tests/Cargo.toml | 3 +- .../tests/nested/deeply-nested/baz.rs | 4 + .../tests/nested/other.rs | 4 + 8 files changed, 173 insertions(+), 6 deletions(-) rename tests/nested-test-files/{only-root-level-tests => only-root-level-tests-dir}/Cargo.toml (100%) rename tests/nested-test-files/{only-root-level-tests => only-root-level-tests-dir}/src/lib.rs (100%) rename tests/nested-test-files/{only-root-level-tests => only-root-level-tests-dir}/tests/bar.rs (100%) rename tests/nested-test-files/{only-root-level-tests => only-root-level-tests-dir}/tests/foo.rs (100%) create mode 100644 tests/nested-test-files/root-and-nested-tests/tests/nested/deeply-nested/baz.rs create mode 100644 tests/nested-test-files/root-and-nested-tests/tests/nested/other.rs diff --git a/src/cargo-fmt/main.rs b/src/cargo-fmt/main.rs index 72b434c32ce..b317c84d2a4 100644 --- a/src/cargo-fmt/main.rs +++ b/src/cargo-fmt/main.rs @@ -529,8 +529,6 @@ fn add_targets( && !test_files_added && target.kind.iter().any(|t| t == "test") { - // If any errors are encountered, just fallback to ignoring - // nested files in subdirectories of `tests` match manifest_path.parent() { None => None, Some(package_dir) => { @@ -902,12 +900,172 @@ mod cargo_fmt_tests { #[test] fn returns_nested_files() { let target_dir = Path::new("tests/nested-test-files/root-and-nested-tests/tests"); + let exp_baz = PathBuf::from( + "tests/nested-test-files/root-and-nested-tests/tests/nested/deeply-nested/baz.rs", + ); + let exp_foo_bar = PathBuf::from( + "tests/nested-test-files/root-and-nested-tests/tests/nested/foo_bar.rs", + ); + let exp_other = PathBuf::from( + "tests/nested-test-files/root-and-nested-tests/tests/nested/other.rs", + ); assert_eq!( - vec![PathBuf::from( - "tests/nested-test-files/root-and-nested-tests/tests/nested/foo_bar.rs" - )], + vec![exp_baz, exp_foo_bar, exp_other], get_nested_integration_test_files(&target_dir, &target_dir), ) } } + + mod get_targets_tests { + use super::*; + + fn create_stub_target( + src_path: &str, + test_files: Vec, + kind: &str, + edition: &str, + ) -> Target { + let path = PathBuf::from(src_path); + let canonicalized = fs::canonicalize(&path).unwrap_or(path); + Target { + path: canonicalized, + kind: String::from(kind), + edition: String::from(edition), + nested_int_test_files: test_files, + } + } + + mod nested_test_files { + use super::*; + + #[test] + fn does_not_include_nested_test_files_when_disabled() { + let edition = "2018"; + let project_dir_base = "tests/nested-test-files/root-and-nested-tests"; + let manifest_path = PathBuf::from(format!("{}/Cargo.toml", project_dir_base)); + let mut exp_targets: BTreeSet = BTreeSet::new(); + exp_targets.insert(create_stub_target( + &format!("{}/src/lib.rs", project_dir_base), + vec![], + "lib", + edition, + )); + exp_targets.insert(create_stub_target( + &format!("{}/tests/bar.rs", project_dir_base), + vec![], + "test", + edition, + )); + exp_targets.insert(create_stub_target( + &format!("{}/tests/foo.rs", project_dir_base), + vec![], + "test", + edition, + )); + let strategy = CargoFmtStrategy::Root; + let act_targets = get_targets(&strategy, Some(&manifest_path), false); + assert_eq!(act_targets.unwrap(), exp_targets); + } + + #[test] + fn does_include_nested_test_files_when_enabled() { + let edition = "2018"; + let project_dir_base = "tests/nested-test-files/root-and-nested-tests"; + let manifest_path = PathBuf::from(format!("{}/Cargo.toml", project_dir_base)); + let exp_baz = PathBuf::from(format!( + "{}/tests/nested/deeply-nested/baz.rs", + project_dir_base + )); + let exp_foo_bar = + PathBuf::from(format!("{}/tests/nested/foo_bar.rs", project_dir_base)); + let exp_other = + PathBuf::from(format!("{}/tests/nested/other.rs", project_dir_base)); + let mut exp_targets: BTreeSet = BTreeSet::new(); + exp_targets.insert(create_stub_target( + &format!("{}/src/lib.rs", project_dir_base), + vec![], + "lib", + edition, + )); + exp_targets.insert(create_stub_target( + &format!("{}/tests/bar.rs", project_dir_base), + vec![exp_baz, exp_foo_bar, exp_other], + "test", + edition, + )); + exp_targets.insert(create_stub_target( + &format!("{}/tests/foo.rs", project_dir_base), + vec![], + "test", + edition, + )); + let strategy = CargoFmtStrategy::Root; + let act_targets = get_targets(&strategy, Some(&manifest_path), true); + assert_eq!(act_targets.unwrap(), exp_targets); + } + + #[test] + fn returns_correct_targets_with_empty_tests_dir() { + let edition = "2015"; + let project_dir_base = "tests/nested-test-files/empty-tests-dir"; + let manifest_path = PathBuf::from(format!("{}/Cargo.toml", project_dir_base)); + let mut exp_targets: BTreeSet = BTreeSet::new(); + exp_targets.insert(create_stub_target( + &format!("{}/src/lib.rs", project_dir_base), + vec![], + "lib", + edition, + )); + let strategy = CargoFmtStrategy::Root; + let act_targets = get_targets(&strategy, Some(&manifest_path), true); + assert_eq!(act_targets.unwrap(), exp_targets); + } + + #[test] + fn returns_correct_targets_with_no_tests_dir() { + let edition = "2015"; + let project_dir_base = "tests/nested-test-files/no-tests-dir"; + let manifest_path = PathBuf::from(format!("{}/Cargo.toml", project_dir_base)); + let mut exp_targets: BTreeSet = BTreeSet::new(); + exp_targets.insert(create_stub_target( + &format!("{}/src/lib.rs", project_dir_base), + vec![], + "lib", + edition, + )); + let strategy = CargoFmtStrategy::Root; + let act_targets = get_targets(&strategy, Some(&manifest_path), true); + assert_eq!(act_targets.unwrap(), exp_targets); + } + + #[test] + fn returns_correct_targets_with_only_root_level_tests() { + let edition = "2015"; + let project_dir_base = "tests/nested-test-files/only-root-level-tests-dir"; + let manifest_path = PathBuf::from(format!("{}/Cargo.toml", project_dir_base)); + let mut exp_targets: BTreeSet = BTreeSet::new(); + exp_targets.insert(create_stub_target( + &format!("{}/src/lib.rs", project_dir_base), + vec![], + "lib", + edition, + )); + exp_targets.insert(create_stub_target( + &format!("{}/tests/bar.rs", project_dir_base), + vec![], + "test", + edition, + )); + exp_targets.insert(create_stub_target( + &format!("{}/tests/foo.rs", project_dir_base), + vec![], + "test", + edition, + )); + let strategy = CargoFmtStrategy::Root; + let act_targets = get_targets(&strategy, Some(&manifest_path), true); + assert_eq!(act_targets.unwrap(), exp_targets); + } + } + } } diff --git a/tests/nested-test-files/only-root-level-tests/Cargo.toml b/tests/nested-test-files/only-root-level-tests-dir/Cargo.toml similarity index 100% rename from tests/nested-test-files/only-root-level-tests/Cargo.toml rename to tests/nested-test-files/only-root-level-tests-dir/Cargo.toml diff --git a/tests/nested-test-files/only-root-level-tests/src/lib.rs b/tests/nested-test-files/only-root-level-tests-dir/src/lib.rs similarity index 100% rename from tests/nested-test-files/only-root-level-tests/src/lib.rs rename to tests/nested-test-files/only-root-level-tests-dir/src/lib.rs diff --git a/tests/nested-test-files/only-root-level-tests/tests/bar.rs b/tests/nested-test-files/only-root-level-tests-dir/tests/bar.rs similarity index 100% rename from tests/nested-test-files/only-root-level-tests/tests/bar.rs rename to tests/nested-test-files/only-root-level-tests-dir/tests/bar.rs diff --git a/tests/nested-test-files/only-root-level-tests/tests/foo.rs b/tests/nested-test-files/only-root-level-tests-dir/tests/foo.rs similarity index 100% rename from tests/nested-test-files/only-root-level-tests/tests/foo.rs rename to tests/nested-test-files/only-root-level-tests-dir/tests/foo.rs diff --git a/tests/nested-test-files/root-and-nested-tests/Cargo.toml b/tests/nested-test-files/root-and-nested-tests/Cargo.toml index 27c0eeb1ba4..ae58bf460d8 100644 --- a/tests/nested-test-files/root-and-nested-tests/Cargo.toml +++ b/tests/nested-test-files/root-and-nested-tests/Cargo.toml @@ -1,6 +1,7 @@ [package] -name = "empty-tests-dir" +name = "root-and-nested-tests" version = "0.1.0" authors = ["rustfmt devs "] +edition = "2018" [dependencies] diff --git a/tests/nested-test-files/root-and-nested-tests/tests/nested/deeply-nested/baz.rs b/tests/nested-test-files/root-and-nested-tests/tests/nested/deeply-nested/baz.rs new file mode 100644 index 00000000000..4f3cec5eb12 --- /dev/null +++ b/tests/nested-test-files/root-and-nested-tests/tests/nested/deeply-nested/baz.rs @@ -0,0 +1,4 @@ +#[test] +fn test_truth() { + assert_eq!!(true, true); +} diff --git a/tests/nested-test-files/root-and-nested-tests/tests/nested/other.rs b/tests/nested-test-files/root-and-nested-tests/tests/nested/other.rs new file mode 100644 index 00000000000..e7a74c1fed0 --- /dev/null +++ b/tests/nested-test-files/root-and-nested-tests/tests/nested/other.rs @@ -0,0 +1,4 @@ +#[test] +fn test_add() { + assert_eq!(9, 4 + 5); +} From 871c6bbdb89dc162a1f1ce433c713da9f0553d1d Mon Sep 17 00:00:00 2001 From: Caleb Cartwright Date: Mon, 21 Oct 2019 11:31:58 -0500 Subject: [PATCH 5/7] doc: fix doc comment Co-Authored-By: Seiichi Uchida --- src/cargo-fmt/main.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cargo-fmt/main.rs b/src/cargo-fmt/main.rs index b317c84d2a4..71ea290045d 100644 --- a/src/cargo-fmt/main.rs +++ b/src/cargo-fmt/main.rs @@ -269,7 +269,7 @@ pub struct Target { kind: String, /// Rust edition for this target. edition: String, - // Rust files residing within subdirectories of the tests directory. + /// Rust files residing within subdirectories of the tests directory. nested_int_test_files: Vec, } From 9805d3cbe90ccc383f7100687cce5f157d691daa Mon Sep 17 00:00:00 2001 From: Caleb Cartwright Date: Tue, 22 Oct 2019 21:43:58 -0500 Subject: [PATCH 6/7] refactor: remove expects in nested test file detection --- src/cargo-fmt/main.rs | 51 +++++++++++++++++++++++-------------------- 1 file changed, 27 insertions(+), 24 deletions(-) diff --git a/src/cargo-fmt/main.rs b/src/cargo-fmt/main.rs index 71ea290045d..5b283d30d6f 100644 --- a/src/cargo-fmt/main.rs +++ b/src/cargo-fmt/main.rs @@ -280,10 +280,7 @@ impl Target { ) -> Self { let path = PathBuf::from(&target.src_path); let canonicalized = fs::canonicalize(&path).unwrap_or(path); - let test_files = match nested_int_test_files { - Some(files) => files, - None => vec![], - }; + let test_files = nested_int_test_files.unwrap_or(vec![]); Target { path: canonicalized, @@ -534,7 +531,7 @@ fn add_targets( Some(package_dir) => { let target_dir = package_dir.join("tests"); test_files_added = true; - Some(get_nested_integration_test_files(&target_dir, &target_dir)) + get_nested_integration_test_files(&target_dir, &target_dir) } } } else { @@ -547,27 +544,33 @@ fn add_targets( // Returns a `Vec` containing `PathBuf`s of nested .rs files within subdirectories // of the `tests` directory for a given package. // https://github.com/rust-lang/rustfmt/issues/1820 -fn get_nested_integration_test_files(path: &Path, root_dir: &Path) -> Vec { +fn get_nested_integration_test_files(path: &Path, root_dir: &Path) -> Option> { let mut files = vec![]; if path.is_dir() { - for entry in fs::read_dir(path).expect(&format!( - "couldn't read directory {}", - path.to_str().unwrap() - )) { - let entry = entry.expect("couldn't get nested `DirEntry` in tests"); - let parent = path; - let entry_path = entry.path(); - if entry_path.is_dir() { - files.append(&mut get_nested_integration_test_files( - &entry_path, - &root_dir, - )); - } else if entry_path.extension().map_or(false, |f| f == "rs") && parent != root_dir { - files.push(entry_path); + if let Ok(dir) = fs::read_dir(path) { + for dir_entry in dir { + if let Ok(entry) = dir_entry { + let parent = path; + let entry_path = entry.path(); + if entry_path.is_dir() { + files.append( + &mut get_nested_integration_test_files(&entry_path, &root_dir) + .unwrap_or(vec![]), + ); + } else if entry_path.extension().map_or(false, |f| f == "rs") + && parent != root_dir + { + files.push(entry_path); + } + } else { + return None; + } } + } else { + return None; } } - files + Some(files) } fn run_rustfmt( @@ -883,7 +886,7 @@ mod cargo_fmt_tests { fn returns_no_files_if_root_not_dir() { let target_dir = PathBuf::from("tests/nested-test-files/no-test-dir/Cargo.toml"); assert_eq!( - Vec::new() as Vec, + Some(Vec::new() as Vec), get_nested_integration_test_files(&target_dir, &target_dir), ) } @@ -892,7 +895,7 @@ mod cargo_fmt_tests { fn returns_no_files_if_tests_has_no_nested_files() { let target_dir = Path::new("tests/nested-test-files/only-root-level-tests/tests"); assert_eq!( - Vec::new() as Vec, + Some(Vec::new() as Vec), get_nested_integration_test_files(&target_dir, &target_dir), ) } @@ -910,7 +913,7 @@ mod cargo_fmt_tests { "tests/nested-test-files/root-and-nested-tests/tests/nested/other.rs", ); assert_eq!( - vec![exp_baz, exp_foo_bar, exp_other], + Some(vec![exp_baz, exp_foo_bar, exp_other]), get_nested_integration_test_files(&target_dir, &target_dir), ) } From 0e2e012783efea63f567cfc9a5f5589ef167d245 Mon Sep 17 00:00:00 2001 From: Caleb Cartwright Date: Sun, 27 Oct 2019 21:00:50 -0500 Subject: [PATCH 7/7] refactor: add early exit to nested test file discovery --- src/cargo-fmt/main.rs | 83 +++++++++++++++++++++++-------------------- 1 file changed, 44 insertions(+), 39 deletions(-) diff --git a/src/cargo-fmt/main.rs b/src/cargo-fmt/main.rs index 5b283d30d6f..e9e92b73f0c 100644 --- a/src/cargo-fmt/main.rs +++ b/src/cargo-fmt/main.rs @@ -421,7 +421,7 @@ fn get_targets_root_only( &package_targets, &mut targets, include_nested_test_files, - ); + )?; Ok(()) } @@ -441,7 +441,7 @@ fn get_targets_recursive( &package.targets, &mut targets, include_nested_test_files, - ); + )?; // Look for local dependencies. for dependency in package.dependencies { @@ -497,7 +497,7 @@ fn get_targets_with_hitlist( &package.targets, &mut targets, include_nested_test_files, - ); + )?; } } @@ -517,57 +517,62 @@ fn add_targets( target_paths: &[cargo_metadata::Target], targets: &mut BTreeSet, include_nested_test_files: bool, -) { +) -> Result<(), io::Error> { let mut test_files_added = false; for target in target_paths { // Packages often have more than one `test` target, // so only add the nested files for the first one. - let test_files = if include_nested_test_files + let check_for_nested_test_files = include_nested_test_files && !test_files_added - && target.kind.iter().any(|t| t == "test") - { - match manifest_path.parent() { - None => None, - Some(package_dir) => { - let target_dir = package_dir.join("tests"); - test_files_added = true; - get_nested_integration_test_files(&target_dir, &target_dir) - } + && target.kind.iter().any(|t| t == "test"); + + if !check_for_nested_test_files { + targets.insert(Target::from_target(&target, None)); + continue; + } + + if let Some(package_dir) = manifest_path.parent() { + let target_dir = package_dir.join("tests"); + test_files_added = true; + let test_files = get_nested_integration_test_files(&target_dir, &target_dir); + if test_files.is_none() { + return Err(io::Error::new( + io::ErrorKind::Other, + "Error encountered while searching for nested integration test files", + )); } + targets.insert(Target::from_target(&target, test_files)); } else { - None - }; - targets.insert(Target::from_target(target, test_files)); + return Err(io::Error::new( + io::ErrorKind::InvalidData, + "Unable to determine root `tests` directory for / + nested integration test file discovery", + )); + } } + + Ok(()) } // Returns a `Vec` containing `PathBuf`s of nested .rs files within subdirectories // of the `tests` directory for a given package. // https://github.com/rust-lang/rustfmt/issues/1820 fn get_nested_integration_test_files(path: &Path, root_dir: &Path) -> Option> { + if !path.is_dir() { + return Some(vec![]); + } let mut files = vec![]; - if path.is_dir() { - if let Ok(dir) = fs::read_dir(path) { - for dir_entry in dir { - if let Ok(entry) = dir_entry { - let parent = path; - let entry_path = entry.path(); - if entry_path.is_dir() { - files.append( - &mut get_nested_integration_test_files(&entry_path, &root_dir) - .unwrap_or(vec![]), - ); - } else if entry_path.extension().map_or(false, |f| f == "rs") - && parent != root_dir - { - files.push(entry_path); - } - } else { - return None; - } - } - } else { - return None; + let dir = fs::read_dir(path).ok()?; + + for dir_entry in dir { + let entry_path = dir_entry.ok()?.path(); + if entry_path.is_dir() { + files.append(&mut get_nested_integration_test_files( + &entry_path, + &root_dir, + )?); + } else if entry_path.extension().map_or(false, |f| f == "rs") && path != root_dir { + files.push(entry_path); } } Some(files)