Make the gix_path::env::git tests clearer and more granular
#2133
Conversation
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
This also avoids using the phrase "common global program files," since it is easy to misread as something like "global common program files," which would mean the same as "global common files" (a directory like `C:\Program Files\Common Files`, which is unrelated to the intended meaning). The phrase "Most common global program files" is probably clear, though.
This splits the `locations_under_program_files_ordinary` and `locations_under_program_files_strange` test cases, each of which contained multiple assertions, into multiple more specific test cases, each of which is named more descriptively and contains exactly one assertion.
That is, to clarify that "strange" means "strange values," and that "ordinary" means "ordinary values" or, really, "no strange values." The vacuously ordinary case is accordingly commented for clarity.
The vacuous case is ordinary, both in the specific relevant sense of ordinary that there are no strange paths, and in the broader sense that it is expected for overly sanitized environments sometimes not to have any environment variables indicating where program files directories are. However, this is the simplest case, and calling it "ordinary" is confusing. This moves it to be first, renames it, and removes the now-obsolete comment.
The `GetCurrentProcess` and `IsWow64Process` Windows API calls (mediated through the `windows` crate) are pretty straightforward, and this code is only in the test suite, not in production. Still, it's probably best to have a "SAFETY:" comment where `unsafe` code is used. This adds such a comment, covering those two calls.
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.
This revises and adds comments to make clearer what is going on and why in the code that supports
gix-pathtests of theenv::gitmodule.This also splits a two large test cases into smaller cases with one assertion each, both so the new test names can clarify what the tests are for, and so that it is easier to figure out exactly what is breaking if they fail.
This PR only changes test code. It is a prelude to a subsequent PR (edit: #2134), which I plan to open shortly after this is merged, to add entries in
ALTERNATIVE_LOCATIONSto findgit.exein a user-wide installation of Git for Windows even when it is not inPATH.In #2115, test refactoring and clarification came in the same PR as the subsequent new functionality whose testing they supported. I did it that way there because those changes by themselves would've reinforced some commented claims that were no longer true, such as the claim that
clangarm64was not yet being used officially by Git for Windows. In contrast, I think there is no disadvantage to the test suite changes made here, even if the subsequent enhancements take longer than I expect.