Skip to content
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.

Sign up for GitHub

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

Make default output less noisy #81

Merged
merged 7 commits into from
Dec 26, 2022
Merged

Make default output less noisy #81

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b5cc645
make default output less noisy
marcospb19 Dec 25, 2022
0e63272
Show the path of the cleaned project
marcospb19 Dec 25, 2022
9120c29
solve Clippy warnings
marcospb19 Dec 25, 2022
f25ec8b
refactor test function `run`
marcospb19 Dec 26, 2022
d78a8eb
add test to compare new default output
marcospb19 Dec 26, 2022
5f6cf75
fix test path regex
marcospb19 Dec 26, 2022
b6f256b
fix test regex to match windows paths
marcospb19 Dec 26, 2022
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
12 changes: 10 additions & 2 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,3 +29,5 @@ predicates = "2.1.1"
tempfile = "3.3.0"
which = { version = "4.3.0", default-features = false }
pretty_assertions = "1.3"
regex = "1.7.0"
unindent = "0.1.11"
8 changes: 4 additions & 4 deletions src/fingerprint.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,21 +215,21 @@ fn remove_not_matching_in_a_dir(
total_disk_space += metadata.len();
if !dry_run {
match remove_file(&path) {
Ok(_) => info!("Successfully removed: {:?}", &path),
Ok(_) => debug!("Successfully removed: {:?}", &path),
Err(e) => warn!("Failed to remove: {:?} {}", &path, e),
};
} else {
info!("Would remove: {:?}", &path);
debug!("Would remove: {:?}", &path);
}
} else if path.is_dir() {
total_disk_space += total_disk_space_dir(&path);
if !dry_run {
match remove_dir_all(&path) {
Ok(_) => info!("Successfully removed: {:?}", &path),
Ok(_) => debug!("Successfully removed: {:?}", &path),
Err(e) => warn!("Failed to remove: {:?} {}", &path, e),
};
} else {
info!("Would remove: {:?}", &path);
debug!("Would remove: {:?}", &path);
}
}
}
Expand Down
30 changes: 24 additions & 6 deletions src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,9 +251,15 @@ fn main() -> anyhow::Result<()> {
for project_path in &paths {
match remove_not_built_with(project_path, matches.value_of("toolchains"), dry_run) {
Ok(cleaned_amount) if dry_run => {
info!("Would clean: {}", format_bytes(cleaned_amount))
info!(
"Would clean: {} from {project_path:?}",
format_bytes(cleaned_amount)
)
}
Ok(cleaned_amount) => info!("Cleaned {}", format_bytes(cleaned_amount)),
Ok(cleaned_amount) => info!(
"Cleaned {} from {project_path:?}",
format_bytes(cleaned_amount)
),
Err(e) => error!(
"{:?}",
e.context(format!("Failed to clean {project_path:?}"))
Expand All @@ -275,9 +281,15 @@ fn main() -> anyhow::Result<()> {
for project_path in &paths {
match remove_older_until_fits(project_path, size, dry_run) {
Ok(cleaned_amount) if dry_run => {
info!("Would clean: {}", format_bytes(cleaned_amount))
info!(
"Would clean: {} from {project_path:?}",
format_bytes(cleaned_amount)
)
}
Ok(cleaned_amount) => info!("Cleaned {}", format_bytes(cleaned_amount)),
Ok(cleaned_amount) => info!(
"Cleaned {} from {project_path:?}",
format_bytes(cleaned_amount)
),
Err(e) => error!("Failed to clean {:?}: {:?}", project_path, e),
};
}
Expand All @@ -297,9 +309,15 @@ fn main() -> anyhow::Result<()> {
for project_path in &paths {
match remove_older_than(project_path, &keep_duration, dry_run) {
Ok(cleaned_amount) if dry_run => {
info!("Would clean: {}", format_bytes(cleaned_amount))
info!(
"Would clean: {} from {project_path:?}",
format_bytes(cleaned_amount)
)
}
Ok(cleaned_amount) => info!("Cleaned {}", format_bytes(cleaned_amount)),
Ok(cleaned_amount) => info!(
"Cleaned {} from {project_path:?}",
format_bytes(cleaned_amount)
),
Err(e) => error!("Failed to clean {:?}: {:?}", project_path, e),
};
}
Expand Down
97 changes: 77 additions & 20 deletions tests/integration.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@ use predicates::{
};
#[allow(unused_imports)]
use pretty_assertions::{assert_eq, assert_ne};
use regex::Regex;
use tempfile::{tempdir, TempDir};
use unindent::unindent;

struct AnyhowWithContext(anyhow::Error);
impl Debug for AnyhowWithContext {
Expand Down Expand Up @@ -49,11 +51,16 @@ fn sweep(args: &[&str]) -> Command {
let mut cmd = Command::new(cargo_bin("cargo-sweep"));
cmd.arg("sweep")
.current_dir(project_dir("sample-project"))
.arg("--verbose")
jyn514 marked this conversation as resolved.
Show resolved Hide resolved
.args(args);
cmd
}

fn run(mut cmd: impl BorrowMut<Command>) -> Assert {
/// Sets the target folder and runs the given command
fn run<'a>(mut cmd: impl BorrowMut<Command>, target: impl Into<Option<&'a Path>>) -> Assert {
if let Some(target) = target.into() {
cmd.borrow_mut().env("CARGO_TARGET_DIR", target);
}
let assert = cmd.borrow_mut().assert().success();
let out = assert.get_output();
let str = |s| std::str::from_utf8(s).unwrap();
Expand All @@ -66,9 +73,7 @@ fn run(mut cmd: impl BorrowMut<Command>) -> Assert {
fn build(project: &str) -> Result<(u64, TempDir)> {
let target = tempdir()?;
let old_size = get_size(target.path())?;
run(cargo(project)
.arg("build")
.env("CARGO_TARGET_DIR", target.path()));
run(cargo(project).arg("build"), target.path());
let new_size = get_size(target.path())?;
assert!(new_size > old_size, "cargo didn't build anything");
Ok((new_size, target))
Expand All @@ -82,18 +87,26 @@ fn clean_and_parse(target: &TempDir, args: &[&str]) -> Result<u64> {
} else {
("Successfully removed", "Cleaned ")
};
let assertion = run(sweep(args).env("CARGO_TARGET_DIR", target.path()))
.stdout(contains(remove_msg).and(contains(clean_msg)));
let assertion =
run(sweep(args), target.path()).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)?
.lines()
.last()
.unwrap()
.split(clean_msg)
.nth(1)
.unwrap();
.unwrap()
.split_inclusive(' ')
.take(2)
.collect::<String>();

let cleaned = amount
.parse::<human_size::Size>()
.context(format!("failed to parse amount {amount}"))?
Expand All @@ -114,7 +127,7 @@ fn count_cleaned(target: &TempDir, args: &[&str], old_size: u64) -> Result<u64>
let one_percent = old_size as f64 * 0.01;
assert!(
diff <= one_percent as u64,
"new_size={new_size}, old_size={old_size}, cleaned={cleaned}, diff={diff}, 1%={one_percent}"
"new_size={new_size}, old_size={old_size}, cleaned={cleaned}, diff={diff}, 1%={one_percent}",
);

Ok(cleaned)
Expand All @@ -131,6 +144,11 @@ fn count_cleaned_dry_run(target: &TempDir, args: &[&str], old_size: u64) -> Resu
Ok(cleaned)
}

fn regex_matches(pattern: &str, text: &str) -> bool {
let pattern = Regex::new(pattern).expect("Failed to compile regex pattern");
pattern.is_match(text)
}

#[test]
fn all_flags() -> TestResult {
let all_combos = [
Expand Down Expand Up @@ -158,7 +176,7 @@ fn stamp_file() -> TestResult {
let (size, target) = build("sample-project")?;

// Create a stamp file for --file.
let assert = run(sweep(&["--stamp", "-v"]));
let assert = run(sweep(&["--stamp"]), target.path());
println!(
"{}",
std::str::from_utf8(&assert.get_output().stdout).unwrap()
Expand All @@ -173,23 +191,63 @@ fn stamp_file() -> TestResult {

// For some reason, we delete the stamp file after `--file` :(
// Recreate it.
run(sweep(&["--stamp"]));
run(sweep(&["--stamp"]), target.path());

let actual_cleaned = count_cleaned(&target, args, size)?;
assert_eq!(actual_cleaned, expected_cleaned);

Ok(())
}

#[test]
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 output = std::str::from_utf8(&assert.get_output().stdout).unwrap();

let regex_skip = r#".+?"#;
Copy link
Collaborator

Choose a reason for hiding this comment

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

this can just be .* instead of needing to make it optional; and at that point I think it would be simpler to inline it instead of using format.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Aha! A regex Gotcha.

Fun fact: Inserting an ? after a quantifier (*, +, ? or {N..M}) switches
to non-greedy matching, which can be good for performance as it decreases
excessive backtracking from the usual greedy eater.

(It's the same symbol to make stuff optional... why...)

Well, that's a tiny text to match so I'll switch to .+ anyways 👍.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I do think this is a bit less readable from the editor, but the GitHub diff doesn't highlight the format placeholder.

image

image


let pattern = unindent(&format!(
r#"\[DEBUG\] cleaning: "/tmp/{regex_skip}" with remove_older_until_fits
\[DEBUG\] size_to_remove: {regex_skip}
\[DEBUG\] Sizing: "/tmp/{regex_skip}/debug" with total_disk_space_in_a_profile
jyn514 marked this conversation as resolved.
Show resolved Hide resolved
\[DEBUG\] Hashs by time: \[
\(
{regex_skip},
"{regex_skip}",
\),
\]
\[DEBUG\] cleaning: "/tmp/{regex_skip}/debug" with remove_not_built_with_in_a_profile
\[DEBUG\] Successfully removed: "/tmp/{regex_skip}/debug/deps/libsample_project-{regex_skip}.rlib"
\[DEBUG\] Successfully removed: "/tmp/{regex_skip}/debug/deps/libsample_project-{regex_skip}.rmeta"
\[DEBUG\] Successfully removed: "/tmp/{regex_skip}/debug/deps/sample_project-{regex_skip}.d"
\[DEBUG\] Successfully removed: "/tmp/{regex_skip}/debug/.fingerprint/sample-project-{regex_skip}"
\[INFO\] Cleaned {regex_skip} from "/tmp/{regex_skip}""#,
));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This does work, but if the test fails, it doesn't give any tips on what changed, I'm not sure if it's a great approach.

Copy link
Collaborator Author

@marcospb19 marcospb19 Dec 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

For reference, here's the output we are matching:

[DEBUG] cleaning: "/tmp/.tmpXhoLjT" with remove_older_until_fits
[DEBUG] size_to_remove: 46978
[DEBUG] Sizing: "/tmp/.tmpXhoLjT/debug" with total_disk_space_in_a_profile
[DEBUG] Hashs by time: [
    (
        13.926894ms,
        "261b05c2354104eb",
    ),
]
[DEBUG] cleaning: "/tmp/.tmpXhoLjT/debug" with remove_not_built_with_in_a_profile
[DEBUG] Successfully removed: "/tmp/.tmpXhoLjT/debug/deps/libsample_project-261b05c2354104eb.rlib"
[DEBUG] Successfully removed: "/tmp/.tmpXhoLjT/debug/deps/libsample_project-261b05c2354104eb.rmeta"
[DEBUG] Successfully removed: "/tmp/.tmpXhoLjT/debug/deps/sample_project-261b05c2354104eb.d"
[DEBUG] Successfully removed: "/tmp/.tmpXhoLjT/debug/.fingerprint/sample-project-261b05c2354104eb"
[INFO] Cleaned 9.43 KiB from "/tmp/.tmpXhoLjT"

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's ok; if you run the test with --nocapture it will show the original output.


assert!(
regex_matches(&pattern, output),
"failed to match pattern with output"
);

Ok(())
}

#[test]
fn hidden() -> TestResult {
// This path is so strange because we use CARGO_TARGET_DIR to set the target to a temporary directory.
// 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"))
.env("CARGO_TARGET_DIR", target.path()))
run(
sweep(args).current_dir(test_dir().join("fresh-prefix")),
target.path(),
)
};

run(&["--maxsize", "0", "-r"]);
Expand Down Expand Up @@ -221,9 +279,10 @@ fn error_output() -> TestResult {
}

let (_, tempdir) = build("sample-project")?;
let assert = run(sweep(&["--installed"])
.env("PATH", test_dir())
.env("CARGO_TARGET_DIR", tempdir.path()));
let assert = run(
sweep(&["--installed"]).env("PATH", test_dir()),
tempdir.path(),
);
assert.stdout(contains("oh no an error"));

Ok(())
Expand All @@ -241,7 +300,7 @@ fn error_status() -> TestResult {

fn golden_reference(args: &[&str], file: &str) -> TestResult {
let mut cmd = Command::new(cargo_bin("cargo-sweep"));
let mut assert = run(cmd.args(args));
let mut assert = run(cmd.args(args), None);

assert = assert.stderr(is_empty());
let actual = std::str::from_utf8(&assert.get_output().stdout)?;
Expand All @@ -264,9 +323,7 @@ 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"))
.env("CARGO_TARGET_DIR", target.path()));
let assert = run(cmd.arg(project_dir("sample-project")), target.path());
assert.stdout(contains("Cleaned"));

Ok(())
Expand Down