Let gix_testtools::Env undo multiple changes to the same var#1560
Merged
Sebastian Thiel (Byron) merged 4 commits intoGitoxideLabs:mainfrom Aug 27, 2024
Merged
Let gix_testtools::Env undo multiple changes to the same var#1560Sebastian Thiel (Byron) merged 4 commits intoGitoxideLabs:mainfrom
gix_testtools::Env undo multiple changes to the same var#1560Sebastian Thiel (Byron) merged 4 commits intoGitoxideLabs:mainfrom
Conversation
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.)
Sebastian Thiel (Byron)
approved these changes
Aug 27, 2024
Member
Sebastian Thiel (Byron)
left a comment
There was a problem hiding this comment.
Thank you, a great catch, this makes perfect sense!
It's very nice to see the new tests as well.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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::Envis 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
Envinstance undoes the modifications it has made in first-in first-out order, i.e., in the same order thesetandunsetcalls 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
Envinstance.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
setandunsetcalls were made, by iterating throughaltered_varsin 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_varsis 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
Envinstances 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
Envin a helper without having to check for overlapping operations; being able to useEnvwhere the variables being unset or unset are (partly) determined at runtime; and being able to useEnvwith 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 sameEnvinstance. 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
Envwho have not looked at the code ofEnvwould 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.unsetmethod was added. Theunsetmethod's docstring is corrected (it had previously been the same asset), and theEnvstruct docstring is updated to better account for how bothsetandunsetare supported.