Improve test coverage using results from mutation testing

.cargo/mutants.toml

@@ -0,0 +1 @@
+test_tool = "nextest"

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:
 

herostratus-tests/src/fixtures/config.rs

@@ -9,6 +9,6 @@ pub struct DataDir {
 
 pub fn empty() -> eyre::Result<DataDir> {
     let tempdir = tempdir()?;
-    let data_dir = tempdir.path().to_path_buf();
+    let data_dir = tempdir.path().join("herostratus");
     Ok(DataDir { tempdir, data_dir })
 }

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

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());
+    }
 }

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<dyn Rule>];
         let achievements = grant_with_rules("HEAD", &repo.repo, rules).unwrap();
         let achievements: Vec<_> = achievements.collect();

herostratus/src/rules/mod.rs

@@ -38,7 +38,7 @@ pub fn builtin_rules(config: Option<&Config>) -> Vec<Box<dyn Rule>> {
                 || 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,24 +128,56 @@ 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()
         };
         let config = Config {
             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));