Skip to content
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

Closed
EliahKagan opened this issue Jan 25, 2024 · 0 comments · Fixed by #1812
Closed

TestGit.test_refresh changes Git.GIT_PYTHON_GIT_EXECUTABLE for other tests #1811

EliahKagan opened this issue Jan 25, 2024 · 0 comments · Fixed by #1812

Comments

@EliahKagan
Copy link
Contributor

EliahKagan commented Jan 25, 2024

In test_git.py, TestGit.test_refresh tests the behavior of git.refresh when called with bad (nonexistent) and good (existing and usable) explicit path arguments.

  • With the bad path, there could only be a lasting change to Git.GIT_PYTHON_GIT_EXECUTABLE or other global state due to a bug in git.refresh, or in git.cmd.Git.refresh or some other code git.refresh uses.
  • But with the good path, it remains set as Git.GIT_PYTHON_GIT_EXECUTABLE, and tests that run after test_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

def test_refresh(self):
# Test a bad git path refresh.
self.assertRaises(GitCommandNotFound, refresh, "yada")
# Test a good path refresh.
which_cmd = "where" if os.name == "nt" else "command -v"
path = os.popen("{0} git".format(which_cmd)).read().strip().split("\n")[0]
refresh(path)

>>> from git import Git
>>> from test.test_git import TestGit
>>> t = TestGit()
>>> Git.GIT_PYTHON_GIT_EXECUTABLE
'git'
>>> t.test_refresh()
>>> Git.GIT_PYTHON_GIT_EXECUTABLE
'C:\\Users\\ek\\scoop\\shims\\git.exe'

(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 same git 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.

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
Development

Successfully merging a pull request may close this issue.

2 participants