Let gix_testtools::Env
undo multiple changes to the same var
#1560
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 theset
andunset
calls it is undoing were made. Each call appends to the end of the vector withpush
, 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
andunset
calls were made, by iterating throughaltered_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: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 useEnv
where the variables being unset or unset are (partly) determined at runtime; and being able to useEnv
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 sameEnv
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 ofEnv
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:
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.unset
method was added. Theunset
method's docstring is corrected (it had previously been the same asset
), and theEnv
struct docstring is updated to better account for how bothset
andunset
are supported.