tests: add TestWorkspace::snapshot() to simplify snapshotting

It's currently a bit complicated to snapshot the working copy and
there's a lot of duplication in tests. This commit introduces a
function to simplify it. I made the function snapshot the working copy
and save the updated state. Some of the tests I changed previously
discarded the changes instead of saving them, but I think they all did
so because it was simpler. I left a few call sites unchanged because
they make concurrent changes.

lib/tests/test_working_copy.rs

@@ -41,22 +41,18 @@ fn test_root(use_git: bool) {
     // Test that the working copy is clean and empty after init.
     let settings = testutils::user_settings();
     let mut test_workspace = TestWorkspace::init(&settings, use_git);
-    let repo = &test_workspace.repo;
 
-    let wc = test_workspace.workspace.working_copy_mut();
+    let wc = test_workspace.workspace.working_copy();
     assert_eq!(wc.sparse_patterns().unwrap(), vec![RepoPath::root()]);
-    let mut locked_wc = wc.start_mutation().unwrap();
+    let new_tree = test_workspace.snapshot();
-    let new_tree_id = locked_wc
+    let repo = &test_workspace.repo;
-        .snapshot(SnapshotOptions::empty_for_test())
-        .unwrap();
-    locked_wc.discard();
     let wc_commit_id = repo
         .view()
         .get_wc_commit_id(&WorkspaceId::default())
         .unwrap();
     let wc_commit = repo.store().get_commit(wc_commit_id).unwrap();
-    assert_eq!(&new_tree_id, wc_commit.tree_id());
+    assert_eq!(new_tree.id(), wc_commit.tree_id());
-    assert_eq!(&new_tree_id, repo.store().empty_tree_id());
+    assert_eq!(new_tree.id(), repo.store().empty_tree_id());
 }
 
 #[test_case(false ; "local backend")]
@@ -205,12 +201,8 @@ fn test_checkout_file_transitions(use_git: bool) {
         .unwrap();
 
     // Check that the working copy is clean.
-    let mut locked_wc = wc.start_mutation().unwrap();
+    let new_tree = test_workspace.snapshot();
-    let new_tree_id = locked_wc
+    assert_eq!(*new_tree.id(), right_tree_id);
-        .snapshot(SnapshotOptions::empty_for_test())
-        .unwrap();
-    locked_wc.discard();
-    assert_eq!(new_tree_id, right_tree_id);
 
     for (_left_kind, right_kind, path) in &files {
         let wc_path = workspace_root.join(path.to_internal_file_string());
@@ -323,6 +315,7 @@ fn test_reset() {
     let settings = testutils::user_settings();
     let mut test_workspace = TestWorkspace::init(&settings, false);
     let repo = &test_workspace.repo;
+    let op_id = repo.op_id().clone();
     let workspace_root = test_workspace.workspace.workspace_root().clone();
 
     let ignored_path = RepoPath::from_internal_string("ignored");
@@ -347,29 +340,22 @@ fn test_reset() {
     // commit the working copy (because it's ignored).
     let mut locked_wc = wc.start_mutation().unwrap();
     locked_wc.reset(&tree_without_file).unwrap();
-    locked_wc.finish(repo.op_id().clone()).unwrap();
+    locked_wc.finish(op_id.clone()).unwrap();
     assert!(ignored_path.to_fs_path(&workspace_root).is_file());
     assert!(!wc.file_states().unwrap().contains_key(&ignored_path));
-    let mut locked_wc = wc.start_mutation().unwrap();
+    let new_tree = test_workspace.snapshot();
-    let new_tree_id = locked_wc
+    assert_eq!(new_tree.id(), tree_without_file.id());
-        .snapshot(SnapshotOptions::empty_for_test())
-        .unwrap();
-    assert_eq!(new_tree_id, *tree_without_file.id());
-    locked_wc.discard();
 
     // Now test the opposite direction: resetting to a commit where the file is
     // tracked. The file should become tracked (even though it's ignored).
+    let wc = test_workspace.workspace.working_copy_mut();
     let mut locked_wc = wc.start_mutation().unwrap();
     locked_wc.reset(&tree_with_file).unwrap();
-    locked_wc.finish(repo.op_id().clone()).unwrap();
+    locked_wc.finish(op_id.clone()).unwrap();
     assert!(ignored_path.to_fs_path(&workspace_root).is_file());
     assert!(wc.file_states().unwrap().contains_key(&ignored_path));
-    let mut locked_wc = wc.start_mutation().unwrap();
+    let new_tree = test_workspace.snapshot();
-    let new_tree_id = locked_wc
+    assert_eq!(new_tree.id(), tree_with_file.id());
-        .snapshot(SnapshotOptions::empty_for_test())
-        .unwrap();
-    assert_eq!(new_tree_id, *tree_with_file.id());
-    locked_wc.discard();
 }
 
 #[test]
@@ -495,17 +481,13 @@ fn test_snapshot_special_file() {
     // Replace a regular file by a socket and snapshot the working copy again
     std::fs::remove_file(&file1_disk_path).unwrap();
     UnixListener::bind(&file1_disk_path).unwrap();
-    let mut locked_wc = wc.start_mutation().unwrap();
+    let tree = test_workspace.snapshot();
-    let tree_id = locked_wc
-        .snapshot(SnapshotOptions::empty_for_test())
-        .unwrap();
-    locked_wc.finish(OperationId::from_hex("abc123")).unwrap();
-    let tree = store.get_tree(&RepoPath::root(), &tree_id).unwrap();
     // Only the regular file should be in the tree
     assert_eq!(
         tree.entries().map(|(path, _value)| path).collect_vec(),
         vec![file2_path.clone()]
     );
+    let wc = test_workspace.workspace.working_copy();
     assert_eq!(
         wc.file_states().unwrap().keys().cloned().collect_vec(),
         vec![file2_path]
@@ -519,7 +501,6 @@ fn test_gitignores(use_git: bool) {
 
     let settings = testutils::user_settings();
     let mut test_workspace = TestWorkspace::init(&settings, use_git);
-    let repo = &test_workspace.repo;
     let workspace_root = test_workspace.workspace.workspace_root().clone();
 
     let gitignore_path = RepoPath::from_internal_string(".gitignore");
@@ -536,16 +517,7 @@ fn test_gitignores(use_git: bool) {
     std::fs::create_dir(workspace_root.join("dir")).unwrap();
     testutils::write_working_copy_file(&workspace_root, &subdir_modified_path, "1");
 
-    let wc = test_workspace.workspace.working_copy_mut();
+    let tree1 = test_workspace.snapshot();
-    let mut locked_wc = wc.start_mutation().unwrap();
-    let new_tree_id1 = locked_wc
-        .snapshot(SnapshotOptions::empty_for_test())
-        .unwrap();
-    locked_wc.finish(repo.op_id().clone()).unwrap();
-    let tree1 = repo
-        .store()
-        .get_tree(&RepoPath::root(), &new_tree_id1)
-        .unwrap();
     let files1 = tree1.entries().map(|(name, _value)| name).collect_vec();
     assert_eq!(
         files1,
@@ -569,15 +541,7 @@ fn test_gitignores(use_git: bool) {
     testutils::write_working_copy_file(&workspace_root, &subdir_modified_path, "2");
     testutils::write_working_copy_file(&workspace_root, &subdir_ignored_path, "2");
 
-    let mut locked_wc = wc.start_mutation().unwrap();
+    let tree2 = test_workspace.snapshot();
-    let new_tree_id2 = locked_wc
-        .snapshot(SnapshotOptions::empty_for_test())
-        .unwrap();
-    locked_wc.discard();
-    let tree2 = repo
-        .store()
-        .get_tree(&RepoPath::root(), &new_tree_id2)
-        .unwrap();
     let files2 = tree2.entries().map(|(name, _value)| name).collect_vec();
     assert_eq!(
         files2,
@@ -645,12 +609,8 @@ fn test_gitignores_ignored_directory_already_tracked(use_git: bool) {
 
     // Check that the file is still in the tree created by snapshotting the working
     // copy (that it didn't get removed because the directory is ignored)
-    let mut locked_wc = wc.start_mutation().unwrap();
+    let new_tree = test_workspace.snapshot();
-    let new_tree_id = locked_wc
+    assert_eq!(new_tree.id(), tree.id());
-        .snapshot(SnapshotOptions::empty_for_test())
-        .unwrap();
-    locked_wc.discard();
-    assert_eq!(new_tree_id, *tree.id());
 }
 
 #[test_case(false ; "local backend")]
@@ -661,7 +621,7 @@ fn test_dotgit_ignored(use_git: bool) {
 
     let settings = testutils::user_settings();
     let mut test_workspace = TestWorkspace::init(&settings, use_git);
-    let repo = &test_workspace.repo;
+    let store = test_workspace.repo.store().clone();
     let workspace_root = test_workspace.workspace.workspace_root().clone();
 
     // Test with a .git/ directory (with a file in, since we don't write empty
@@ -673,16 +633,8 @@ fn test_dotgit_ignored(use_git: bool) {
         &RepoPath::from_internal_string(".git/file"),
         "contents",
     );
-    let mut locked_wc = test_workspace
+    let new_tree = test_workspace.snapshot();
-        .workspace
+    assert_eq!(new_tree.id(), store.empty_tree_id());
-        .working_copy_mut()
-        .start_mutation()
-        .unwrap();
-    let new_tree_id = locked_wc
-        .snapshot(SnapshotOptions::empty_for_test())
-        .unwrap();
-    assert_eq!(new_tree_id, *repo.store().empty_tree_id());
-    locked_wc.discard();
     std::fs::remove_dir_all(&dotgit_path).unwrap();
 
     // Test with a .git file
@@ -691,16 +643,8 @@ fn test_dotgit_ignored(use_git: bool) {
         &RepoPath::from_internal_string(".git"),
         "contents",
     );
-    let mut locked_wc = test_workspace
+    let new_tree = test_workspace.snapshot();
-        .workspace
+    assert_eq!(new_tree.id(), store.empty_tree_id());
-        .working_copy_mut()
-        .start_mutation()
-        .unwrap();
-    let new_tree_id = locked_wc
-        .snapshot(SnapshotOptions::empty_for_test())
-        .unwrap();
-    assert_eq!(new_tree_id, *repo.store().empty_tree_id());
-    locked_wc.discard();
 }
 
 #[test]
@@ -751,12 +695,8 @@ fn test_gitsubmodule() {
 
     // Check that the files present in the submodule are not tracked
     // when we snapshot
-    let mut locked_wc = wc.start_mutation().unwrap();
+    let new_tree = test_workspace.snapshot();
-    let new_tree_id = locked_wc
+    assert_eq!(*new_tree.id(), tree_id);
-        .snapshot(SnapshotOptions::empty_for_test())
-        .unwrap();
-    locked_wc.discard();
-    assert_eq!(new_tree_id, tree_id);
 
     // Check that the files in the submodule are not deleted
     let file_in_submodule_path = added_submodule_path.to_fs_path(&workspace_root);

lib/tests/test_working_copy_concurrent.rs

@@ -16,8 +16,6 @@ use std::cmp::max;
 use std::thread;
 
 use assert_matches::assert_matches;
-use jj_lib::backend::{ObjectId, TreeId};
-use jj_lib::op_store::OperationId;
 use jj_lib::repo::{Repo, StoreFactories};
 use jj_lib::repo_path::RepoPath;
 use jj_lib::working_copy::{CheckoutError, SnapshotOptions};
@@ -151,24 +149,16 @@ fn test_racy_checkout() {
     let settings = testutils::user_settings();
     let mut test_workspace = TestWorkspace::init(&settings, true);
     let repo = &test_workspace.repo;
+    let op_id = repo.op_id().clone();
     let workspace_root = test_workspace.workspace.workspace_root().clone();
 
     let path = RepoPath::from_internal_string("file");
     let tree = testutils::create_tree(repo, &[(&path, "1")]);
 
-    fn snapshot(workspace: &mut Workspace) -> TreeId {
-        let mut locked_wc = workspace.working_copy_mut().start_mutation().unwrap();
-        let tree_id = locked_wc
-            .snapshot(SnapshotOptions::empty_for_test())
-            .unwrap();
-        locked_wc.finish(OperationId::from_hex("abc123")).unwrap();
-        tree_id
-    }
-
     let mut num_matches = 0;
     for _ in 0..100 {
         let wc = test_workspace.workspace.working_copy_mut();
-        wc.check_out(repo.op_id().clone(), None, &tree).unwrap();
+        wc.check_out(op_id.clone(), None, &tree).unwrap();
         assert_eq!(
             std::fs::read(path.to_fs_path(&workspace_root)).unwrap(),
             b"1".to_vec()
@@ -176,8 +166,8 @@ fn test_racy_checkout() {
         // A file written right after checkout (hopefully, from the test's perspective,
         // within the file system timestamp granularity) is detected as changed.
         write_working_copy_file(&workspace_root, &path, "x");
-        let modified_tree_id = snapshot(&mut test_workspace.workspace);
+        let modified_tree = test_workspace.snapshot();
-        if modified_tree_id == *tree.id() {
+        if modified_tree.id() == tree.id() {
             num_matches += 1;
         }
         // Reset the state for the next round

lib/tests/test_working_copy_sparse.rs

@@ -16,7 +16,7 @@ use itertools::Itertools;
 use jj_lib::matchers::EverythingMatcher;
 use jj_lib::repo::Repo;
 use jj_lib::repo_path::RepoPath;
-use jj_lib::working_copy::{CheckoutStats, SnapshotOptions, WorkingCopy};
+use jj_lib::working_copy::{CheckoutStats, WorkingCopy};
 use testutils::TestWorkspace;
 
 #[test]
@@ -131,6 +131,7 @@ fn test_sparse_commit() {
     let settings = testutils::user_settings();
     let mut test_workspace = TestWorkspace::init(&settings, false);
     let repo = &test_workspace.repo;
+    let op_id = repo.op_id().clone();
     let working_copy_path = test_workspace.workspace.workspace_root().clone();
 
     let root_file1_path = RepoPath::from_internal_string("file1");
@@ -166,36 +167,21 @@ fn test_sparse_commit() {
 
     // Create a tree from the working copy. Only dir1/file1 should be updated in the
     // tree.
-    let mut locked_wc = wc.start_mutation().unwrap();
+    let modified_tree = test_workspace.snapshot();
-    let modified_tree_id = locked_wc
-        .snapshot(SnapshotOptions::empty_for_test())
-        .unwrap();
-    locked_wc.finish(repo.op_id().clone()).unwrap();
-    let modified_tree = repo
-        .store()
-        .get_tree(&RepoPath::root(), &modified_tree_id)
-        .unwrap();
     let diff = tree.diff(&modified_tree, &EverythingMatcher).collect_vec();
     assert_eq!(diff.len(), 1);
     assert_eq!(diff[0].0, dir1_file1_path);
 
     // Set sparse patterns to also include dir2/
+    let wc = test_workspace.workspace.working_copy_mut();
     let mut locked_wc = wc.start_mutation().unwrap();
     let sparse_patterns = vec![dir1_path, dir2_path];
     locked_wc.set_sparse_patterns(sparse_patterns).unwrap();
-    locked_wc.finish(repo.op_id().clone()).unwrap();
+    locked_wc.finish(op_id).unwrap();
 
     // Create a tree from the working copy. Only dir1/file1 and dir2/file1 should be
     // updated in the tree.
-    let mut locked_wc = wc.start_mutation().unwrap();
+    let modified_tree = test_workspace.snapshot();
-    let modified_tree_id = locked_wc
-        .snapshot(SnapshotOptions::empty_for_test())
-        .unwrap();
-    locked_wc.finish(repo.op_id().clone()).unwrap();
-    let modified_tree = repo
-        .store()
-        .get_tree(&RepoPath::root(), &modified_tree_id)
-        .unwrap();
     let diff = tree.diff(&modified_tree, &EverythingMatcher).collect_vec();
     assert_eq!(diff.len(), 2);
     assert_eq!(diff[0].0, dir1_file1_path);
@@ -230,15 +216,7 @@ fn test_sparse_commit_gitignore() {
 
     // Create a tree from the working copy. Only dir1/file2 should be updated in the
     // tree because dir1/file1 is ignored.
-    let mut locked_wc = wc.start_mutation().unwrap();
+    let modified_tree = test_workspace.snapshot();
-    let modified_tree_id = locked_wc
-        .snapshot(SnapshotOptions::empty_for_test())
-        .unwrap();
-    locked_wc.finish(repo.op_id().clone()).unwrap();
-    let modified_tree = repo
-        .store()
-        .get_tree(&RepoPath::root(), &modified_tree_id)
-        .unwrap();
     let entries = modified_tree.entries().collect_vec();
     assert_eq!(entries.len(), 1);
     assert_eq!(entries[0].0, dir1_file2_path);

lib/testutils/src/lib.rs

@@ -30,6 +30,7 @@ use jj_lib::settings::UserSettings;
 use jj_lib::store::Store;
 use jj_lib::tree::Tree;
 use jj_lib::tree_builder::TreeBuilder;
+use jj_lib::working_copy::SnapshotOptions;
 use jj_lib::workspace::Workspace;
 use tempfile::TempDir;
 
@@ -156,6 +157,23 @@ impl TestWorkspace {
     pub fn root_dir(&self) -> PathBuf {
         self.temp_dir.path().join("repo").join("..")
     }
+
+    /// Snapshots the working copy and returns the tree. Updates the working
+    /// copy state on disk, but does not update the working-copy commit (no
+    /// new operation).
+    pub fn snapshot(&mut self) -> Tree {
+        let mut locked_wc = self.workspace.working_copy_mut().start_mutation().unwrap();
+        let tree_id = locked_wc
+            .snapshot(SnapshotOptions::empty_for_test())
+            .unwrap();
+        // arbitrary operation id
+        locked_wc.finish(self.repo.op_id().clone()).unwrap();
+        return self
+            .repo
+            .store()
+            .get_tree(&RepoPath::root(), &tree_id)
+            .unwrap();
+    }
 }
 
 pub fn load_repo_at_head(settings: &UserSettings, repo_path: &Path) -> Arc<ReadonlyRepo> {