Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix test-lib archive creation #1441

Merged
merged 2 commits into from
Jul 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Binary file not shown.
Binary file not shown.
9 changes: 0 additions & 9 deletions gix-diff/tests/tree/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -253,17 +253,8 @@ mod changes {
":100644 100644 28ce6a8b26aa170e1de65536fe8abe1832bd3242 13c2aca72ab576cb5f22dc8e7f8ba8ddab553a8a M f/f"
);

#[cfg(windows)]
let tree_with_link_id = hex_to_id("3b287f8730c81d0b763c2d294618a5e32b67b4f8");
#[cfg(windows)]
let link_entry_oid = hex_to_id("e69de29bb2d1d6434b8b29ae775ad8c2e48c5391");
#[cfg(windows)]
let link_entry_mode = EntryKind::Blob;
#[cfg(not(windows))]
let tree_with_link_id = hex_to_id("7e26dba59b6336f87d1d4ae3505a2da302b91c76");
#[cfg(not(windows))]
let link_entry_oid = hex_to_id("2e65efe2a145dda7ee51d1741299f848e5bf752e");
#[cfg(not(windows))]
let link_entry_mode = EntryKind::Link;
Copy link
Contributor

@EliahKagan EliahKagan Jul 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As detailed in #1442 and mentioned in #1440 (comment), this changes the expected hash values to what I believe they should be on all platforms including Windows, but currently the repository that is created when the fixture script is actually run on Windows produces the situation described by the old values.

This comment is for reference in connection with #1442. I do not advocate the old code here be brought back.

(For some reason GitHub is not showing the whole diff here, even though I selected it when making this comment. Oh well.)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, thank you! I yet have to catch up on the new issues though, to see if there is anything that can be done.

assert_eq!(
diff_with_previous_commit_from(&db, &all_commits["f/f mode changed to link"])?,
Expand Down
2 changes: 1 addition & 1 deletion gix-filter/tests/driver/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ mod baseline {
if cfg!(windows) {
exe = exe.replace('\\', "/");
}
gix_testtools::scripted_fixture_read_only_with_args("baseline.sh", [exe])?;
gix_testtools::scripted_fixture_read_only_with_args_single_archive("baseline.sh", [exe])?;
Ok(())
}
}
Expand Down
1 change: 1 addition & 0 deletions gix-ref/tests/fixtures/generated-archives/.gitignore
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
make_worktree_repo.tar
make_worktree_repo_packed.tar
11 changes: 7 additions & 4 deletions gix/src/remote/connection/fetch/update_refs/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,16 +25,19 @@ mod update {
}

fn repo(name: &str) -> gix::Repository {
let dir =
gix_testtools::scripted_fixture_read_only_with_args("make_fetch_repos.sh", [base_repo_path()]).unwrap();
let dir = gix_testtools::scripted_fixture_read_only_with_args_single_archive(
"make_fetch_repos.sh",
[base_repo_path()],
)
.unwrap();
gix::open_opts(dir.join(name), restricted()).unwrap()
}
fn named_repo(name: &str) -> gix::Repository {
let dir = gix_testtools::scripted_fixture_read_only("make_remote_repos.sh").unwrap();
gix::open_opts(dir.join(name), restricted()).unwrap()
}
fn repo_rw(name: &str) -> (gix::Repository, gix_testtools::tempfile::TempDir) {
let dir = gix_testtools::scripted_fixture_writable_with_args(
let dir = gix_testtools::scripted_fixture_writable_with_args_single_archive(
"make_fetch_repos.sh",
[base_repo_path()],
gix_testtools::Creation::ExecuteScript,
Expand Down Expand Up @@ -189,7 +192,7 @@ mod update {

#[test]
fn checked_out_branches_in_worktrees_are_rejected_with_additional_information() -> Result {
let root = gix_path::realpath(gix_testtools::scripted_fixture_read_only_with_args(
let root = gix_path::realpath(gix_testtools::scripted_fixture_read_only_with_args_single_archive(
"make_fetch_repos.sh",
[base_repo_path()],
)?)?;
Expand Down
1 change: 1 addition & 0 deletions gix/tests/fixtures/generated-archives/.gitignore
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
/make_worktree_repo.tar
/make_worktree_repo_bare.tar
/make_worktree_repo_with_configs.tar
/make_remote_repos.tar
/make_complex_shallow_repo.tar
Expand Down
2 changes: 1 addition & 1 deletion gix/tests/remote/fetch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ mod blocking_and_async_io {
args: impl IntoIterator<Item = S>,
mode: Mode,
) -> Result<(gix::Repository, gix_testtools::tempfile::TempDir), gix::open::Error> {
let dir = gix_testtools::scripted_fixture_writable_with_args(
let dir = gix_testtools::scripted_fixture_writable_with_args_single_archive(
"make_fetch_repos.sh",
[{
let mut url = base_repo_path();
Expand Down
2 changes: 1 addition & 1 deletion gix/tests/repository/shallow.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ mod traverse {
#[parallel]
fn complex_graphs_can_be_iterated_despite_multiple_shallow_boundaries() -> crate::Result {
let base = gix_path::realpath(gix_testtools::scripted_fixture_read_only("make_remote_repos.sh")?.join("base"))?;
let shallow_base = gix_testtools::scripted_fixture_read_only_with_args(
let shallow_base = gix_testtools::scripted_fixture_read_only_with_args_single_archive(
"make_complex_shallow_repo.sh",
Some(base.to_string_lossy()),
)?;
Expand Down
119 changes: 101 additions & 18 deletions tests/tools/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,15 @@ enum DirectoryRoot {
StandaloneTest,
}

/// Don't add a suffix to the archive name as `args` are platform dependent, none-deterministic,
/// or otherwise don't influence the content of the archive.
/// Note that this also means that `args` won't be used to control the hash of the archive itself.
#[derive(Copy, Clone)]
enum ArgsInHash {
Yes,
No,
}

/// Return the path to the `<crate-root>/tests/fixtures/<path>` directory.
pub fn fixture_path(path: impl AsRef<Path>) -> PathBuf {
fixture_path_inner(path, DirectoryRoot::IntegrationTest)
Expand Down Expand Up @@ -309,7 +318,19 @@ pub fn scripted_fixture_writable_with_args(
args: impl IntoIterator<Item = impl Into<String>>,
mode: Creation,
) -> Result<tempfile::TempDir> {
scripted_fixture_writable_with_args_inner(script_name, args, mode, DirectoryRoot::IntegrationTest)
scripted_fixture_writable_with_args_inner(script_name, args, mode, DirectoryRoot::IntegrationTest, ArgsInHash::Yes)
}

/// Like [`scripted_fixture_writable()`], but passes `args` to `script_name` while providing control over
/// the way files are created with `mode`.
///
/// See [`scripted_fixture_read_only_with_args_single_archive()`] for important details on what `single_archive` means.
pub fn scripted_fixture_writable_with_args_single_archive(
script_name: impl AsRef<Path>,
args: impl IntoIterator<Item = impl Into<String>>,
mode: Creation,
) -> Result<tempfile::TempDir> {
scripted_fixture_writable_with_args_inner(script_name, args, mode, DirectoryRoot::IntegrationTest, ArgsInHash::No)
}

/// Like [`scripted_fixture_writable_with_args`], but does not prefix the fixture directory with `tests`
Expand All @@ -318,24 +339,36 @@ pub fn scripted_fixture_writable_with_args_standalone(
args: impl IntoIterator<Item = impl Into<String>>,
mode: Creation,
) -> Result<tempfile::TempDir> {
scripted_fixture_writable_with_args_inner(script_name, args, mode, DirectoryRoot::StandaloneTest)
scripted_fixture_writable_with_args_inner(script_name, args, mode, DirectoryRoot::StandaloneTest, ArgsInHash::Yes)
}

/// Like [`scripted_fixture_writable_with_args`], but does not prefix the fixture directory with `tests`
///
/// See [`scripted_fixture_read_only_with_args_single_archive()`] for important details on what `single_archive` means.
pub fn scripted_fixture_writable_with_args_standalone_single_archive(
script_name: &str,
args: impl IntoIterator<Item = impl Into<String>>,
mode: Creation,
) -> Result<tempfile::TempDir> {
scripted_fixture_writable_with_args_inner(script_name, args, mode, DirectoryRoot::StandaloneTest, ArgsInHash::No)
}

fn scripted_fixture_writable_with_args_inner(
script_name: impl AsRef<Path>,
args: impl IntoIterator<Item = impl Into<String>>,
mode: Creation,
root: DirectoryRoot,
args_in_hash: ArgsInHash,
) -> Result<tempfile::TempDir> {
let dst = tempfile::TempDir::new()?;
Ok(match mode {
Creation::CopyFromReadOnly => {
let ro_dir = scripted_fixture_read_only_with_args_inner(script_name, args, None, root)?;
let ro_dir = scripted_fixture_read_only_with_args_inner(script_name, args, None, root, args_in_hash)?;
copy_recursively_into_existing_dir(ro_dir, dst.path())?;
dst
}
Creation::ExecuteScript => {
scripted_fixture_read_only_with_args_inner(script_name, args, dst.path().into(), root)?;
scripted_fixture_read_only_with_args_inner(script_name, args, dst.path().into(), root, args_in_hash)?;
dst
}
})
Expand Down Expand Up @@ -365,22 +398,49 @@ pub fn scripted_fixture_read_only_with_args(
script_name: impl AsRef<Path>,
args: impl IntoIterator<Item = impl Into<String>>,
) -> Result<PathBuf> {
scripted_fixture_read_only_with_args_inner(script_name, args, None, DirectoryRoot::IntegrationTest)
scripted_fixture_read_only_with_args_inner(script_name, args, None, DirectoryRoot::IntegrationTest, ArgsInHash::Yes)
}

/// Like `scripted_fixture_read_only()`], but passes `args` to `script_name`.
///
/// Also, don't add a suffix to the archive name as `args` are platform dependent, none-deterministic,
/// or otherwise don't influence the content of the archive.
/// Note that this also means that `args` won't be used to control the hash of the archive itself.
///
/// Sometimes, this should be combined with adding the archive name to `.gitignore` to prevent its creation
/// in the first place.
///
/// Note that suffixing archives by default helps to learn what calls are made, and forces the author to
/// think about what should be done to get it right.
pub fn scripted_fixture_read_only_with_args_single_archive(
script_name: impl AsRef<Path>,
args: impl IntoIterator<Item = impl Into<String>>,
) -> Result<PathBuf> {
scripted_fixture_read_only_with_args_inner(script_name, args, None, DirectoryRoot::IntegrationTest, ArgsInHash::No)
}

/// Like [`scripted_fixture_read_only_with_args()`], but does not prefix the fixture directory with `tests`
pub fn scripted_fixture_read_only_with_args_standalone(
script_name: impl AsRef<Path>,
args: impl IntoIterator<Item = impl Into<String>>,
) -> Result<PathBuf> {
scripted_fixture_read_only_with_args_inner(script_name, args, None, DirectoryRoot::StandaloneTest)
scripted_fixture_read_only_with_args_inner(script_name, args, None, DirectoryRoot::StandaloneTest, ArgsInHash::Yes)
}

/// Like [`scripted_fixture_read_only_with_args_standalone()`], only has a single archive.
pub fn scripted_fixture_read_only_with_args_standalone_single_archive(
script_name: impl AsRef<Path>,
args: impl IntoIterator<Item = impl Into<String>>,
) -> Result<PathBuf> {
scripted_fixture_read_only_with_args_inner(script_name, args, None, DirectoryRoot::StandaloneTest, ArgsInHash::No)
}

fn scripted_fixture_read_only_with_args_inner(
script_name: impl AsRef<Path>,
args: impl IntoIterator<Item = impl Into<String>>,
destination_dir: Option<&Path>,
root: DirectoryRoot,
args_in_hash: ArgsInHash,
) -> Result<PathBuf> {
// Assure tempfiles get removed when aborting the test.
gix_tempfile::signal::setup(
Expand Down Expand Up @@ -414,11 +474,23 @@ fn scripted_fixture_read_only_with_args_inner(

let script_basename = script_location.file_stem().unwrap_or(script_location.as_os_str());
let archive_file_path = fixture_path_inner(
Path::new("generated-archives").join(format!(
"{}.tar{}",
script_basename.to_str().expect("valid UTF-8"),
if cfg!(feature = "xz") { ".xz" } else { "" }
)),
{
let suffix = match args_in_hash {
ArgsInHash::Yes => {
let mut suffix = args.join("_");
if !suffix.is_empty() {
suffix.insert(0, '_');
}
suffix.replace(['\\', '/', ' ', '.'], "_")
}
ArgsInHash::No => "".into(),
};
Path::new("generated-archives").join(format!(
"{}{suffix}.tar{}",
script_basename.to_str().expect("valid UTF-8"),
if cfg!(feature = "xz") { ".xz" } else { "" }
))
},
root,
);
let (force_run, script_result_directory) = destination_dir.map_or_else(
Expand Down Expand Up @@ -447,7 +519,15 @@ fn scripted_fixture_read_only_with_args_inner(
std::fs::remove_dir_all(&script_result_directory).map_err(|err| format!("Failed to remove '{script_result_directory:?}', please try to do that by hand. Original error: {err}"))?
}
std::fs::create_dir_all(&script_result_directory)?;
match extract_archive(&archive_file_path, &script_result_directory, script_identity) {
let script_identity_for_archive = match args_in_hash {
ArgsInHash::Yes => script_identity,
ArgsInHash::No => 0,
};
match extract_archive(
&archive_file_path,
&script_result_directory,
script_identity_for_archive,
) {
Ok((archive_id, platform)) => {
eprintln!(
"Extracted fixture from archive '{}' ({}, {:?})",
Expand Down Expand Up @@ -489,12 +569,15 @@ fn scripted_fixture_read_only_with_args_inner(
output.stdout.as_bstr(),
output.stderr.as_bstr()
);
create_archive_if_not_on_ci(&script_result_directory, &archive_file_path, script_identity).map_err(
|err| {
write_failure_marker(&failure_marker);
err
},
)?;
create_archive_if_not_on_ci(
&script_result_directory,
&archive_file_path,
script_identity_for_archive,
)
.map_err(|err| {
write_failure_marker(&failure_marker);
err
})?;
}
}
}
Expand Down
Loading