Skip to content

Let gix_testtools::Env undo multiple changes to the same var #1560

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

Merged
merged 4 commits into from
Aug 27, 2024

Conversation

EliahKagan
Copy link
Member

@EliahKagan EliahKagan commented Aug 26, 2024

I believe gix_testtools::Env is meant to undo all changes made through it. But modifying the state of the same environment variable multiple times on the same instance (setting twice, unsetting twice, setting and then unsetting, or unsetting and then setting) breaks this.

This happens because, prior to the changes in this PR, an Env instance undoes the modifications it has made in first-in first-out order, i.e., in the same order the set and unset calls it is undoing were made. Each call appends to the end of the vector with push, and the iteration on drop is done by:

https://github.com/Byron/gitoxide/blob/ec0d03a154777964147aa4a064dd4e5ba38dd78c/tests/tools/src/lib.rs#L860

As a result, the state that was associated with a variable just before its most recent modification is restored, rather than the state associated with it before all modifications through the Env instance.

This PR fixes it by having it undo the modifications in last-in first-out order, i.e., in the opposite of the order the set and unset calls were made, by iterating through altered_vars in reverse. Besides newly introduced tests (which are the bulk of the diff) and some small documentation changes, this PR consists of changing the above iteration to:

https://github.com/Byron/gitoxide/blob/555164f2387f77348ab876d05a77c142a69cacfa/tests/tools/src/lib.rs#L860

altered_vars is thus now an undo stack, though I did not also add any new operations such as undoing the most recent change, because they don't seem to be needed. Furthermore, even though this change makes it easy to add new operations, it likely makes it unnecessary ever to do so, because of a nice property it introduces. The following now have the same effect:

let _env1 = Env::new().set(...).unset(...).set(...);
//      first sequence ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
let _env2 = Env::new().unset(...).set(...).set(...);
//     second sequence ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
let _env = Env::new().set(...).unset(...).set(...)
//     first sequence ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                     .unset(...).set(...).set(...);
//    second sequence ^^^^^^^^^^^^^^^^^^^^^^^^^^^^

So separate Env instances declared one after the other can now be merged and split back up as needed or desired, without worrying about lasting breakage from operating on the same variable multiple times.

Other use cases include being able to create an Env in a helper without having to check for overlapping operations; being able to use Env where the variables being unset or unset are (partly) determined at runtime; and being able to use Env with variables that differ on Unix but are the same on Windows, because environment variable names in Windows, while case-aware, are case-insensitive.

But all that pales in comparison the main "use case": accidentally modifying the same variable multiple times. This should either have an intuitive effect or cause an error. That way, debugging is much easier than silent success with an unintuitive effect, especially since the unintuitive effect would allow tests cases to contaminate global state used by other test cases, even for tests that are made to run in series.

Because gix-testtools, even if used outside of gitoxide's own test suite, is meant for testing, I think it would be a workable alternative to panic when a variable is modified multiple times with the same Env instance. But this would itself be somewhat unexpected, and it is more complex to implement, especially if it is to be implemented properly to account for case equivalence on Windows.

I think the LIFO behavior introduced here already what any users of Env who have not looked at the code of Env would have expected, in any context, and that this expectation is strong enough that this change should be considered a bugfix, even if one does not consider this expectation to follow from the documentation itself. But I think it is a non-breaking change, because I doubt anyone has relied on the previous behavior (except possibly by accident, introducing a bug into their code).

This PR also includes:

  • Tests of gix_testtools::Env. One of these passed before and after the changes made here. The others failed before the changes and passed afterwards. As mentioned above, the actual bugfix is very simple, consisting just of iterating through the vector in the other direction; most of the code in this PR is the tests.
  • Changes to improve docstrings that seem to have been only partially updated when the unset method was added. The unset method's docstring is corrected (it had previously been the same as set), and the Env struct docstring is updated to better account for how both set and unset are supported.

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.
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).
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.
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.)
Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

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

Thank you, a great catch, this makes perfect sense!
It's very nice to see the new tests as well.

@Byron Byron merged commit 2972ea8 into GitoxideLabs:main Aug 27, 2024
15 checks passed
@EliahKagan EliahKagan deleted the run-ci/env-lifo branch August 27, 2024 05:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants