From c4c5e19dffe36b86bd6c76b186d40b8fcc1de53f Mon Sep 17 00:00:00 2001 From: Erik Zivkovic Date: Sat, 17 Dec 2022 17:37:17 +0100 Subject: [PATCH 1/8] Make the recursive option of cargo-sweep recurse past Cargo directories When passing the -r (recursive) option to cargo-sweep, it is supposed to be recursive over the subdirectories of the root path it is given (whether implicit or explicit). cargo-sweep erronously assumed that a Cargo project could not contain unrelated subprojects. This patch lets cargo-sweep keep recursing past cargo roots. The drawback of this patch is that cargo-sweep will need to recurse more, and as such becomes a bit slower to run. Fixes https://github.com/holmgr/cargo-sweep/issues/66 --- src/main.rs | 3 +- tests/integration.rs | 190 +++++++++++++----- tests/nested-root-workspace/Cargo.toml | 7 + .../bin-crate/Cargo.toml | 8 + .../bin-crate/src/main.rs | 17 ++ .../crates/crate-one/Cargo.toml | 8 + .../crates/crate-one/src/lib.rs | 13 ++ .../crates/crate-two/Cargo.toml | 8 + .../crates/crate-two/src/lib.rs | 13 ++ 9 files changed, 217 insertions(+), 50 deletions(-) create mode 100644 tests/nested-root-workspace/Cargo.toml create mode 100644 tests/nested-root-workspace/bin-crate/Cargo.toml create mode 100644 tests/nested-root-workspace/bin-crate/src/main.rs create mode 100644 tests/nested-root-workspace/crates/crate-one/Cargo.toml create mode 100644 tests/nested-root-workspace/crates/crate-one/src/lib.rs create mode 100644 tests/nested-root-workspace/crates/crate-two/Cargo.toml create mode 100644 tests/nested-root-workspace/crates/crate-two/src/lib.rs diff --git a/src/main.rs b/src/main.rs index 68b8312..0e5f7a9 100644 --- a/src/main.rs +++ b/src/main.rs @@ -109,7 +109,8 @@ fn find_cargo_projects(root: &Path, include_hidden: bool) -> Vec { } if let Some(target_directory) = is_cargo_root(entry.path()) { target_paths.insert(target_directory); - iter.skip_current_dir(); // no reason to look at the src and such + // Previously cargo-sweep skipped subdirectories here, but it is valid for + // subdirectories to contain cargo roots. } } } diff --git a/tests/integration.rs b/tests/integration.rs index 6313006..407fb6e 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -7,7 +7,7 @@ use std::{ use anyhow::{Context, Result}; use assert_cmd::{assert::Assert, cargo::cargo_bin, Command}; -use fs_extra::dir::get_size; +use fs_extra::dir::{get_size, CopyOptions}; use predicates::{prelude::PredicateBooleanExt, str::contains}; #[allow(unused_imports)] use pretty_assertions::{assert_eq, assert_ne}; @@ -16,16 +16,19 @@ use tempfile::{tempdir, TempDir}; use unindent::unindent; struct AnyhowWithContext(anyhow::Error); + impl Debug for AnyhowWithContext { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { write!(f, "{:#?}", self.0) } } + impl> From for AnyhowWithContext { fn from(err: T) -> Self { Self(err.into()) } } + type TestResult = Result<(), AnyhowWithContext>; fn test_dir() -> PathBuf { @@ -52,10 +55,7 @@ fn sweep(args: &[&str]) -> Command { } /// Sets the target folder and runs the given command -fn run<'a>(mut cmd: impl BorrowMut, target: impl Into>) -> Assert { - if let Some(target) = target.into() { - cmd.borrow_mut().env("CARGO_TARGET_DIR", target); - } +fn run(mut cmd: impl BorrowMut) -> Assert { let assert = cmd.borrow_mut().assert().success(); let out = assert.get_output(); let str = |s| std::str::from_utf8(s).unwrap(); @@ -64,17 +64,29 @@ fn run<'a>(mut cmd: impl BorrowMut, target: impl Into> assert } -/// Returns the size of the build directory. +/// Runs a cargo build of the named project (which is expected to exist as a direct +/// child of the `tests` directory. +/// Returns the size of the build directory, as well as the [TempDir] of the unique +/// temporary build target directory. fn build(project: &str) -> Result<(u64, TempDir)> { let target = tempdir()?; let old_size = get_size(target.path())?; - run(cargo(project).arg("build"), target.path()); + run(cargo(project) + .arg("build") + .env("CARGO_TARGET_DIR", target.path())); let new_size = get_size(target.path())?; assert!(new_size > old_size, "cargo didn't build anything"); Ok((new_size, target)) } -fn clean_and_parse(target: &TempDir, args: &[&str]) -> Result { +/// Run the sweep command, cleaning up target files, and then parse the results and +/// return the number of cleaned files. This function takes a `cmd_modifier` which can +/// be used to modify the `sweep` [Command], e.g. adding or changing environment variables. +fn clean_and_parse( + args: &[&str], + // Use this to modify the Command generated by the sweep function. + cmd_modifier: impl Fn(&mut Command) -> &mut Command, +) -> Result { let dry_run = args.iter().any(|&f| f == "-d" || f == "--dry-run"); let (remove_msg, clean_msg) = if dry_run { @@ -83,40 +95,59 @@ fn clean_and_parse(target: &TempDir, args: &[&str]) -> Result { ("Successfully removed", "Cleaned ") }; let assertion = - run(sweep(args), target.path()).stdout(contains(remove_msg).and(contains(clean_msg))); + run(cmd_modifier(&mut sweep(args))).stdout(contains(remove_msg).and(contains(clean_msg))); let output = assertion.get_output(); assert!(output.stderr.is_empty()); - // Extract the size from the last line of stdout, example: - // - stdout: "[INFO] Would clean: 9.43 KiB from "/home/user/project/target" - // - extracted amount: "9.43 KiB " - let amount = std::str::from_utf8(&output.stdout)? + // Collect all the lines that contain the "clean message". + let amount_lines: Vec = std::str::from_utf8(&output.stdout)? .lines() - .last() - .unwrap() - .split(clean_msg) - .nth(1) - .unwrap() - .split_inclusive(' ') - .take(2) - .collect::(); - - let cleaned = amount - .parse::() - .context(format!("failed to parse amount {amount}"))? - .to_bytes(); + .filter(|line| line.contains(clean_msg)) + .map(|line| { + line.split(clean_msg) + .nth(1) + .unwrap() + .split_inclusive(' ') + .take(2) + .collect::() + }) + .collect(); + + // Turn all those lines into a collected sum of bytes called `clean`. + let cleaned: u64 = amount_lines + .iter() + .map(|amount| { + amount + .parse::() + .context(format!("failed to parse amount {amount}")) + .unwrap() + .to_bytes() + }) + .sum(); Ok(cleaned) } fn count_cleaned(target: &TempDir, args: &[&str], old_size: u64) -> Result { - let cleaned = clean_and_parse(target, args)?; + let cleaned = clean_and_parse(args, |cmd| cmd.env("CARGO_TARGET_DIR", target.path()))?; + assert_sweeped_size(target.path(), cleaned, old_size)?; + Ok(cleaned) +} - // Make sure this is accurate. - let new_size = get_size(target.path())?; - // Due to rounding and truncation we might have an inexact result. Make sure these are within 1% of each other, - // but don't require an exact match. +/// Assert that the size of the target directory has the expected size +/// (within a small margin of error). +fn assert_sweeped_size( + path: impl AsRef, + // The number of bytes that were sweeped ("cleaned"). + cleaned: u64, + // The size of the target directory (in bytes) before the sweep. + old_size: u64, +) -> Result<()> { + // Make sure that the expected target directory size is accurate. + let new_size = get_size(path)?; + // Due to rounding and truncation we might have an inexact result. Make sure + // these are within 1% of each other, but don't require an exact match. let calculated_size = old_size - cleaned; let diff = new_size.abs_diff(calculated_size); let one_percent = old_size as f64 * 0.01; @@ -124,14 +155,13 @@ fn count_cleaned(target: &TempDir, args: &[&str], old_size: u64) -> Result diff <= one_percent as u64, "new_size={new_size}, old_size={old_size}, cleaned={cleaned}, diff={diff}, 1%={one_percent}", ); - - Ok(cleaned) + Ok(()) } fn count_cleaned_dry_run(target: &TempDir, args: &[&str], old_size: u64) -> Result { let mut args = args.to_vec(); args.push("--dry-run"); - let cleaned = clean_and_parse(target, &args)?; + let cleaned = clean_and_parse(&args, |cmd| cmd.env("CARGO_TARGET_DIR", target.path()))?; let new_size = get_size(target.path())?; assert_eq!(old_size, new_size); @@ -171,7 +201,7 @@ fn stamp_file() -> TestResult { let (size, target) = build("sample-project")?; // Create a stamp file for --file. - let assert = run(sweep(&["--stamp"]), target.path()); + let assert = run(sweep(&["--stamp"]).env("CARGO_TARGET_DIR", target.path())); println!( "{}", std::str::from_utf8(&assert.get_output().stdout).unwrap() @@ -186,7 +216,7 @@ fn stamp_file() -> TestResult { // For some reason, we delete the stamp file after `--file` :( // Recreate it. - run(sweep(&["--stamp"]), target.path()); + run(sweep(&["--stamp"]).env("CARGO_TARGET_DIR", target.path())); let actual_cleaned = count_cleaned(&target, args, size)?; assert_eq!(actual_cleaned, expected_cleaned); @@ -198,10 +228,9 @@ fn stamp_file() -> TestResult { fn empty_project_output() -> TestResult { let (_size, target) = build("sample-project")?; - let assert = run( - sweep(&["--maxsize", "0"]).current_dir(test_dir().join("sample-project")), - target.path(), - ); + let assert = run(sweep(&["--maxsize", "0"]) + .current_dir(test_dir().join("sample-project")) + .env("CARGO_TARGET_DIR", target.path())); let output = std::str::from_utf8(&assert.get_output().stdout).unwrap(); @@ -237,10 +266,9 @@ fn hidden() -> TestResult { // So we can't let cargo-sweep discover any other projects, or it will think they share the same directory as this hidden project. let (size, target) = build("fresh-prefix/.hidden/hidden-project")?; let run = |args| { - run( - sweep(args).current_dir(test_dir().join("fresh-prefix")), - target.path(), - ) + run(sweep(args) + .current_dir(test_dir().join("fresh-prefix")) + .env("CARGO_TARGET_DIR", target.path())) }; run(&["--maxsize", "0", "-r"]); @@ -272,10 +300,9 @@ fn error_output() -> TestResult { } let (_, tempdir) = build("sample-project")?; - let assert = run( - sweep(&["--installed"]).env("PATH", test_dir()), - tempdir.path(), - ); + let assert = run(sweep(&["--installed"]) + .env("PATH", test_dir()) + .env("CARGO_TARGET_DIR", tempdir.path())); assert.stdout(contains("oh no an error")); Ok(()) @@ -299,7 +326,9 @@ fn path() -> TestResult { cmd.arg("sweep").arg("--installed").current_dir(temp_dir()); // Pass `path` as an argument, instead of `current_dir` like it normally is. - let assert = run(cmd.arg(project_dir("sample-project")), target.path()); + let assert = run(cmd + .arg(project_dir("sample-project")) + .env("CARGO_TARGET_DIR", target.path())); assert.stdout(contains("Cleaned")); Ok(()) @@ -310,3 +339,66 @@ fn usage() -> TestResult { trycmd::TestCases::new().case("tests/*.trycmd"); Ok(()) } + +/// This test is a bit more verbose than the other tests because it uses a slightly +/// different mechanism for setting up and sweeping / cleaning. +/// +/// Step-by-step: +/// +/// * Create a new temporary directory. Other tests only output the built files into +/// the target directory, but this test copies the whole template project into the +/// target directory. +/// * Build the two different (nested) projects using cargo build, but without incremental +/// compilation, because cargo-sweep doesn't remove files built by incremental compilation, +/// which affects this test since this test doesn't use a separate target folder for just +/// the target files. +/// * Do a dry run sweep, asserting that nothing was actually cleaned. +/// * Do a proper sweep, asserting that the target directory size is back down to its +/// expected size. +#[test] +fn recursive_multiple_root_workspaces() -> TestResult { + let target = tempdir()?; + + let nested_workspace_dir = test_dir().join("nested-root-workspace"); + let options = CopyOptions::default(); + + // Copy the whole nested-root-workspace folder (and its content) into the new temp dir, + // and then `cargo build` and run the sweep tests inside that directory. + fs_extra::copy_items(&[&nested_workspace_dir], target.path(), &options)?; + + let old_size = get_size(target.path())?; + + // Build bin-crate + run(Command::new(env!("CARGO")) + .env("CARGO_INCREMENTAL", "0") + .arg("build") + .current_dir(target.path().join("nested-root-workspace/bin-crate"))); + + let intermediate_build_size = get_size(target.path())?; + assert!(intermediate_build_size > old_size); + + // Build workspace crates + run(Command::new(env!("CARGO")) + .env("CARGO_INCREMENTAL", "0") + .arg("build") + .current_dir(target.path().join("nested-root-workspace"))); + + let final_build_size = get_size(target.path())?; + assert!(final_build_size > intermediate_build_size); + + // Run a dry-run of cargo-sweep ("clean") in the target directory (recursive) + let args = &["-r", "--time", "0", "--dry-run"]; + let expected_cleaned = clean_and_parse(args, |cmd| cmd.current_dir(target.path()))?; + assert!(expected_cleaned > 0); + let size_after_dry_run_clean = get_size(target.path())?; + // Make sure that nothing was actually cleaned + assert_eq!(final_build_size, size_after_dry_run_clean); + + // Run a proper cargo-sweep ("clean") in the target directory (recursive) + let args = &["-r", "--time", "0"]; + let actual_cleaned = clean_and_parse(args, |cmd| cmd.current_dir(target.path()))?; + assert_sweeped_size(target.path(), actual_cleaned, final_build_size)?; + assert_eq!(actual_cleaned, expected_cleaned); + + Ok(()) +} diff --git a/tests/nested-root-workspace/Cargo.toml b/tests/nested-root-workspace/Cargo.toml new file mode 100644 index 0000000..b4d859d --- /dev/null +++ b/tests/nested-root-workspace/Cargo.toml @@ -0,0 +1,7 @@ +[workspace] +members = [ + "crates/*", +] +exclude = [ + "bin-crate", +] diff --git a/tests/nested-root-workspace/bin-crate/Cargo.toml b/tests/nested-root-workspace/bin-crate/Cargo.toml new file mode 100644 index 0000000..c5f9791 --- /dev/null +++ b/tests/nested-root-workspace/bin-crate/Cargo.toml @@ -0,0 +1,8 @@ +[package] +name = "bin-crate" +version = "0.1.0" +edition = "2021" + +# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html + +[dependencies] diff --git a/tests/nested-root-workspace/bin-crate/src/main.rs b/tests/nested-root-workspace/bin-crate/src/main.rs new file mode 100644 index 0000000..93efce5 --- /dev/null +++ b/tests/nested-root-workspace/bin-crate/src/main.rs @@ -0,0 +1,17 @@ +fn main() { + println!("{}", bin_string()); +} + +fn bin_string() -> String { + "This is bin-crate".to_string() +} + +#[cfg(test)] +mod test { + use super::bin_string; + + #[test] + fn test() { + assert_eq!("This is bin-crate".to_string(), bin_string()); + } +} diff --git a/tests/nested-root-workspace/crates/crate-one/Cargo.toml b/tests/nested-root-workspace/crates/crate-one/Cargo.toml new file mode 100644 index 0000000..df8e0fc --- /dev/null +++ b/tests/nested-root-workspace/crates/crate-one/Cargo.toml @@ -0,0 +1,8 @@ +[package] +name = "crate-one" +version = "0.1.0" +edition = "2021" + +# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html + +[dependencies] diff --git a/tests/nested-root-workspace/crates/crate-one/src/lib.rs b/tests/nested-root-workspace/crates/crate-one/src/lib.rs new file mode 100644 index 0000000..8f3bba6 --- /dev/null +++ b/tests/nested-root-workspace/crates/crate-one/src/lib.rs @@ -0,0 +1,13 @@ +pub fn crate_one_string() -> String { + "This is crate-one".to_string() +} + +#[cfg(test)] +mod test { + use super::crate_one_string; + + #[test] + fn test() { + assert_eq!("This is crate-one".to_string(), crate_one_string()); + } +} diff --git a/tests/nested-root-workspace/crates/crate-two/Cargo.toml b/tests/nested-root-workspace/crates/crate-two/Cargo.toml new file mode 100644 index 0000000..df1daae --- /dev/null +++ b/tests/nested-root-workspace/crates/crate-two/Cargo.toml @@ -0,0 +1,8 @@ +[package] +name = "crate-two" +version = "0.1.0" +edition = "2021" + +# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html + +[dependencies] diff --git a/tests/nested-root-workspace/crates/crate-two/src/lib.rs b/tests/nested-root-workspace/crates/crate-two/src/lib.rs new file mode 100644 index 0000000..f9aaf7e --- /dev/null +++ b/tests/nested-root-workspace/crates/crate-two/src/lib.rs @@ -0,0 +1,13 @@ +pub fn crate_two_string() -> String { + "This is crate-two".to_string() +} + +#[cfg(test)] +mod test { + use super::crate_two_string; + + #[test] + fn test() { + assert_eq!("This is crate-two".to_string(), crate_two_string()); + } +} From e920ea518301e8358d0ffb48429613bc314f97cf Mon Sep 17 00:00:00 2001 From: Erik Zivkovic Date: Sat, 28 Jan 2023 23:47:43 +0100 Subject: [PATCH 2/8] Fix the empty_project_output test for macos The empty_project_output test doesn't have the same traversing/logging order on macos as it does on whatever os/machine it was written for. This commit tries to improve the situation somewhat by making the test runnable on macos as well, while sacrificing readability and test accuracy. An alternative fix would be to not use regex for the test at all. --- tests/integration.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/integration.rs b/tests/integration.rs index 407fb6e..44f02f4 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -234,6 +234,8 @@ fn empty_project_output() -> TestResult { let output = std::str::from_utf8(&assert.get_output().stdout).unwrap(); + // Please note: The output from the sweep command is platform dependent. The regular + // expression tries to take that into account by letting the file output order vary. let pattern = unindent( r#"\[DEBUG\] cleaning: ".+" with remove_older_until_fits \[DEBUG\] size_to_remove: .+ @@ -248,7 +250,7 @@ fn empty_project_output() -> TestResult { \[DEBUG\] Successfully removed: ".+debug.+deps.+sample_project.+" \[DEBUG\] Successfully removed: ".+debug.+deps.+sample_project.+" \[DEBUG\] Successfully removed: ".+debug.+deps.+sample_project.+" - \[DEBUG\] Successfully removed: ".+.fingerprint.+sample-project-.+" + \[DEBUG\] Successfully removed: ".+debug.+fingerprint.+sample-project.+" \[INFO\] Cleaned .+ from ".+""#, ); From e94aaecdcb305ee22684dbf71a0fd539320ad25e Mon Sep 17 00:00:00 2001 From: Erik Zivkovic Date: Sat, 28 Jan 2023 23:53:18 +0100 Subject: [PATCH 3/8] Add clarifying comments to the recursive_multiple_root_workspaces test --- tests/integration.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/integration.rs b/tests/integration.rs index 44f02f4..fa3e587 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -372,6 +372,7 @@ fn recursive_multiple_root_workspaces() -> TestResult { // Build bin-crate run(Command::new(env!("CARGO")) + // Don't output incremental build artifacts .env("CARGO_INCREMENTAL", "0") .arg("build") .current_dir(target.path().join("nested-root-workspace/bin-crate"))); @@ -381,6 +382,7 @@ fn recursive_multiple_root_workspaces() -> TestResult { // Build workspace crates run(Command::new(env!("CARGO")) + // Don't output incremental build artifacts .env("CARGO_INCREMENTAL", "0") .arg("build") .current_dir(target.path().join("nested-root-workspace"))); From e29c83ff4e91216b84740e3b5bb3c822b73c9f51 Mon Sep 17 00:00:00 2001 From: Erik Zivkovic Date: Wed, 1 Feb 2023 08:41:14 +0100 Subject: [PATCH 4/8] Make the recursive_multiple_root_workspaces test ignore CARGO_TARGET_DIR There is an edge case where the `cargo test` invocation can be run with `CARGO_TARGET_DIR` set, which needs to be handled by the recursive_multiple_root_workspaces test since it assumes that `CARGO_TARGET_DIR` isn't set. --- tests/integration.rs | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/tests/integration.rs b/tests/integration.rs index fa3e587..d6212ec 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -357,6 +357,11 @@ fn usage() -> TestResult { /// * Do a dry run sweep, asserting that nothing was actually cleaned. /// * Do a proper sweep, asserting that the target directory size is back down to its /// expected size. +/// +/// Please note that there is some ceremony involved, namely correctly setting: +/// * `CARGO_TARGET_DIR` to `unset`, so as not to clash with users who use `CARGO_TARGET_DIR` +/// when they invoke `cargo test` for running these tests. +/// * `CARGO_INCREMENTAL` to `0`, because cargo-sweep doesn't yet clear incremental files. #[test] fn recursive_multiple_root_workspaces() -> TestResult { let target = tempdir()?; @@ -372,6 +377,9 @@ fn recursive_multiple_root_workspaces() -> TestResult { // Build bin-crate run(Command::new(env!("CARGO")) + // If someone has built & run these tests with CARGO_TARGET_DIR, + // we need to override that. + .env_remove("CARGO_TARGET_DIR") // Don't output incremental build artifacts .env("CARGO_INCREMENTAL", "0") .arg("build") @@ -382,6 +390,9 @@ fn recursive_multiple_root_workspaces() -> TestResult { // Build workspace crates run(Command::new(env!("CARGO")) + // If someone has built & run these tests with CARGO_TARGET_DIR, + // we need to override that. + .env_remove("CARGO_TARGET_DIR") // Don't output incremental build artifacts .env("CARGO_INCREMENTAL", "0") .arg("build") @@ -392,7 +403,12 @@ fn recursive_multiple_root_workspaces() -> TestResult { // Run a dry-run of cargo-sweep ("clean") in the target directory (recursive) let args = &["-r", "--time", "0", "--dry-run"]; - let expected_cleaned = clean_and_parse(args, |cmd| cmd.current_dir(target.path()))?; + let expected_cleaned = clean_and_parse(args, |cmd| { + // If someone has built & run these tests with CARGO_TARGET_DIR, + // we need to override that. + cmd.env_remove("CARGO_TARGET_DIR") + .current_dir(target.path()) + })?; assert!(expected_cleaned > 0); let size_after_dry_run_clean = get_size(target.path())?; // Make sure that nothing was actually cleaned @@ -400,7 +416,12 @@ fn recursive_multiple_root_workspaces() -> TestResult { // Run a proper cargo-sweep ("clean") in the target directory (recursive) let args = &["-r", "--time", "0"]; - let actual_cleaned = clean_and_parse(args, |cmd| cmd.current_dir(target.path()))?; + let actual_cleaned = clean_and_parse(args, |cmd| { + // If someone has built & run these tests with CARGO_TARGET_DIR, + // we need to override that. + cmd.env_remove("CARGO_TARGET_DIR") + .current_dir(target.path()) + })?; assert_sweeped_size(target.path(), actual_cleaned, final_build_size)?; assert_eq!(actual_cleaned, expected_cleaned); From a13db04ef3a2f0d0b78510ec693910eaf23ecfc2 Mon Sep 17 00:00:00 2001 From: Erik Zivkovic Date: Wed, 1 Feb 2023 23:13:52 +0100 Subject: [PATCH 5/8] Make cargo-sweep output parselable by human-size --- src/util.rs | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/src/util.rs b/src/util.rs index a87eee2..46924bb 100644 --- a/src/util.rs +++ b/src/util.rs @@ -1,6 +1,6 @@ /// Formats a number of bytes into the closest binary SI unit, i.e KiB, MiB etc. pub fn format_bytes(bytes: u64) -> String { - let prefixes = ["bytes", "KiB", "MiB", "GiB", "TiB"]; + let prefixes = ["B", "KiB", "MiB", "GiB", "TiB"]; let mut bytes = bytes as f64; for prefix in prefixes.iter() { if bytes < 1024. { @@ -18,7 +18,26 @@ mod tests { #[test] fn test_format_bytes() { assert_eq!(format_bytes(1024), "1.00 KiB"); - assert_eq!(format_bytes(1023), "1023.00 bytes"); + assert_eq!(format_bytes(1023), "1023.00 B"); assert_eq!(format_bytes(500 * 1024 * 1024), "500.00 MiB"); + + // Assert that the human-size crate can parse the output from cargo-sweep + assert_eq!("1.00 B".parse::().unwrap().to_bytes(), 1); + assert_eq!( + "1.00 KiB".parse::().unwrap().to_bytes(), + 1024 + ); + assert_eq!( + "1.00 MiB".parse::().unwrap().to_bytes(), + 1024 * 1024 + ); + assert_eq!( + "1.00 GiB".parse::().unwrap().to_bytes(), + 1024 * 1024 * 1024 + ); + assert_eq!( + "1.00 TiB".parse::().unwrap().to_bytes(), + 1024 * 1024 * 1024 * 1024 + ); } } From c88fa946c9c13b62d1e86d8c202da70cdc406a1c Mon Sep 17 00:00:00 2001 From: Erik Zivkovic Date: Thu, 2 Feb 2023 08:46:53 +0100 Subject: [PATCH 6/8] Improve the recursive_multiple_root_workspaces test This commit improves the recursive_multiple_root_workspaces by applying some review comments: * Deduplicating the cargo command by making the cargo function take an absolute path instead of a relative one. * Removing an unnecessary CARGO_INCREMENTAL=0 environment variable * Renaming the temporary workspace directory variable --- tests/integration.rs | 38 ++++++++++++++++---------------------- 1 file changed, 16 insertions(+), 22 deletions(-) diff --git a/tests/integration.rs b/tests/integration.rs index d6212ec..ff2eb7a 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -39,9 +39,9 @@ fn project_dir(project: &str) -> PathBuf { test_dir().join(project) } -fn cargo(project: &str) -> Command { +fn cargo(cmd_current_dir: impl AsRef) -> Command { let mut cmd = Command::new(env!("CARGO")); - cmd.current_dir(project_dir(project)); + cmd.current_dir(cmd_current_dir); cmd } @@ -71,7 +71,7 @@ fn run(mut cmd: impl BorrowMut) -> Assert { fn build(project: &str) -> Result<(u64, TempDir)> { let target = tempdir()?; let old_size = get_size(target.path())?; - run(cargo(project) + run(cargo(project_dir(project)) .arg("build") .env("CARGO_TARGET_DIR", target.path())); let new_size = get_size(target.path())?; @@ -364,41 +364,35 @@ fn usage() -> TestResult { /// * `CARGO_INCREMENTAL` to `0`, because cargo-sweep doesn't yet clear incremental files. #[test] fn recursive_multiple_root_workspaces() -> TestResult { - let target = tempdir()?; + let temp_workspace_dir = tempdir()?; let nested_workspace_dir = test_dir().join("nested-root-workspace"); let options = CopyOptions::default(); // Copy the whole nested-root-workspace folder (and its content) into the new temp dir, // and then `cargo build` and run the sweep tests inside that directory. - fs_extra::copy_items(&[&nested_workspace_dir], target.path(), &options)?; + fs_extra::copy_items(&[&nested_workspace_dir], temp_workspace_dir.path(), &options)?; - let old_size = get_size(target.path())?; + let old_size = get_size(temp_workspace_dir.path())?; // Build bin-crate - run(Command::new(env!("CARGO")) + run(cargo(temp_workspace_dir.path().join("nested-root-workspace/bin-crate")) // If someone has built & run these tests with CARGO_TARGET_DIR, // we need to override that. .env_remove("CARGO_TARGET_DIR") - // Don't output incremental build artifacts - .env("CARGO_INCREMENTAL", "0") - .arg("build") - .current_dir(target.path().join("nested-root-workspace/bin-crate"))); + .arg("build")); - let intermediate_build_size = get_size(target.path())?; + let intermediate_build_size = get_size(temp_workspace_dir.path())?; assert!(intermediate_build_size > old_size); // Build workspace crates - run(Command::new(env!("CARGO")) + run(cargo(temp_workspace_dir.path().join("nested-root-workspace")) // If someone has built & run these tests with CARGO_TARGET_DIR, // we need to override that. .env_remove("CARGO_TARGET_DIR") - // Don't output incremental build artifacts - .env("CARGO_INCREMENTAL", "0") - .arg("build") - .current_dir(target.path().join("nested-root-workspace"))); + .arg("build")); - let final_build_size = get_size(target.path())?; + let final_build_size = get_size(temp_workspace_dir.path())?; assert!(final_build_size > intermediate_build_size); // Run a dry-run of cargo-sweep ("clean") in the target directory (recursive) @@ -407,10 +401,10 @@ fn recursive_multiple_root_workspaces() -> TestResult { // If someone has built & run these tests with CARGO_TARGET_DIR, // we need to override that. cmd.env_remove("CARGO_TARGET_DIR") - .current_dir(target.path()) + .current_dir(temp_workspace_dir.path()) })?; assert!(expected_cleaned > 0); - let size_after_dry_run_clean = get_size(target.path())?; + let size_after_dry_run_clean = get_size(temp_workspace_dir.path())?; // Make sure that nothing was actually cleaned assert_eq!(final_build_size, size_after_dry_run_clean); @@ -420,9 +414,9 @@ fn recursive_multiple_root_workspaces() -> TestResult { // If someone has built & run these tests with CARGO_TARGET_DIR, // we need to override that. cmd.env_remove("CARGO_TARGET_DIR") - .current_dir(target.path()) + .current_dir(temp_workspace_dir.path()) })?; - assert_sweeped_size(target.path(), actual_cleaned, final_build_size)?; + assert_sweeped_size(temp_workspace_dir.path(), actual_cleaned, final_build_size)?; assert_eq!(actual_cleaned, expected_cleaned); Ok(()) From 442d75b119ec3a2ce21c2f924bbbc1a1b767f642 Mon Sep 17 00:00:00 2001 From: Erik Zivkovic Date: Sat, 4 Feb 2023 21:45:04 +0100 Subject: [PATCH 7/8] Assert that cargo-sweep was effective in recursive_multiple_root_workspaces This commit changes the recursive_multiple_root_workspaces test by adding assertions to make sure that the actions of cargo-sweep were actually effective in cleaning nested root workspaces. --- tests/integration.rs | 56 +++++++++++++++++++++++++++++++++++--------- 1 file changed, 45 insertions(+), 11 deletions(-) diff --git a/tests/integration.rs b/tests/integration.rs index ff2eb7a..897d0e3 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -371,30 +371,49 @@ fn recursive_multiple_root_workspaces() -> TestResult { // Copy the whole nested-root-workspace folder (and its content) into the new temp dir, // and then `cargo build` and run the sweep tests inside that directory. - fs_extra::copy_items(&[&nested_workspace_dir], temp_workspace_dir.path(), &options)?; + fs_extra::copy_items( + &[&nested_workspace_dir], + temp_workspace_dir.path(), + &options, + )?; let old_size = get_size(temp_workspace_dir.path())?; // Build bin-crate - run(cargo(temp_workspace_dir.path().join("nested-root-workspace/bin-crate")) - // If someone has built & run these tests with CARGO_TARGET_DIR, - // we need to override that. - .env_remove("CARGO_TARGET_DIR") - .arg("build")); + run(cargo( + temp_workspace_dir + .path() + .join("nested-root-workspace/bin-crate"), + ) + // If someone has built & run these tests with CARGO_TARGET_DIR, + // we need to override that. + .env_remove("CARGO_TARGET_DIR") + .arg("build")); let intermediate_build_size = get_size(temp_workspace_dir.path())?; assert!(intermediate_build_size > old_size); // Build workspace crates - run(cargo(temp_workspace_dir.path().join("nested-root-workspace")) - // If someone has built & run these tests with CARGO_TARGET_DIR, - // we need to override that. - .env_remove("CARGO_TARGET_DIR") - .arg("build")); + run( + cargo(temp_workspace_dir.path().join("nested-root-workspace")) + // If someone has built & run these tests with CARGO_TARGET_DIR, + // we need to override that. + .env_remove("CARGO_TARGET_DIR") + .arg("build"), + ); let final_build_size = get_size(temp_workspace_dir.path())?; assert!(final_build_size > intermediate_build_size); + // Measure the size of the nested root workspace and the bin crate before cargo-sweep is invoked. + let pre_clean_size_nested_root_workspace = + get_size(temp_workspace_dir.path().join("nested-root-workspace"))?; + let pre_clean_size_bin_create = get_size( + temp_workspace_dir + .path() + .join("nested-root-workspace/bin-crate"), + )?; + // Run a dry-run of cargo-sweep ("clean") in the target directory (recursive) let args = &["-r", "--time", "0", "--dry-run"]; let expected_cleaned = clean_and_parse(args, |cmd| { @@ -419,5 +438,20 @@ fn recursive_multiple_root_workspaces() -> TestResult { assert_sweeped_size(temp_workspace_dir.path(), actual_cleaned, final_build_size)?; assert_eq!(actual_cleaned, expected_cleaned); + // Measure the size of the nested root workspace and the bin crate after cargo-sweep is invoked, + // and assert that both of their sizes have been reduced. + let post_clean_size_nested_root_workspace = + get_size(temp_workspace_dir.path().join("nested-root-workspace"))?; + let post_clean_size_bin_crate = get_size( + temp_workspace_dir + .path() + .join("nested-root-workspace/bin-crate"), + )?; + assert!(post_clean_size_nested_root_workspace < pre_clean_size_nested_root_workspace, "The size of the nested root workspace create has not been reduced after running cargo-sweep."); + assert!( + post_clean_size_bin_crate < pre_clean_size_bin_create, + "The size of the bin create has not been reduced after running cargo-sweep." + ); + Ok(()) } From d5a72d64de31880c9c9420f693e5f65d138240d5 Mon Sep 17 00:00:00 2001 From: Erik Zivkovic Date: Sun, 5 Feb 2023 22:36:01 +0100 Subject: [PATCH 8/8] Clean up recursive_multiple_root_workspaces test documentation * Remove a few comments that are no longer valid. * Improve the understanding of the final assertions by adding a more detailed explanation. --- tests/integration.rs | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/tests/integration.rs b/tests/integration.rs index 897d0e3..794a419 100644 --- a/tests/integration.rs +++ b/tests/integration.rs @@ -65,7 +65,7 @@ fn run(mut cmd: impl BorrowMut) -> Assert { } /// Runs a cargo build of the named project (which is expected to exist as a direct -/// child of the `tests` directory. +/// child of the `tests` directory). /// Returns the size of the build directory, as well as the [TempDir] of the unique /// temporary build target directory. fn build(project: &str) -> Result<(u64, TempDir)> { @@ -350,18 +350,14 @@ fn usage() -> TestResult { /// * Create a new temporary directory. Other tests only output the built files into /// the target directory, but this test copies the whole template project into the /// target directory. -/// * Build the two different (nested) projects using cargo build, but without incremental -/// compilation, because cargo-sweep doesn't remove files built by incremental compilation, -/// which affects this test since this test doesn't use a separate target folder for just -/// the target files. +/// * Build the two different (nested) projects using cargo build. /// * Do a dry run sweep, asserting that nothing was actually cleaned. /// * Do a proper sweep, asserting that the target directory size is back down to its /// expected size. /// -/// Please note that there is some ceremony involved, namely correctly setting: -/// * `CARGO_TARGET_DIR` to `unset`, so as not to clash with users who use `CARGO_TARGET_DIR` +/// Please note that there is some ceremony involved, namely correctly setting +/// `CARGO_TARGET_DIR` to `unset`, so as not to clash with users who use `CARGO_TARGET_DIR` /// when they invoke `cargo test` for running these tests. -/// * `CARGO_INCREMENTAL` to `0`, because cargo-sweep doesn't yet clear incremental files. #[test] fn recursive_multiple_root_workspaces() -> TestResult { let temp_workspace_dir = tempdir()?; @@ -440,6 +436,8 @@ fn recursive_multiple_root_workspaces() -> TestResult { // Measure the size of the nested root workspace and the bin crate after cargo-sweep is invoked, // and assert that both of their sizes have been reduced. + // This works because by default cargo generates the `target/` directory in a sub-directory + // of the package root. let post_clean_size_nested_root_workspace = get_size(temp_workspace_dir.path().join("nested-root-workspace"))?; let post_clean_size_bin_crate = get_size(