From 04c2f716f3df717a39a410b9cb1f4ca57f6974e6 Mon Sep 17 00:00:00 2001 From: Austin Gill Date: Mon, 30 Dec 2024 12:03:15 -0600 Subject: [PATCH 1/7] Add cargo-nextest and cargo-mutants instructions to the README --- .cargo/mutants.toml | 1 + README.md | 24 +++++++++++++++++++++++- 2 files changed, 24 insertions(+), 1 deletion(-) create mode 100644 .cargo/mutants.toml diff --git a/.cargo/mutants.toml b/.cargo/mutants.toml new file mode 100644 index 0000000..efa7ea6 --- /dev/null +++ b/.cargo/mutants.toml @@ -0,0 +1 @@ +test_tool = "nextest" diff --git a/README.md b/README.md index 042b94d..ae2f78c 100644 --- a/README.md +++ b/README.md @@ -58,9 +58,31 @@ Achievement { name: "I meant to fix that up later, I swear!", commit: 60b480b554 The usual `cargo build` and `cargo test`. There are a few integration tests that take too long to run every time. These can be run with `cargo test -- --ignored`. +### cargo-nextest + +I recommend using [cargo-nextest](https://nexte.st/) for running the tests. It's not strictly +necessary, but does give a better experience than `cargo test`. + +```sh +cargo nextest run +``` + +### Mutation testing + +Using [cargo-mutants](https://mutants.rs/) for mutation testing is phenomenally easy, and even +though Herostratus has pretty good test coverage, `cargo-mutants` did highlight a few bugs, and +several gaps in tests. + +```sh +cargo mutants --in-place --package herostratus +``` + +While not every issue it points out is worth fixing, it is sometimes a useful tool. + ### Test Branches -There are orphan test branches in this repository used for integration tests. +There are [orphan test branches](https://github.com/Notgnoshi/herostratus/branches/all?query=test) +in this repository used for integration tests. For example, the `test/simple` branch was created like this: From 0c3c9d10dddb3a7d43f297177a82b3eec11e3876 Mon Sep 17 00:00:00 2001 From: Austin Gill Date: Mon, 30 Dec 2024 12:03:39 -0600 Subject: [PATCH 2/7] Improve Rule metadata test coverage A favorite mutation of cargo-mutants is to replace static &str's with "xyzzy". While I think what I want from the Rule metadata is clear to me, it might not be clear to a future contributor (or even my future self). --- herostratus/src/rules/mod.rs | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/herostratus/src/rules/mod.rs b/herostratus/src/rules/mod.rs index 6b1a6d6..7698807 100644 --- a/herostratus/src/rules/mod.rs +++ b/herostratus/src/rules/mod.rs @@ -128,6 +128,28 @@ mod tests { } } + #[test] + fn rule_metadata_characteristics() { + let rules = builtin_rules_all(); + + for rule in &rules { + // Names start with capitals (if they start with an alphabetic character) + let first = rule.name().chars().next().unwrap(); + assert!((first.is_alphabetic() && first.is_uppercase()) || first.is_numeric()); + + // Names are allowed to be a single word, but descriptions are not + let words = rule.description().split_whitespace(); + assert!(words.count() > 2); + + // Human IDs are lower-alphabetic-only, separated by hyphens + let words = rule.human_id().split('-'); + for word in words { + assert!(word.chars().all(|c| c.is_alphabetic())); + assert!(word.chars().all(|c| c.is_lowercase())); + } + } + } + #[test] fn exclude_rules() { let config = RulesConfig { From bf480099f85cc21a1bcbfd77fd304fe94f878e54 Mon Sep 17 00:00:00 2001 From: Austin Gill Date: Sun, 29 Dec 2024 20:41:22 -0600 Subject: [PATCH 3/7] Reproduce fetch bug in a failing test There were several mutations attempted that all relate to this bug: * changing += to *= * changing == to != * etc all in the region that calculates the number of new commits. It turns out that counting the number of new commits only works the first time the branch is pulled, and that subsequent updates don't actually update the local branch. This means that the desired workflow of herostratus add ... herostratus fetch-all herostratus check-all ... time goes by herostratus fetch-all herostratus check-all doesn't actually work as-expected. This might be a tricky bug to fix, and I might end up choosing to not fix it until I finish migrating to gix. The value is in having the tests, since fetching has turned up being more complex than imagined. TODO: There should be an integration test covering the desired workflow. --- herostratus/src/git/clone.rs | 69 ++++++++++++++++++++++++++++++++++-- 1 file changed, 67 insertions(+), 2 deletions(-) diff --git a/herostratus/src/git/clone.rs b/herostratus/src/git/clone.rs index 8a4013e..e347992 100644 --- a/herostratus/src/git/clone.rs +++ b/herostratus/src/git/clone.rs @@ -167,11 +167,24 @@ pub fn pull_branch( // If the fetch was successful, resolving the reference should succeed, even if this was the // first fetch ever for this reference. let reference = repo.resolve_reference_from_short_name(reference_name)?; + // BUG: The fetch doesn't update the local branch (unless it creates the branch), so this + // 'after' remains unchanged after fetching. Can use Reference::set_target to update the commit + // the reference points to, but to do that, we need to find the target of the remote reference. + // + // TODO: This doesn't work, possibly because fetch is unfinished? (TODO: Read about FETCH_HEAD; + // the .git/refs/ only has heads/ and not remotes/ - there's nowhere else where the remote + // upstream target of the reference is tracked). + // + // let remote_reference = repo.find_reference(&format!("origin/{reference_name}"))?; let after = reference.peel_to_commit()?; let mut new_commits: usize = 0; if before.is_some() && before.as_ref().unwrap().id() == after.id() { - tracing::debug!("... done. No new commits"); + tracing::debug!( + "... done. {:?} -> {:?} No new commits", + before.as_ref().unwrap().id(), + after.id() + ); } else { let commits = crate::git::rev::walk(after.id(), repo)?; for commit_id in commits { @@ -182,7 +195,11 @@ pub fn pull_branch( } new_commits += 1; } - tracing::debug!("... done. {new_commits} new commits"); + tracing::debug!( + "... done. {:?} -> {:?} {new_commits} new commits", + before, + after.id() + ); } Ok(new_commits) @@ -413,4 +430,52 @@ mod tests { .find_branch("branch2", git2::BranchType::Remote); assert!(result.is_err()); } + + #[test] + #[ignore = "XFAIL: Reproduces pull_branch bug"] + fn test_number_of_fetched_commits_update_existing() { + let (upstream, downstream) = fixtures::repository::upstream_downstream().unwrap(); + fixtures::repository::switch_branch(&upstream.repo, "branch1").unwrap(); + fixtures::repository::switch_branch(&downstream.repo, "branch1").unwrap(); + + fixtures::repository::add_empty_commit(&upstream.repo, "commit 1 on branch1").unwrap(); + fixtures::repository::add_empty_commit(&upstream.repo, "commit 2 on branch1").unwrap(); + fixtures::repository::add_empty_commit(&upstream.repo, "commit 3 on branch1").unwrap(); + fixtures::repository::add_empty_commit(&upstream.repo, "commit 4 on branch1").unwrap(); + + let downstream = downstream.forget(); + let remote = downstream.find_remote("origin").unwrap(); + let config = crate::config::RepositoryConfig { + branch: Some("branch1".to_string()), + url: remote.url().unwrap().to_string(), + ..Default::default() + }; + + let fetched_commits = pull_branch(&config, &downstream).unwrap(); + assert_eq!(fetched_commits, 4); + } + + #[test] + fn test_number_of_fetched_commits_create_new() { + let (upstream, downstream) = fixtures::repository::upstream_downstream().unwrap(); + // This branch doesn't exist in the downstream repo + fixtures::repository::switch_branch(&upstream.repo, "branch1").unwrap(); + + fixtures::repository::add_empty_commit(&upstream.repo, "commit 1 on branch1").unwrap(); + fixtures::repository::add_empty_commit(&upstream.repo, "commit 2 on branch1").unwrap(); + fixtures::repository::add_empty_commit(&upstream.repo, "commit 3 on branch1").unwrap(); + fixtures::repository::add_empty_commit(&upstream.repo, "commit 4 on branch1").unwrap(); + + let remote = downstream.repo.find_remote("origin").unwrap(); + let config = crate::config::RepositoryConfig { + branch: Some("branch1".to_string()), + url: remote.url().unwrap().to_string(), + ..Default::default() + }; + + let fetched_commits = pull_branch(&config, &downstream.repo).unwrap(); + // The 4 new commits on the branch1 branch, as well as the single commit on the master + // branch + assert_eq!(fetched_commits, 5); + } } From d01093a99dc5e3a3505517bc796b8202206b6df1 Mon Sep 17 00:00:00 2001 From: Austin Gill Date: Sun, 29 Dec 2024 20:47:19 -0600 Subject: [PATCH 4/7] Use a directory that doesn't exist in Config test fixture This improves a test coverage gap highlighted by mutation testing. write_config() creates the data-dir if it doesn't exist. So give it a scenario where the data-dir doesn't exist. --- herostratus-tests/src/fixtures/config.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/herostratus-tests/src/fixtures/config.rs b/herostratus-tests/src/fixtures/config.rs index 78ef822..3de670f 100644 --- a/herostratus-tests/src/fixtures/config.rs +++ b/herostratus-tests/src/fixtures/config.rs @@ -9,6 +9,6 @@ pub struct DataDir { pub fn empty() -> eyre::Result { let tempdir = tempdir()?; - let data_dir = tempdir.path().to_path_buf(); + let data_dir = tempdir.path().join("herostratus"); Ok(DataDir { tempdir, data_dir }) } From e504530fc4625aae6dc88d442bb768c61f3582ae Mon Sep 17 00:00:00 2001 From: Austin Gill Date: Sun, 29 Dec 2024 20:59:26 -0600 Subject: [PATCH 5/7] Improve rule filtering test The mutation switched one of the == to != in the filtering, which caused _every_ rule to be excluded. So change the test so that not every rule is excluded. --- herostratus/src/rules/mod.rs | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/herostratus/src/rules/mod.rs b/herostratus/src/rules/mod.rs index 7698807..3c47c49 100644 --- a/herostratus/src/rules/mod.rs +++ b/herostratus/src/rules/mod.rs @@ -38,7 +38,7 @@ pub fn builtin_rules(config: Option<&Config>) -> Vec> { || rule.human_id() == exclude || &pretty_id == exclude { - tracing::info!("Excluding rule: {pretty_id}"); + tracing::info!("Excluding rule: {pretty_id} due to exclusion rule {exclude:?}"); continue 'outer; } } @@ -154,10 +154,9 @@ mod tests { fn exclude_rules() { let config = RulesConfig { exclude: Some(vec![ - "1".to_string(), - "H2".to_string(), - "longest-subject-line".to_string(), - "H4-non-unicode".to_string(), + "H2".to_string(), // H2, short pretty id + "longest-subject-line".to_string(), // H3, human id + "H4-non-unicode".to_string(), // H4, pretty id ]), ..Default::default() }; @@ -165,9 +164,20 @@ mod tests { rules: Some(config), ..Default::default() }; + + // Rules 1 through 4 are included by default + let all_rules = builtin_rules_all(); + let all_ids: Vec<_> = all_rules.iter().map(|r| r.id()).collect(); + assert!(all_ids.contains(&1)); + assert!(all_ids.contains(&2)); + assert!(all_ids.contains(&3)); + assert!(all_ids.contains(&4)); + let rules = builtin_rules(Some(&config)); let ids: Vec<_> = rules.iter().map(|r| r.id()).collect(); - assert!(!ids.contains(&1)); + + // let at least one rule through, so we can test that we're not excluding everything + assert!(ids.contains(&1)); assert!(!ids.contains(&2)); assert!(!ids.contains(&3)); assert!(!ids.contains(&4)); From 63d67b9bd3ff019b675ad0ee1ac9a89a9c866b33 Mon Sep 17 00:00:00 2001 From: Austin Gill Date: Sun, 29 Dec 2024 21:12:14 -0600 Subject: [PATCH 6/7] Add test for repository cloning The mutation this resolves is removing the remove_dir_contents implementation with a do-nothing impl. --- Cargo.lock | 1 + herostratus/Cargo.toml | 1 + herostratus/src/git/clone.rs | 28 ++++++++++++++++++++++++++++ 3 files changed, 30 insertions(+) diff --git a/Cargo.lock b/Cargo.lock index 157a855..8c58223 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1408,6 +1408,7 @@ dependencies = [ "inventory", "predicates", "serde", + "tempfile", "toml", "tracing", "tracing-subscriber", diff --git a/herostratus/Cargo.toml b/herostratus/Cargo.toml index 8711b7b..c58ec15 100644 --- a/herostratus/Cargo.toml +++ b/herostratus/Cargo.toml @@ -19,6 +19,7 @@ git2.workspace = true gix.workspace = true inventory.workspace = true serde.workspace = true +tempfile.workspace = true toml.workspace = true tracing.workspace = true tracing-subscriber.workspace = true diff --git a/herostratus/src/git/clone.rs b/herostratus/src/git/clone.rs index e347992..983624b 100644 --- a/herostratus/src/git/clone.rs +++ b/herostratus/src/git/clone.rs @@ -478,4 +478,32 @@ mod tests { // branch assert_eq!(fetched_commits, 5); } + + #[test] + fn test_force_clone() { + let upstream = fixtures::repository::simplest().unwrap(); + let tempdir = tempfile::tempdir().unwrap(); + let downstream_dir = tempdir.path().join("downstream"); + std::fs::create_dir_all(&downstream_dir).unwrap(); + // Something's already using the clone directory + let sentinel = downstream_dir.join("sentinel.txt"); + std::fs::File::create(&sentinel).unwrap(); + assert!(sentinel.exists()); + + let config = crate::config::RepositoryConfig { + branch: None, // HEAD + url: format!("file://{}", upstream.tempdir.path().display()), + path: downstream_dir, + ..Default::default() + }; + let force = false; + let result = clone_repository(&config, force); + assert!(result.is_err()); + assert!(sentinel.exists()); + + let force = true; + let result = clone_repository(&config, force); + assert!(result.is_ok()); + assert!(!sentinel.exists()); + } } From 1de9e7693129980fecd4580de0c8c2999fe78ebd Mon Sep 17 00:00:00 2001 From: Austin Gill Date: Sun, 29 Dec 2024 21:18:27 -0600 Subject: [PATCH 7/7] Improve shortest subject line test Mutation: Replace < with ==, which effectively makes the "shorest" item be the last. So re-order the commit subject lines so that the shortest is no longer the last. --- herostratus/src/rules/h002_shortest_subject_line.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/herostratus/src/rules/h002_shortest_subject_line.rs b/herostratus/src/rules/h002_shortest_subject_line.rs index a2c8f02..3a117b3 100644 --- a/herostratus/src/rules/h002_shortest_subject_line.rs +++ b/herostratus/src/rules/h002_shortest_subject_line.rs @@ -101,7 +101,8 @@ mod tests { #[test] fn test_has_short_subject() { let repo = - fixtures::repository::with_empty_commits(&["0123456789", "1234567", "1234"]).unwrap(); + fixtures::repository::with_empty_commits(&["0123456789", "1234", "1234567", "12345"]) + .unwrap(); let rules = vec![Box::new(ShortestSubjectLine::default()) as Box]; let achievements = grant_with_rules("HEAD", &repo.repo, rules).unwrap(); let achievements: Vec<_> = achievements.collect();