From 3b173054c76f5113f36beca3ba5a3a44642e1915 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 26 Aug 2024 08:46:03 -0400 Subject: [PATCH 1/4] doc: Fix description of `gix_testtools::Env::unset` The `unset` method inadvertently had the same docstring as `set`, even though this was not correct for `unset`. This fixes that, and also rewords the `Env` docstring to better account for the ability to unset. --- tests/tools/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/tools/src/lib.rs b/tests/tools/src/lib.rs index 9e1f52587b6..03ff73d2ffa 100644 --- a/tests/tools/src/lib.rs +++ b/tests/tools/src/lib.rs @@ -824,7 +824,7 @@ fn family_name() -> &'static str { } } -/// A utility to set environment variables, while unsetting them (or resetting them to their previous value) on drop. +/// A utility to set and unset environment variables, while restoring or removing them on drop. #[derive(Default)] pub struct Env<'a> { altered_vars: Vec<(&'a str, Option)>, @@ -846,7 +846,7 @@ impl<'a> Env<'a> { self } - /// Set `var` to `value`. + /// Unset `var`. pub fn unset(mut self, var: &'a str) -> Self { let prev = std::env::var_os(var); std::env::remove_var(var); From 505151c9a9f61d9706a803d1cd7b25eaa1a99417 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 26 Aug 2024 08:54:32 -0400 Subject: [PATCH 2/4] Add tests for `gix_testtools::Env` The `nonoverlapping` test, which fortunately is what most closely resembles existing known usage (and most likely all current usage in gitoxide's own test suite), already passes. The other tests test the situation where the same environment variable is affected by multiple `set` or `unset` calls (or both a `set` and an `unset` call). These do not pass yet, because while the assertions about the immediate effect on the environment of each such call all pass, the assertions about the effect after drop fail. This is because, on drop, `Env` currently restores the state of a variable that was most recently saved, i.e., it puts it back to whatever it was just before the most recent modification it made to it. This goes against the intuitive expectation that `Env` will reset things to the way they were before the `Env` object was created and used (so long as all changes were by `set` and `unset` calls). --- Cargo.lock | 1 + tests/tools/Cargo.toml | 3 ++ tests/tools/src/lib.rs | 94 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 98 insertions(+) diff --git a/Cargo.lock b/Cargo.lock index 799429bcd20..a7ae2d8eb73 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2578,6 +2578,7 @@ dependencies = [ "is_ci", "once_cell", "parking_lot", + "serial_test", "tar", "tempfile", "winnow", diff --git a/tests/tools/Cargo.toml b/tests/tools/Cargo.toml index 5ac8e9f7445..e3ca2456452 100644 --- a/tests/tools/Cargo.toml +++ b/tests/tools/Cargo.toml @@ -48,6 +48,9 @@ xz2 = { version = "0.1.6", optional = true } document-features = { version = "0.2.1", optional = true } +[dev-dependencies] +serial_test = { version = "3.1.0", default-features = false } + [package.metadata.docs.rs] all-features = true features = ["document-features"] diff --git a/tests/tools/src/lib.rs b/tests/tools/src/lib.rs index 03ff73d2ffa..e4363c3762f 100644 --- a/tests/tools/src/lib.rs +++ b/tests/tools/src/lib.rs @@ -883,4 +883,98 @@ mod tests { fn parse_version_with_trailing_newline() { assert_eq!(git_version_from_bytes(b"git version 2.37.2\n").unwrap(), (2, 37, 2)); } + + mod env { + use super::Env; + use serial_test::serial; + + // We rely on these not already existing, to test `Env` without using or rewriting it. + static VAR1: &str = "VAR_03FC4045_6043_4A61_9D15_852236CB632B"; + static VAR2: &str = "VAR_8C135840_05DB_4F3A_BFDD_FC755EC35B89"; + static VAR3: &str = "VAR_9B23A2BE_E20B_4670_93E2_3A6A8D47F274"; + + struct TestEnv; + + impl TestEnv { + fn new() -> Self { + assert_eq!(std::env::var_os(VAR1), None); + assert_eq!(std::env::var_os(VAR2), None); + assert_eq!(std::env::var_os(VAR3), None); + Self + } + } + + impl Drop for TestEnv { + fn drop(&mut self) { + std::env::remove_var(VAR1); + std::env::remove_var(VAR2); + std::env::remove_var(VAR3); + } + } + + #[test] + #[serial] + fn nonoverlapping() { + let _meta = TestEnv::new(); + std::env::set_var(VAR1, "old1"); + std::env::set_var(VAR2, "old2"); + { + let _env = Env::new().set(VAR1, "new1").unset(VAR2).set(VAR3, "new3"); + assert_eq!(std::env::var_os(VAR1), Some("new1".into())); + assert_eq!(std::env::var_os(VAR2), None); + assert_eq!(std::env::var_os(VAR3), Some("new3".into())); + } + assert_eq!(std::env::var_os(VAR1), Some("old1".into())); + assert_eq!(std::env::var_os(VAR2), Some("old2".into())); + assert_eq!(std::env::var_os(VAR3), None); + } + + #[test] + #[serial] + fn overlapping_reset() { + let _meta = TestEnv::new(); + { + let _env = Env::new().set(VAR1, "new1A").set(VAR1, "new1B"); + assert_eq!(std::env::var_os(VAR1), Some("new1B".into())); + } + assert_eq!(std::env::var_os(VAR1), None); + } + + #[test] + #[serial] + fn overlapping_unset() { + let _meta = TestEnv::new(); + std::env::set_var(VAR1, "old1"); + { + let _env = Env::new().unset(VAR1).unset(VAR1); + assert_eq!(std::env::var_os(VAR1), None); + } + assert_eq!(std::env::var_os(VAR1), Some("old1".into())); + } + + #[test] + #[serial] + fn overlapping_combo() { + let _meta = TestEnv::new(); + std::env::set_var(VAR1, "old1"); + std::env::set_var(VAR2, "old2"); + { + let _env = Env::new() + .set(VAR1, "new1A") + .unset(VAR2) + .set(VAR1, "new1B") + .unset(VAR3) + .set(VAR2, "new2") + .set(VAR3, "new3") + .unset(VAR1) + .unset(VAR3); + assert_eq!(std::env::var_os(VAR1), None); + assert_eq!(std::env::var_os(VAR2), Some("new2".into())); + assert_eq!(std::env::var_os(VAR3), None); + } + assert_eq!(std::env::var_os(VAR1), Some("old1".into())); + assert_eq!(std::env::var_os(VAR2), Some("old2".into())); + assert_eq!(std::env::var_os(VAR3), None); + } + } } From 581957ea3d810da7529b818604067d16fc025631 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 26 Aug 2024 09:03:11 -0400 Subject: [PATCH 3/4] fix: Let `gix_testtools::Env` undo multiple changes to the same var Previously, an `Env` instance would restore the original state on drop if no more than one modification was made to any one variable through it, but would restore an intermediate state if the same variable was ever set multiple times, unset multiple times, or both set and unset in any order. The state it would restore for each variable was its state immediately before the most recent modification (through the `Env` instance) that affected it, rather than its original state before the first time it was modified through that `Env` instance. This fixes that by undoing the changes in the opposite of the order they were made. --- tests/tools/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/tools/src/lib.rs b/tests/tools/src/lib.rs index e4363c3762f..12803316fa7 100644 --- a/tests/tools/src/lib.rs +++ b/tests/tools/src/lib.rs @@ -857,7 +857,7 @@ impl<'a> Env<'a> { impl<'a> Drop for Env<'a> { fn drop(&mut self) { - for (var, prev_value) in &self.altered_vars { + for (var, prev_value) in self.altered_vars.iter().rev() { match prev_value { Some(value) => std::env::set_var(var, value), None => std::env::remove_var(var), From 555164f2387f77348ab876d05a77c142a69cacfa Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Mon, 26 Aug 2024 09:24:33 -0400 Subject: [PATCH 4/4] Move new tests up into a `tests` subdirectory The new tests of `gix_testtools::Env` are effectively end-to-end tests, since they are involve modifying the actual environment, and more importantly they are only testing the public surface of the crate (`Env` is public), and therefore need not be inside any non-test module. So this moves from residing inside the nested `tests::env` module within `tests/tools/src/lib.rs`, into the newly created `tests/tools/tests/env.rs`. (As expected, they still all pass, and when the fix in `tests/tools/src/lib.rs` is temporarily undone, the `overlapping_*` tests fail again, confirming they still work as regression tests.) --- tests/tools/src/lib.rs | 94 ---------------------------------------- tests/tools/tests/env.rs | 93 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 93 insertions(+), 94 deletions(-) create mode 100644 tests/tools/tests/env.rs diff --git a/tests/tools/src/lib.rs b/tests/tools/src/lib.rs index 12803316fa7..e7cfc546520 100644 --- a/tests/tools/src/lib.rs +++ b/tests/tools/src/lib.rs @@ -883,98 +883,4 @@ mod tests { fn parse_version_with_trailing_newline() { assert_eq!(git_version_from_bytes(b"git version 2.37.2\n").unwrap(), (2, 37, 2)); } - - mod env { - use super::Env; - use serial_test::serial; - - // We rely on these not already existing, to test `Env` without using or rewriting it. - static VAR1: &str = "VAR_03FC4045_6043_4A61_9D15_852236CB632B"; - static VAR2: &str = "VAR_8C135840_05DB_4F3A_BFDD_FC755EC35B89"; - static VAR3: &str = "VAR_9B23A2BE_E20B_4670_93E2_3A6A8D47F274"; - - struct TestEnv; - - impl TestEnv { - fn new() -> Self { - assert_eq!(std::env::var_os(VAR1), None); - assert_eq!(std::env::var_os(VAR2), None); - assert_eq!(std::env::var_os(VAR3), None); - Self - } - } - - impl Drop for TestEnv { - fn drop(&mut self) { - std::env::remove_var(VAR1); - std::env::remove_var(VAR2); - std::env::remove_var(VAR3); - } - } - - #[test] - #[serial] - fn nonoverlapping() { - let _meta = TestEnv::new(); - std::env::set_var(VAR1, "old1"); - std::env::set_var(VAR2, "old2"); - { - let _env = Env::new().set(VAR1, "new1").unset(VAR2).set(VAR3, "new3"); - assert_eq!(std::env::var_os(VAR1), Some("new1".into())); - assert_eq!(std::env::var_os(VAR2), None); - assert_eq!(std::env::var_os(VAR3), Some("new3".into())); - } - assert_eq!(std::env::var_os(VAR1), Some("old1".into())); - assert_eq!(std::env::var_os(VAR2), Some("old2".into())); - assert_eq!(std::env::var_os(VAR3), None); - } - - #[test] - #[serial] - fn overlapping_reset() { - let _meta = TestEnv::new(); - { - let _env = Env::new().set(VAR1, "new1A").set(VAR1, "new1B"); - assert_eq!(std::env::var_os(VAR1), Some("new1B".into())); - } - assert_eq!(std::env::var_os(VAR1), None); - } - - #[test] - #[serial] - fn overlapping_unset() { - let _meta = TestEnv::new(); - std::env::set_var(VAR1, "old1"); - { - let _env = Env::new().unset(VAR1).unset(VAR1); - assert_eq!(std::env::var_os(VAR1), None); - } - assert_eq!(std::env::var_os(VAR1), Some("old1".into())); - } - - #[test] - #[serial] - fn overlapping_combo() { - let _meta = TestEnv::new(); - std::env::set_var(VAR1, "old1"); - std::env::set_var(VAR2, "old2"); - { - let _env = Env::new() - .set(VAR1, "new1A") - .unset(VAR2) - .set(VAR1, "new1B") - .unset(VAR3) - .set(VAR2, "new2") - .set(VAR3, "new3") - .unset(VAR1) - .unset(VAR3); - assert_eq!(std::env::var_os(VAR1), None); - assert_eq!(std::env::var_os(VAR2), Some("new2".into())); - assert_eq!(std::env::var_os(VAR3), None); - } - assert_eq!(std::env::var_os(VAR1), Some("old1".into())); - assert_eq!(std::env::var_os(VAR2), Some("old2".into())); - assert_eq!(std::env::var_os(VAR3), None); - } - } } diff --git a/tests/tools/tests/env.rs b/tests/tools/tests/env.rs new file mode 100644 index 00000000000..4e6a86319dd --- /dev/null +++ b/tests/tools/tests/env.rs @@ -0,0 +1,93 @@ +use std::env; + +use gix_testtools::Env; +use serial_test::serial; + +// We rely on these not already existing, to test `Env` without using or rewriting it. +static VAR1: &str = "VAR_03FC4045_6043_4A61_9D15_852236CB632B"; +static VAR2: &str = "VAR_8C135840_05DB_4F3A_BFDD_FC755EC35B89"; +static VAR3: &str = "VAR_9B23A2BE_E20B_4670_93E2_3A6A8D47F274"; + +struct TestEnv; + +impl TestEnv { + fn new() -> Self { + assert_eq!(env::var_os(VAR1), None); + assert_eq!(env::var_os(VAR2), None); + assert_eq!(env::var_os(VAR3), None); + Self + } +} + +impl Drop for TestEnv { + fn drop(&mut self) { + env::remove_var(VAR1); + env::remove_var(VAR2); + env::remove_var(VAR3); + } +} + +#[test] +#[serial] +fn nonoverlapping() { + let _meta = TestEnv::new(); + env::set_var(VAR1, "old1"); + env::set_var(VAR2, "old2"); + { + let _env = Env::new().set(VAR1, "new1").unset(VAR2).set(VAR3, "new3"); + assert_eq!(env::var_os(VAR1), Some("new1".into())); + assert_eq!(env::var_os(VAR2), None); + assert_eq!(env::var_os(VAR3), Some("new3".into())); + } + assert_eq!(env::var_os(VAR1), Some("old1".into())); + assert_eq!(env::var_os(VAR2), Some("old2".into())); + assert_eq!(env::var_os(VAR3), None); +} + +#[test] +#[serial] +fn overlapping_reset() { + let _meta = TestEnv::new(); + { + let _env = Env::new().set(VAR1, "new1A").set(VAR1, "new1B"); + assert_eq!(env::var_os(VAR1), Some("new1B".into())); + } + assert_eq!(env::var_os(VAR1), None); +} + +#[test] +#[serial] +fn overlapping_unset() { + let _meta = TestEnv::new(); + env::set_var(VAR1, "old1"); + { + let _env = Env::new().unset(VAR1).unset(VAR1); + assert_eq!(env::var_os(VAR1), None); + } + assert_eq!(env::var_os(VAR1), Some("old1".into())); +} + +#[test] +#[serial] +fn overlapping_combo() { + let _meta = TestEnv::new(); + env::set_var(VAR1, "old1"); + env::set_var(VAR2, "old2"); + { + let _env = Env::new() + .set(VAR1, "new1A") + .unset(VAR2) + .set(VAR1, "new1B") + .unset(VAR3) + .set(VAR2, "new2") + .set(VAR3, "new3") + .unset(VAR1) + .unset(VAR3); + assert_eq!(env::var_os(VAR1), None); + assert_eq!(env::var_os(VAR2), Some("new2".into())); + assert_eq!(env::var_os(VAR3), None); + } + assert_eq!(env::var_os(VAR1), Some("old1".into())); + assert_eq!(env::var_os(VAR2), Some("old2".into())); + assert_eq!(env::var_os(VAR3), None); +}