-
-
Notifications
You must be signed in to change notification settings - Fork 919
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
TestGit.test_refresh changes Git.GIT_PYTHON_GIT_EXECUTABLE for other tests #1811
Labels
Comments
EliahKagan
added a commit
to EliahKagan/GitPython
that referenced
this issue
Jan 25, 2024
+ Test a successful refresh with a relative path, which will be safer to do once the refresh tests restore changed state. See gitpython-developers#1811. This addresses it incompletely, because while it is probably not necessary while running the test suite to preserve an old False value of git.GIT_OK (most tests can't work if that happens anyway), the value of Git.GIT_PYTHON_GIT_EXECUTABLE is not the only other global state that effects the behavior of subsequently run tests and that may be changed as a result of the refresh tests. 1. After the git.refresh function calls git.cmd.Git.refresh, it calls git.remote.FetchInfo.refresh, which rebinds the git.remote.FetchInfo._flag_map attribute. 2. Future changes to git.cmd.Git.refresh may mutate other state in the Git class, and ideally the coupling would be loose enough that the refresh tests wouldn't have to be updated for that if the behavior being tested does not change. 3. Future changes to git.refresh may perform other refreshing actions, and ideally it would be easy (and obvious) what has to be done to patch it back. In particular, it will likely call another Git method that mutates class-wide state due to gitpython-developers#1791, and for such state that is also of the Git class, ideally no further changes would have to be made to the code that restores state after the refresh tests. If we assume git.refresh is working at least in the case that it is called with no arguments, then the cleanup can just be a call to git.refresh(). Otherwise, sufficiently general cleanup may be more complicated.
EliahKagan
added a commit
to EliahKagan/GitPython
that referenced
this issue
Jan 25, 2024
For gitpython-developers#1811. This is the simple way to do it. This relies on git.refresh working when called when no arguments, so if that ever breaks, then it may be difficult or inefficient to investigate. But this is simpler than any alternatives that are equally or more robust. This is already better than the previous incomplete way that only restored Git.GIT_PYTHON_GIT_EXECUTABLE (and nothing in FetchInfo). Furthermore, because the tests already depend to a large extent on git.refreh working when called with no arguments, as otherwise the initial refresh may not have set Git.GIT_PYTHON_GIT_EXECUTABLE usably, it might not be worthwhile to explore other approaches.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
In
test_git.py
,TestGit.test_refresh
tests the behavior ofgit.refresh
when called with bad (nonexistent) and good (existing and usable) explicitpath
arguments.Git.GIT_PYTHON_GIT_EXECUTABLE
or other global state due to a bug ingit.refresh
, or ingit.cmd.Git.refresh
or some other codegit.refresh
uses.Git.GIT_PYTHON_GIT_EXECUTABLE
, and tests that run aftertest_refresh
use that instead of the original value that (when tests are run normally in an environment that supports them) is usually"git"
.GitPython/test/test_git.py
Lines 309 to 316 in 307f198
(That was run on Windows, but the effect is not specific to Windows.)
Although the good-path code can be simplified in the way it finds the absolute path it tests, that is not the cause of this.
In the absence of bugs in the code under test, this change to
Git.GIT_PYTHON_GIT_EXECUTABLE
between some tests and others should make no difference on most working test systems, because both the old and new paths should both work and typically run the samegit
installation. However, if there are bugs related to this attribute that the test suite should reveal or that otherwise interact with tests, then I think this may lead to inaccurate or less useful results, or complicate troubleshooting.The text was updated successfully, but these errors were encountered: