Skip to content

Commit

Permalink
Fix bug in new test where temp dir should be a repo
Browse files Browse the repository at this point in the history
The problem was that the path used for environment variables that
inform `env::temp_dir()` was a relative path. Although `temp_dir()`
itself will return an absolute path (resolved from it), the test
changes the current directory. This change occurred between when
the relative path was obtained by the test code and when it was
resolved by the code under test.

The path was therefore resolved to a different location (the last
few path components were repeated) that never exists. Since it
didn't exist, the `git config -l` command failed, and no
installation config file path was returned, causing the test to
wrongly pass (since `None` is the correct result when only local
scope configuration files are available).

The fix is to use an absolute path. In addition, this canonicalizes
the path rather than merely resolving it, and does so even before
changing directory to it, so that if `temp_dir()` "fixes up" the
path in ways beyond resolving it, such as resolving symlinks, the
test is less likely to wrongly pass.

The test now rightly fails, since the hardening it tests for has
not yet been implemented in the code under test.

Further changes to the test are still warranted, at least to clean
it up, and possibly also to make it slightly more robust.
  • Loading branch information
EliahKagan committed Aug 28, 2024
1 parent 287f267 commit 744bb38
Showing 1 changed file with 7 additions and 1 deletion.
8 changes: 7 additions & 1 deletion gix-path/src/env/git/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -391,7 +391,10 @@ fn exe_info_never_from_local_scope() {
#[test]
#[serial]
fn exe_info_never_from_local_scope_even_if_temp_is_here() {
let repo = gix_testtools::scripted_fixture_read_only("local_config.sh").expect("script succeeds");
let repo = gix_testtools::scripted_fixture_read_only("local_config.sh")
.expect("script succeeds")
.canonicalize()
.expect("path is valid and exists");
let repo_str = repo.to_str().expect("valid Unicode");
let _cwd = gix_testtools::set_current_dir(&repo).expect("can change to repo dir");
let _env = gix_testtools::Env::new()
Expand All @@ -400,6 +403,9 @@ fn exe_info_never_from_local_scope_even_if_temp_is_here() {
.set("TMPDIR", repo_str) // Mainly for Unix.
.set("TMP", repo_str) // Mainly for Windows.
.set("TEMP", repo_str); // Mainly for Windows, too.
//assert_eq!(std::env::temp_dir(), repo);
//assert_eq!(std::env::temp_dir(), Path::new("/a/b/c"), "Bogus assertion to debug test");
//std::thread::sleep(std::time::Duration::from_secs(3600));
let maybe_path = super::exe_info();
assert!(
maybe_path.is_none(),
Expand Down

0 comments on commit 744bb38

Please sign in to comment.