Skip to content

Commit

Permalink
lib: internalize testutils::new_temp_dir() for unit tests
Browse files Browse the repository at this point in the history
This fixes real dependency cycle that would expose slightly different versions
of jj_lib types to unit tests, one from super::* and another from testutils::*.

The testutils dependency can be removed by splitting lib/tests to a separate
crate.
  • Loading branch information
yuja committed Feb 20, 2025
1 parent 683ee92 commit 88fdcd3
Show file tree
Hide file tree
Showing 9 changed files with 58 additions and 36 deletions.
21 changes: 11 additions & 10 deletions lib/src/default_index/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ mod tests {
use crate::object_id::HexPrefix;
use crate::object_id::ObjectId;
use crate::object_id::PrefixResolution;
use crate::tests::new_temp_dir;

/// Generator of unique 16-byte CommitId excluding root id
fn commit_id_generator() -> impl FnMut() -> CommitId {
Expand All @@ -80,7 +81,7 @@ mod tests {
#[test_case(false; "memory")]
#[test_case(true; "file")]
fn index_empty(on_disk: bool) {
let temp_dir = testutils::new_temp_dir();
let temp_dir = new_temp_dir();
let mutable_segment = MutableIndexSegment::full(3, 16);
let index_segment: Box<DynIndexSegment> = if on_disk {
let saved_index = mutable_segment.save_in(temp_dir.path()).unwrap();
Expand All @@ -107,7 +108,7 @@ mod tests {
#[test_case(false; "memory")]
#[test_case(true; "file")]
fn index_root_commit(on_disk: bool) {
let temp_dir = testutils::new_temp_dir();
let temp_dir = new_temp_dir();
let mut new_change_id = change_id_generator();
let mut mutable_segment = MutableIndexSegment::full(3, 16);
let id_0 = CommitId::from_hex("000000");
Expand Down Expand Up @@ -159,7 +160,7 @@ mod tests {
#[test_case(true, false; "incremental in memory")]
#[test_case(true, true; "incremental on disk")]
fn index_multiple_commits(incremental: bool, on_disk: bool) {
let temp_dir = testutils::new_temp_dir();
let temp_dir = new_temp_dir();
let mut new_change_id = change_id_generator();
let mut mutable_segment = MutableIndexSegment::full(3, 16);
// 5
Expand Down Expand Up @@ -280,7 +281,7 @@ mod tests {
#[test_case(false; "in memory")]
#[test_case(true; "on disk")]
fn index_many_parents(on_disk: bool) {
let temp_dir = testutils::new_temp_dir();
let temp_dir = new_temp_dir();
let mut new_change_id = change_id_generator();
let mut mutable_segment = MutableIndexSegment::full(3, 16);
// 6
Expand Down Expand Up @@ -344,7 +345,7 @@ mod tests {

#[test]
fn resolve_commit_id_prefix() {
let temp_dir = testutils::new_temp_dir();
let temp_dir = new_temp_dir();
let mut new_change_id = change_id_generator();
let mut mutable_segment = MutableIndexSegment::full(3, 16);

Expand Down Expand Up @@ -416,7 +417,7 @@ mod tests {
#[test]
#[allow(clippy::redundant_clone)] // allow id_n.clone()
fn neighbor_commit_ids() {
let temp_dir = testutils::new_temp_dir();
let temp_dir = new_temp_dir();
let mut new_change_id = change_id_generator();
let mut mutable_segment = MutableIndexSegment::full(3, 16);

Expand Down Expand Up @@ -544,7 +545,7 @@ mod tests {

#[test]
fn shortest_unique_commit_id_prefix() {
let temp_dir = testutils::new_temp_dir();
let temp_dir = new_temp_dir();
let mut new_change_id = change_id_generator();
let mut mutable_segment = MutableIndexSegment::full(3, 16);

Expand Down Expand Up @@ -598,7 +599,7 @@ mod tests {

#[test]
fn resolve_change_id_prefix() {
let temp_dir = testutils::new_temp_dir();
let temp_dir = new_temp_dir();
let mut new_commit_id = commit_id_generator();
let local_positions_vec = |positions: &[u32]| -> SmallLocalPositionsVec {
positions.iter().copied().map(LocalPosition).collect()
Expand Down Expand Up @@ -768,7 +769,7 @@ mod tests {

#[test]
fn neighbor_change_ids() {
let temp_dir = testutils::new_temp_dir();
let temp_dir = new_temp_dir();
let mut new_commit_id = commit_id_generator();

let id_0 = ChangeId::from_hex("00000001");
Expand Down Expand Up @@ -914,7 +915,7 @@ mod tests {

#[test]
fn shortest_unique_change_id_prefix() {
let temp_dir = testutils::new_temp_dir();
let temp_dir = new_temp_dir();
let mut new_commit_id = commit_id_generator();

let id_0 = ChangeId::from_hex("00000001");
Expand Down
5 changes: 3 additions & 2 deletions lib/src/file_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,7 @@ mod tests {
use test_case::test_case;

use super::*;
use crate::tests::new_temp_dir;

#[test]
fn normalize_too_many_dot_dot() {
Expand All @@ -233,7 +234,7 @@ mod tests {

#[test]
fn test_persist_no_existing_file() {
let temp_dir = testutils::new_temp_dir();
let temp_dir = new_temp_dir();
let target = temp_dir.path().join("file");
let mut temp_file = NamedTempFile::new_in(&temp_dir).unwrap();
temp_file.write_all(b"contents").unwrap();
Expand All @@ -243,7 +244,7 @@ mod tests {
#[test_case(false ; "existing file open")]
#[test_case(true ; "existing file closed")]
fn test_persist_target_exists(existing_file_closed: bool) {
let temp_dir = testutils::new_temp_dir();
let temp_dir = new_temp_dir();
let target = temp_dir.path().join("file");
let mut temp_file = NamedTempFile::new_in(&temp_dir).unwrap();
temp_file.write_all(b"contents").unwrap();
Expand Down
19 changes: 10 additions & 9 deletions lib/src/git_backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1539,12 +1539,13 @@ mod tests {
use super::*;
use crate::config::StackedConfig;
use crate::content_hash::blake2b_hash;
use crate::tests::new_temp_dir;

#[test_case(false; "legacy tree format")]
#[test_case(true; "tree-level conflict format")]
fn read_plain_git_commit(uses_tree_conflict_format: bool) {
let settings = user_settings();
let temp_dir = testutils::new_temp_dir();
let temp_dir = new_temp_dir();
let store_path = temp_dir.path();
let git_repo_path = temp_dir.path().join("git");
let git_repo = git2::Repository::init(git_repo_path).unwrap();
Expand Down Expand Up @@ -1706,7 +1707,7 @@ mod tests {
#[test]
fn read_git_commit_without_importing() {
let settings = user_settings();
let temp_dir = testutils::new_temp_dir();
let temp_dir = new_temp_dir();
let store_path = temp_dir.path();
let git_repo_path = temp_dir.path().join("git");
let git_repo = git2::Repository::init(git_repo_path).unwrap();
Expand Down Expand Up @@ -1746,7 +1747,7 @@ mod tests {
#[test]
fn read_signed_git_commit() {
let settings = user_settings();
let temp_dir = testutils::new_temp_dir();
let temp_dir = new_temp_dir();
let store_path = temp_dir.path();
let git_repo_path = temp_dir.path().join("git");
let git_repo = git2::Repository::init(git_repo_path).unwrap();
Expand Down Expand Up @@ -1839,7 +1840,7 @@ mod tests {
#[test]
fn git_commit_parents() {
let settings = user_settings();
let temp_dir = testutils::new_temp_dir();
let temp_dir = new_temp_dir();
let store_path = temp_dir.path();
let git_repo_path = temp_dir.path().join("git");
let git_repo = git2::Repository::init(git_repo_path).unwrap();
Expand Down Expand Up @@ -1908,7 +1909,7 @@ mod tests {
#[test]
fn write_tree_conflicts() {
let settings = user_settings();
let temp_dir = testutils::new_temp_dir();
let temp_dir = new_temp_dir();
let store_path = temp_dir.path();
let git_repo_path = temp_dir.path().join("git");
let git_repo = git2::Repository::init(git_repo_path).unwrap();
Expand Down Expand Up @@ -2009,7 +2010,7 @@ mod tests {
#[test]
fn commit_has_ref() {
let settings = user_settings();
let temp_dir = testutils::new_temp_dir();
let temp_dir = new_temp_dir();
let backend = GitBackend::init_internal(&settings, temp_dir.path()).unwrap();
let git_repo = backend.open_git_repo().unwrap();
let signature = Signature {
Expand Down Expand Up @@ -2059,7 +2060,7 @@ mod tests {
#[test]
fn import_head_commits_duplicates() {
let settings = user_settings();
let temp_dir = testutils::new_temp_dir();
let temp_dir = new_temp_dir();
let backend = GitBackend::init_internal(&settings, temp_dir.path()).unwrap();
let git_repo = backend.open_git_repo().unwrap();

Expand Down Expand Up @@ -2095,7 +2096,7 @@ mod tests {
#[test]
fn overlapping_git_commit_id() {
let settings = user_settings();
let temp_dir = testutils::new_temp_dir();
let temp_dir = new_temp_dir();
let backend = GitBackend::init_internal(&settings, temp_dir.path()).unwrap();
let mut commit1 = Commit {
parents: vec![backend.root_commit_id().clone()],
Expand Down Expand Up @@ -2140,7 +2141,7 @@ mod tests {
#[test]
fn write_signed_commit() {
let settings = user_settings();
let temp_dir = testutils::new_temp_dir();
let temp_dir = new_temp_dir();
let backend = GitBackend::init_internal(&settings, temp_dir.path()).unwrap();

let commit = Commit {
Expand Down
14 changes: 14 additions & 0 deletions lib/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -113,3 +113,17 @@ pub mod union_find;
pub mod view;
pub mod working_copy;
pub mod workspace;

#[cfg(test)]
mod tests {
use tempfile::TempDir;

/// Unlike `testutils::new_temp_dir()`, this function doesn't set up
/// hermetic libgit2 environment.
pub fn new_temp_dir() -> TempDir {
tempfile::Builder::new()
.prefix("jj-test-")
.tempdir()
.unwrap()
}
}
3 changes: 2 additions & 1 deletion lib/src/local_backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -547,11 +547,12 @@ mod tests {
use pollster::FutureExt;

use super::*;
use crate::tests::new_temp_dir;

/// Test that parents get written correctly
#[test]
fn write_commit_parents() {
let temp_dir = testutils::new_temp_dir();
let temp_dir = new_temp_dir();
let store_path = temp_dir.path();

let backend = LocalBackend::init(store_path);
Expand Down
5 changes: 3 additions & 2 deletions lib/src/lock/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,11 +47,12 @@ mod tests {
use test_case::test_case;

use super::*;
use crate::tests::new_temp_dir;

#[test_case(FileLock::lock)]
#[cfg_attr(unix, test_case(fallback::FileLock::lock))]
fn lock_basic<T>(lock_fn: fn(PathBuf) -> Result<T, FileLockError>) {
let temp_dir = testutils::new_temp_dir();
let temp_dir = new_temp_dir();
let lock_path = temp_dir.path().join("test.lock");
assert!(!lock_path.exists());
{
Expand All @@ -64,7 +65,7 @@ mod tests {
#[test_case(FileLock::lock)]
#[cfg_attr(unix, test_case(fallback::FileLock::lock))]
fn lock_concurrent<T>(lock_fn: fn(PathBuf) -> Result<T, FileLockError>) {
let temp_dir = testutils::new_temp_dir();
let temp_dir = new_temp_dir();
let data_path = temp_dir.path().join("test");
let lock_path = temp_dir.path().join("test.lock");
fs::write(&data_path, 0_u32.to_le_bytes()).unwrap();
Expand Down
7 changes: 4 additions & 3 deletions lib/src/repo_path.rs
Original file line number Diff line number Diff line change
Expand Up @@ -662,6 +662,7 @@ mod tests {
use itertools::Itertools as _;

use super::*;
use crate::tests::new_temp_dir;

fn repo_path(value: &str) -> &RepoPath {
RepoPath::from_internal_string(value)
Expand Down Expand Up @@ -955,7 +956,7 @@ mod tests {

#[test]
fn parse_fs_path_wc_in_cwd() {
let temp_dir = testutils::new_temp_dir();
let temp_dir = new_temp_dir();
let cwd_path = temp_dir.path().join("repo");
let wc_path = &cwd_path;

Expand Down Expand Up @@ -1014,7 +1015,7 @@ mod tests {

#[test]
fn parse_fs_path_wc_in_cwd_parent() {
let temp_dir = testutils::new_temp_dir();
let temp_dir = new_temp_dir();
let cwd_path = temp_dir.path().join("dir");
let wc_path = cwd_path.parent().unwrap().to_path_buf();

Expand Down Expand Up @@ -1053,7 +1054,7 @@ mod tests {

#[test]
fn parse_fs_path_wc_in_cwd_child() {
let temp_dir = testutils::new_temp_dir();
let temp_dir = new_temp_dir();
let cwd_path = temp_dir.path().join("cwd");
let wc_path = cwd_path.join("repo");

Expand Down
5 changes: 3 additions & 2 deletions lib/src/simple_op_store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -706,6 +706,7 @@ mod tests {
use maplit::hashset;

use super::*;
use crate::tests::new_temp_dir;

fn create_view() -> View {
let new_remote_ref = |target: &RefTarget| RemoteRef {
Expand Down Expand Up @@ -805,7 +806,7 @@ mod tests {

#[test]
fn test_read_write_view() {
let temp_dir = testutils::new_temp_dir();
let temp_dir = new_temp_dir();
let root_data = RootOperationData {
root_commit_id: CommitId::from_hex("000000"),
};
Expand All @@ -818,7 +819,7 @@ mod tests {

#[test]
fn test_read_write_operation() {
let temp_dir = testutils::new_temp_dir();
let temp_dir = new_temp_dir();
let root_data = RootOperationData {
root_commit_id: CommitId::from_hex("000000"),
};
Expand Down
15 changes: 8 additions & 7 deletions lib/src/stacked_table.rs
Original file line number Diff line number Diff line change
Expand Up @@ -553,11 +553,12 @@ mod tests {
use test_case::test_case;

use super::*;
use crate::tests::new_temp_dir;

#[test_case(false; "memory")]
#[test_case(true; "file")]
fn stacked_table_empty(on_disk: bool) {
let temp_dir = testutils::new_temp_dir();
let temp_dir = new_temp_dir();
let store = TableStore::init(temp_dir.path().to_path_buf(), 3);
let mut_table = store.get_head().unwrap().start_mutation();
let mut _saved_table = None;
Expand All @@ -577,7 +578,7 @@ mod tests {
#[test_case(false; "memory")]
#[test_case(true; "file")]
fn stacked_table_single_key(on_disk: bool) {
let temp_dir = testutils::new_temp_dir();
let temp_dir = new_temp_dir();
let store = TableStore::init(temp_dir.path().to_path_buf(), 3);
let mut mut_table = store.get_head().unwrap().start_mutation();
mut_table.add_entry(b"abc".to_vec(), b"value".to_vec());
Expand All @@ -598,7 +599,7 @@ mod tests {
#[test_case(false; "memory")]
#[test_case(true; "file")]
fn stacked_table_multiple_keys(on_disk: bool) {
let temp_dir = testutils::new_temp_dir();
let temp_dir = new_temp_dir();
let store = TableStore::init(temp_dir.path().to_path_buf(), 3);
let mut mut_table = store.get_head().unwrap().start_mutation();
mut_table.add_entry(b"zzz".to_vec(), b"val3".to_vec());
Expand All @@ -624,7 +625,7 @@ mod tests {

#[test]
fn stacked_table_multiple_keys_with_parent_file() {
let temp_dir = testutils::new_temp_dir();
let temp_dir = new_temp_dir();
let store = TableStore::init(temp_dir.path().to_path_buf(), 3);
let mut mut_table = store.get_head().unwrap().start_mutation();
mut_table.add_entry(b"abd".to_vec(), b"value 2".to_vec());
Expand Down Expand Up @@ -654,7 +655,7 @@ mod tests {

#[test]
fn stacked_table_merge() {
let temp_dir = testutils::new_temp_dir();
let temp_dir = new_temp_dir();
let store = TableStore::init(temp_dir.path().to_path_buf(), 3);
let mut mut_base_table = store.get_head().unwrap().start_mutation();
mut_base_table.add_entry(b"abc".to_vec(), b"value1".to_vec());
Expand Down Expand Up @@ -687,7 +688,7 @@ mod tests {
#[test]
fn stacked_table_automatic_merge() {
// Same test as above, but here we let the store do the merging on load
let temp_dir = testutils::new_temp_dir();
let temp_dir = new_temp_dir();
let store = TableStore::init(temp_dir.path().to_path_buf(), 3);
let mut mut_base_table = store.get_head().unwrap().start_mutation();
mut_base_table.add_entry(b"abc".to_vec(), b"value1".to_vec());
Expand Down Expand Up @@ -724,7 +725,7 @@ mod tests {

#[test]
fn stacked_table_store_save_empty() {
let temp_dir = testutils::new_temp_dir();
let temp_dir = new_temp_dir();
let store = TableStore::init(temp_dir.path().to_path_buf(), 3);

let mut mut_table = store.get_head().unwrap().start_mutation();
Expand Down

0 comments on commit 88fdcd3

Please sign in to comment.