From 3ef552c4c1038ce21b73b2282fcfca692474c611 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Thu, 27 Jul 2023 09:52:36 -0700 Subject: [PATCH] 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 | 116 ++++++---------------- lib/tests/test_working_copy_concurrent.rs | 18 +--- lib/tests/test_working_copy_sparse.rs | 36 ++----- lib/testutils/src/lib.rs | 18 ++++ 4 files changed, 57 insertions(+), 131 deletions(-) diff --git a/lib/tests/test_working_copy.rs b/lib/tests/test_working_copy.rs index ed28f6a8a..e1f355ce8 100644 --- a/lib/tests/test_working_copy.rs +++ b/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_id = locked_wc - .snapshot(SnapshotOptions::empty_for_test()) - .unwrap(); - locked_wc.discard(); + let new_tree = test_workspace.snapshot(); + let repo = &test_workspace.repo; 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, repo.store().empty_tree_id()); + assert_eq!(new_tree.id(), wc_commit.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_id = locked_wc - .snapshot(SnapshotOptions::empty_for_test()) - .unwrap(); - locked_wc.discard(); - assert_eq!(new_tree_id, right_tree_id); + let new_tree = test_workspace.snapshot(); + 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_id = locked_wc - .snapshot(SnapshotOptions::empty_for_test()) - .unwrap(); - assert_eq!(new_tree_id, *tree_without_file.id()); - locked_wc.discard(); + let new_tree = test_workspace.snapshot(); + assert_eq!(new_tree.id(), tree_without_file.id()); // 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_id = locked_wc - .snapshot(SnapshotOptions::empty_for_test()) - .unwrap(); - assert_eq!(new_tree_id, *tree_with_file.id()); - locked_wc.discard(); + let new_tree = test_workspace.snapshot(); + assert_eq!(new_tree.id(), tree_with_file.id()); } #[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_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(); + let tree = test_workspace.snapshot(); // 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 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 tree1 = test_workspace.snapshot(); 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 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 tree2 = test_workspace.snapshot(); 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_id = locked_wc - .snapshot(SnapshotOptions::empty_for_test()) - .unwrap(); - locked_wc.discard(); - assert_eq!(new_tree_id, *tree.id()); + let new_tree = test_workspace.snapshot(); + 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 - .workspace - .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(); + let new_tree = test_workspace.snapshot(); + assert_eq!(new_tree.id(), store.empty_tree_id()); 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 - .workspace - .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(); + let new_tree = test_workspace.snapshot(); + assert_eq!(new_tree.id(), store.empty_tree_id()); } #[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_id = locked_wc - .snapshot(SnapshotOptions::empty_for_test()) - .unwrap(); - locked_wc.discard(); - assert_eq!(new_tree_id, tree_id); + let new_tree = test_workspace.snapshot(); + 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); diff --git a/lib/tests/test_working_copy_concurrent.rs b/lib/tests/test_working_copy_concurrent.rs index c446e4bd0..177512249 100644 --- a/lib/tests/test_working_copy_concurrent.rs +++ b/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); - if modified_tree_id == *tree.id() { + let modified_tree = test_workspace.snapshot(); + if modified_tree.id() == tree.id() { num_matches += 1; } // Reset the state for the next round diff --git a/lib/tests/test_working_copy_sparse.rs b/lib/tests/test_working_copy_sparse.rs index 95b5cd684..8a4cea87a 100644 --- a/lib/tests/test_working_copy_sparse.rs +++ b/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_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 modified_tree = test_workspace.snapshot(); 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_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 modified_tree = test_workspace.snapshot(); 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_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 modified_tree = test_workspace.snapshot(); let entries = modified_tree.entries().collect_vec(); assert_eq!(entries.len(), 1); assert_eq!(entries[0].0, dir1_file2_path); diff --git a/lib/testutils/src/lib.rs b/lib/testutils/src/lib.rs index 541a671c0..4a203fca1 100644 --- a/lib/testutils/src/lib.rs +++ b/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 {