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/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/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: 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 }) } 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 8a4013e..983624b 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,80 @@ 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); + } + + #[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()); + } } 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(); diff --git a/herostratus/src/rules/mod.rs b/herostratus/src/rules/mod.rs index 6b1a6d6..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; } } @@ -128,14 +128,35 @@ 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 { 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() }; @@ -143,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));