Make clear every test's status in every CI run by EliahKagan · Pull Request #1679 · gitpython-developers/GitPython
added 30 commits
September 25, 2023 10:55This makes jobs from both test workflows give more information relevant to examining which tests are skipped (and if any tests xfail, those too) in what environments: - Values of os.name and git.util.is_win. - The name of each test that runs, with its status. The latter doesn't increase the output length as much as might be expected, because due to the way the output is handled, the pytest-sugar pretty output format without -v looked like: test/test_actor.py ✓ 0% test/test_actor.py ✓✓ 0% ▏ test/test_actor.py ✓✓✓ 1% ▏ test/test_actor.py ✓✓✓✓ 1% ▏ When instead it was intended to fit on a single line. Still, the current output with -v has extra newlines, increasing length and worsening readability, so it should be improved on if possible.
Instead of splitting it in into two places where at least one of the places is highly likely to be missed, this puts it together just before the first steps that makes nontrivial use of the installed packages. Grouping it together, it can't really be shown earlier, because one of the pieces of information is obtained using the git module (to examine that behavior of the code). This also presents the information more clearly. "set -x" makes this easy, so the commands are rewritten to take advantage of it.
There are two benefits of the pytest-sugar plugin: 1. Pretty output. 2. Show details on each failure immediately instead of at the end. The first benefit is effectively local-only, because extra newlines are appearing when it runs on CI, both with and without -v. The second benefit applies both locally and on CI. So this adds the pytest-instafail plugin and uses it on CI to get the second benefit. It is not set up to run automatically, and pytest-sugar still is (though no longer forced), so local testing retains no benefit and we don't have a clash. The name "instafail" refers only to instantly *seeing* failures: it does not cause the pytest runner to stop earlier than otherwise.
While pytest-sugar output gets mangled with extra newlines on CI, colorized output seems to work fine and improves readability.
This permits the longer delay in test_blocking_lock_file--which was already allowed for native Windows--on Cygwin, where it is also needed. That lets the xfail mark for Cygwin be removed. This also updates the comments to avoid implying that the need for the delay is AppVeyor-specific (it seems needed on CI and locally).
They were not running on Cygwin, because git.util.is_win is False on Cygwin. They were running on native Windows, with a number of them always failing; these failures had sometimes been obscured by the --maxfail=10 that had formerly been used (from pyproject.toml). Many of them (not all the same ones) fail on Cygwin, and it might be valuable for cygpath to work on other platforms, especially native Windows. But I think it still makes sense to limit the tests to Cygwin at this time, because all the uses of cygpath in the project are in code that only runs after a check that the platform is Cygwin. Part of that check, as it is implemented, explicitly excludes native Windows (is_win must be false).
Two of the groups of cygpath tests in test_util.py generate tests that fail on Cygwin. There is no easy way to still run, but xfail, just the specific tests that fail, because the groups of tests are generated with `@ddt` parameterization, but neither the unittest nor pytest xfail mechanisms interact with that. If `@pytest.mark.parametrized` were used, this could be done. But that does not work on methods of test classes that derive from unittest.TestCase, including those in this project that indirectly derive from it by deriving from TestBase. The TestBase base class cannot be removed without overhauling many tests, due to fixtures it provides such as rorepo. So this marks too many tests as xfail, but in doing so allows test runs to pass while still exercising and showing status on all the tests, allowing result changes to be observed easily.
This changes a default Windows skip of test_commit_msg_hook_success to an xfail, and makes it more specific, expecting failure only when either bash.exe is unavailable (definitely expected) or when bash.exe is the WSL bash wrapper in System32, which fails for some reason even though it's not at all clear it ought to. This showcases the failures rather than skipping, and also lets the test pass on Windows systems where bash.exe is something else, including the Git Bash bash.exe that native Windows CI would use.
This makes the test explicitly error out, rather than skipping, if it appears the environment doesn't support encoding Unicode filenames. Platforms these days should be capable of that, and reporting it as an error lessens the risk of missing a bug in the test code (that method or a fixture) if one is ever introduced. Erroring out will also make it easier to see the details in the chained UnicodeDecodeError exception. This does not affect the behavior of GitPython itself. It only changes how a test reports an unusual condition that keeps the test\ from being usefully run.
Although GitPython does not require git >=2.5.1 in general, and this does *not* change that, this makes the unavailability of git 2.5.1 or later an error in test_linked_worktree_traversal, where it is needed to exercise that test, rather than skipping the test. A few systems, such as CentOS 7, may have downstream patched versions of git that remain safe to use yet are numbered <2.5.1 and do not have the necesary feature to run this test. But by now, users of those systems likely anticipate that other software would rely on the presence of features added in git 2.5.1, which was released over 7 years ago. As such, I think it is more useful to give an error for that test, so the test's inability to be run on the system is clear, than to automatically skip the test, which is likely to go unnoticed.
It looked like test_untracked_files was sometimes skipped, and
specifically that it would be skipped on Cygwin. But the `@skipIf`
on it had the condition:
HIDE_WINDOWS_KNOWN_ERRORS and Git.is_cygwin()
HIDE_WINDOWS_KNOWN_ERRORS can only ever be true if it is set to a
truthy value directly (not an intended use as it's a "constant"),
or on native Windows systems: no matter how the environment
variable related to it is set, it's only checked if is_win, which
is set by checking os.name, which is only "nt" on native Windows
systems, not Cygwin.
So whenever HIDE_WINDOWS_KNOWN_ERRORS is true Git.is_cygwin() will
be false. Thus this condition is never true and the test was never
being skipped anyway: it was running and passing on Cygwin.
In the tests only, and not in any way affecting the feature set or requirements of GitPython itself. This is similar to, and with the same reasoning as, cf5f1dc.
And rewrite the reason to give more useful information. (The new reason also doesn't state the exception type, because that is now specified, and checked by pytest, by being passed as "raises".)
This is working on Cygwin, so that old reason no longer applies. (The test was not being skipped on Cygwin, and was passing.) It is not working on native Windows, due to a PermissionError from attempting to move a file that is still open (which Windows doesn't allow). That may have been the original native Windows skip reason, but the old AppVeyor CI link for it is broken or not public. This makes the reason clear, though maybe I should add more details.
… xfail And improve details. The xfail is only for native Windows, not Cygwin (same as the old skip was, and still via checking HIDE_WINDOWS_KNOWN_ERRORS). This change is analogous to the change in c1798f5, but for test_git_submodules_and_add_sm_with_new_commit rather than test_root_module.
I had forgotten to do this earlier when converting from skip to xfail. Besides consistency with the other uses of xfail in the test suite, the benefit of passing "raises" is that pytest checks that the failure gave the expected exception and makes it a non-expected failure if it didn't.
I had put that step in the Cygwin workflow for purposes of experimentation, and it seemed to make clearer what is going on, but really it does the opposite: it's deceptive because Cygwin uses other logic to set its PATH. So this step is unnecessary and ineffective at doing what it appears to do.
This was referenced
Nov 3, 2023EliahKagan added a commit to EliahKagan/GitPython that referenced this pull request
Nov 20, 2023This makes three of the four hook-related tests pass instead of failing, and changes the way the fourth fails. GitPython uses bash.exe to run hooks that are (or appear to be) shell scripts. On many Windows systems, this is the bash.exe in the system32 directory, which delegates to bash in a WSL system if at least one such system is installed (for the current user), and gives an error otherwise. It may be a bug that GitPython ends up using this bash.exe when WSL is installed but no WSL systems exist, since that is actually a fairly common situation. One place that happened was on the GitHub Actions runners used for Windows jobs. Those runners have WSL available, and are capable of running WSL 1 systems (not currently WSL 2 systems), but no WSL systems were actually installed. This commit fixes that cause of failure, for all four tests it happened in, by setting up a Debian WSL system on the test runner. (This increases the amount of time it takes Windows jobs to run, but that might be possible to improve on.) Three of those four tests now pass, while the other fails for another reason. The newly passing tests are: - test/test_index.py::TestIndex::test_commit_msg_hook_fail - test/test_index.py::TestIndex::test_pre_commit_hook_fail - test/test_index.py::TestIndex::test_pre_commit_hook_success The test that still fails, but differently, is: - test/test_index.py::TestIndex::test_commit_msg_hook_success I had previously found that test to fail on a local WSL 2 system, and attempted to add a suitable xfail marking in 881456b (gitpython-developers#1679). But the condition I wrote there *appears* to have a bug related to the different orders in which subproces.Popen and shutil.which find executables, causing it not always to detect when the WSL-related bash.exe is the one the Popen call in git.index.fun.run_commit_hook will use.
EliahKagan added a commit to EliahKagan/GitPython that referenced this pull request
Nov 22, 2023This makes three of the four hook-related tests pass instead of failing, and changes the way the fourth fails. GitPython uses bash.exe to run hooks that are (or appear to be) shell scripts. On many Windows systems, this is the bash.exe in the system32 directory, which delegates to bash in a WSL system if at least one such system is installed (for the current user), and gives an error otherwise. It may be a bug that GitPython ends up using this bash.exe when WSL is installed but no WSL systems exist, since that is actually a fairly common situation. One place that happened was on the GitHub Actions runners used for Windows jobs. Those runners have WSL available, and are capable of running WSL 1 systems (not currently WSL 2 systems), but no WSL systems were actually installed. This commit fixes that cause of failure, for all four tests it happened in, by setting up a Debian WSL system on the test runner. (This increases the amount of time it takes Windows jobs to run, but that might be possible to improve on.) Three of those four tests now pass, while the other fails for another reason. The newly passing tests are: - test/test_index.py::TestIndex::test_commit_msg_hook_fail - test/test_index.py::TestIndex::test_pre_commit_hook_fail - test/test_index.py::TestIndex::test_pre_commit_hook_success The test that still fails, but differently, is: - test/test_index.py::TestIndex::test_commit_msg_hook_success I had previously found that test to fail on a local WSL 2 system, and attempted to add a suitable xfail marking in 881456b (gitpython-developers#1679). But the condition I wrote there *appears* to have a bug related to the different orders in which subproces.Popen and shutil.which find executables, causing it not always to detect when the WSL-related bash.exe is the one the Popen call in git.index.fun.run_commit_hook will use.
EliahKagan added a commit to EliahKagan/GitPython that referenced this pull request
Nov 22, 2023This makes three of the four hook-related tests pass instead of failing, and changes the way the fourth fails. GitPython uses bash.exe to run hooks that are (or appear to be) shell scripts. On many Windows systems, this is the bash.exe in the system32 directory, which delegates to bash in a WSL system if at least one such system is installed (for the current user), and gives an error otherwise. It may be a bug that GitPython ends up using this bash.exe when WSL is installed but no WSL systems exist, since that is actually a fairly common situation. One place that happened was on the GitHub Actions runners used for Windows jobs. Those runners have WSL available, and are capable of running WSL 1 systems (not currently WSL 2 systems), but no WSL systems were actually installed. This commit fixes that cause of failure, for all four tests it happened in, by setting up a Debian WSL system on the test runner. (This increases the amount of time it takes Windows jobs to run, but that might be possible to improve on.) Three of those four tests now pass, while the other fails for another reason. The newly passing tests are: - test/test_index.py::TestIndex::test_commit_msg_hook_fail - test/test_index.py::TestIndex::test_pre_commit_hook_fail - test/test_index.py::TestIndex::test_pre_commit_hook_success The test that still fails, but differently, is: - test/test_index.py::TestIndex::test_commit_msg_hook_success I had previously found that test to fail on a local WSL 2 system, and attempted to add a suitable xfail marking in 881456b (gitpython-developers#1679). But the condition I wrote there *appears* to have a bug related to the different orders in which subproces.Popen and shutil.which find executables, causing it not always to detect when the WSL-related bash.exe is the one the Popen call in git.index.fun.run_commit_hook will use.
EliahKagan added a commit to EliahKagan/GitPython that referenced this pull request
Nov 22, 2023This makes three of the four hook-related tests pass instead of failing, and changes the way the fourth fails. GitPython uses bash.exe to run hooks that are (or appear to be) shell scripts. On many Windows systems, this is the bash.exe in the system32 directory, which delegates to bash in a WSL system if at least one such system is installed (for the current user), and gives an error otherwise. It may be a bug that GitPython ends up using this bash.exe when WSL is installed but no WSL systems exist, since that is actually a fairly common situation. One place that happened was on the GitHub Actions runners used for Windows jobs. Those runners have WSL available, and are capable of running WSL 1 systems (not currently WSL 2 systems), but no WSL systems were actually installed. This commit fixes that cause of failure, for all four tests it happened in, by setting up a Debian WSL system on the test runner. (This increases the amount of time it takes Windows jobs to run, but that might be possible to improve on.) Three of those four tests now pass, while the other fails for another reason. The newly passing tests are: - test/test_index.py::TestIndex::test_commit_msg_hook_fail - test/test_index.py::TestIndex::test_pre_commit_hook_fail - test/test_index.py::TestIndex::test_pre_commit_hook_success The test that still fails, but differently, is: - test/test_index.py::TestIndex::test_commit_msg_hook_success I had previously found that test to fail on a local WSL 2 system, and attempted to add a suitable xfail marking in 881456b (gitpython-developers#1679). But the condition I wrote there *appears* to have a bug related to the different orders in which subproces.Popen and shutil.which find executables, causing it not always to detect when the WSL-related bash.exe is the one the Popen call in git.index.fun.run_commit_hook will use.
EliahKagan added a commit to EliahKagan/GitPython that referenced this pull request
Nov 24, 2023This makes three of the four hook-related tests pass instead of failing, and changes the way the fourth fails. GitPython uses bash.exe to run hooks that are (or appear to be) shell scripts. On many Windows systems, this is the bash.exe in the system32 directory, which delegates to bash in a WSL system if at least one such system is installed (for the current user), and gives an error otherwise. It may be a bug that GitPython ends up using this bash.exe when WSL is installed but no WSL systems exist, since that is actually a fairly common situation. One place that happened was on the GitHub Actions runners used for Windows jobs. Those runners have WSL available, and are capable of running WSL 1 systems (not currently WSL 2 systems), but no WSL systems were actually installed. This commit fixes that cause of failure, for all four tests it happened in, by setting up a Debian WSL system on the test runner. (This increases the amount of time it takes Windows jobs to run, but that might be possible to improve on.) Three of those four tests now pass, while the other fails for another reason. The newly passing tests are: - test/test_index.py::TestIndex::test_commit_msg_hook_fail - test/test_index.py::TestIndex::test_pre_commit_hook_fail - test/test_index.py::TestIndex::test_pre_commit_hook_success The test that still fails, but differently, is: - test/test_index.py::TestIndex::test_commit_msg_hook_success I had previously found that test to fail on a local WSL 2 system, and attempted to add a suitable xfail marking in 881456b (gitpython-developers#1679). But the condition I wrote there *appears* to have a bug related to the different orders in which subproces.Popen and shutil.which find executables, causing it not always to detect when the WSL-related bash.exe is the one the Popen call in git.index.fun.run_commit_hook will use.
EliahKagan added a commit to EliahKagan/GitPython that referenced this pull request
Nov 24, 2023This makes three of the four hook-related tests pass instead of failing, and changes the way the fourth fails. GitPython uses bash.exe to run hooks that are (or appear to be) shell scripts. On many Windows systems, this is the bash.exe in the system32 directory, which delegates to bash in a WSL system if at least one such system is installed (for the current user), and gives an error otherwise. It may be a bug that GitPython ends up using this bash.exe when WSL is installed but no WSL systems exist, since that is actually a fairly common situation. One place that happened was on the GitHub Actions runners used for Windows jobs. Those runners have WSL available, and are capable of running WSL 1 systems (not currently WSL 2 systems), but no WSL systems were actually installed. This commit fixes that cause of failure, for all four tests it happened in, by setting up a Debian WSL system on the test runner. (This increases the amount of time it takes Windows jobs to run, but that might be possible to improve on.) Three of those four tests now pass, while the other fails for another reason. The newly passing tests are: - test/test_index.py::TestIndex::test_commit_msg_hook_fail - test/test_index.py::TestIndex::test_pre_commit_hook_fail - test/test_index.py::TestIndex::test_pre_commit_hook_success The test that still fails, but differently, is: - test/test_index.py::TestIndex::test_commit_msg_hook_success I had previously found that test to fail on a local WSL 2 system, and attempted to add a suitable xfail marking in 881456b (gitpython-developers#1679). But the condition I wrote there *appears* to have a bug related to the different orders in which subproces.Popen and shutil.which find executables, causing it not always to detect when the WSL-related bash.exe is the one the Popen call in git.index.fun.run_commit_hook will use.
EliahKagan added a commit to EliahKagan/GitPython that referenced this pull request
Nov 25, 2023881456b (gitpython-developers#1679) expanded the use of shutil.which in test_index.py to attempt to mark test_commit_msg_hook_success xfail when bash.exe is a WSL bash wrapper in System32 (because that test currently is not passing when the hook is run via bash in a WSL system, which the WSL bash.exe wraps). But this was not reliable, due to significant differences between shell and non-shell search behavior for executable commands on Windows. As the new docstring notes, lookup due to Popen generally checks System32 before going through directories in PATH, as this is how CreateProcessW behaves. - https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessw - python/cpython#91558 (comment) The technique I had used in 881456b also had the shortcoming of assuming that a bash.exe in System32 was the WSL wrapper. But such a file may exist on some systems without WSL, and be a bash interpreter unrelated to WSL that may be able to run hooks. In addition, one common situation, which was the case on CI before a step to install a WSL distribution was added, is that WSL is present but no WSL distributions are installed. In that situation bash.exe is found in System32, but it can't be used to run any hooks, because there's no actual system with a bash in it to wrap. This was not covered before. Unlike some conditions that prevent a WSL system from being used, such as resource exhaustion blocking it from being started, the absence of a WSL system should probably not fail the pytest run, for the same reason as we are trying not to have the complete *absence* of bash.exe fail the pytest run. Both conditions should be xfail. Fortunately, the error message when no distribution exists is distinctive and can be checked for. There is probably no correct and reasonable way to check LBYL-style which executable file bash.exe resolves to by using shutil.which, due to shutil.which and subprocess.Popen's differing seach orders and other subtleties. So this adds code to do it EAFP-style using subprocess.run (which itself uses Popen, so giving the same CreateProcessW behavior). It tries to run a command with bash.exe whose output pretty reliably shows if the system is WSL or not. We may fail to get to the point of running that command at all, if bash.exe is not usable, in which case the failure's details tell us if bash.exe is absent (xfail), present as the WSL wrapper with no WSL systems (xfail), or has some other error (not xfail, so the user can become aware of and proably fix the problem). If we do get to that point, then a file that is almost always present on both WSL 1 and WSL 2 systems and almost always absent on any other system is checked for, to distinguish whether the working bash shell operates in a WSL system, or outside any such system as e.g. Git Bash does. See https://superuser.com/a/1749811 on various techniques for checking for WSL, including the /proc/sys/fs/binfmt_misc/WSLInterop technique used here (which seems overall may be the most reliable). Although the Windows CI runners have Git Bash, and this is even the bash.exe that appears first in PATH (giving rise to the problem with shutil.which detailed above), it would be a bit awkward to test the behavior when Git Bash is actually used to run hooks on CI, because of how Popen selects the System32 bash.exe first, whether or not any WSL distribution is present. However, local testing shows that when Git Bash's bash.exe is selected, all four hook tests in the module pass, both before and after this change, and furthermore that bash.exe is correctly detected as "native", continuing to avoid an erronous xfail mark in that case.
EliahKagan added a commit to EliahKagan/GitPython that referenced this pull request
Nov 25, 2023881456b (gitpython-developers#1679) expanded the use of shutil.which in test_index.py to attempt to mark test_commit_msg_hook_success xfail when bash.exe is a WSL bash wrapper in System32 (because that test currently is not passing when the hook is run via bash in a WSL system, which the WSL bash.exe wraps). But this was not reliable, due to significant differences between shell and non-shell search behavior for executable commands on Windows. As the new docstring notes, lookup due to Popen generally checks System32 before going through directories in PATH, as this is how CreateProcessW behaves. - https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessw - python/cpython#91558 (comment) The technique I had used in 881456b also had the shortcoming of assuming that a bash.exe in System32 was the WSL wrapper. But such a file may exist on some systems without WSL, and be a bash interpreter unrelated to WSL that may be able to run hooks. In addition, one common situation, which was the case on CI before a step to install a WSL distribution was added, is that WSL is present but no WSL distributions are installed. In that situation bash.exe is found in System32, but it can't be used to run any hooks, because there's no actual system with a bash in it to wrap. This was not covered before. Unlike some conditions that prevent a WSL system from being used, such as resource exhaustion blocking it from being started, the absence of a WSL system should probably not fail the pytest run, for the same reason as we are trying not to have the complete *absence* of bash.exe fail the pytest run. Both conditions should be xfail. Fortunately, the error message when no distribution exists is distinctive and can be checked for. There is probably no correct and reasonable way to check LBYL-style which executable file bash.exe resolves to by using shutil.which, due to shutil.which and subprocess.Popen's differing seach orders and other subtleties. So this adds code to do it EAFP-style using subprocess.run (which itself uses Popen, so giving the same CreateProcessW behavior). It tries to run a command with bash.exe whose output pretty reliably shows if the system is WSL or not. We may fail to get to the point of running that command at all, if bash.exe is not usable, in which case the failure's details tell us if bash.exe is absent (xfail), present as the WSL wrapper with no WSL systems (xfail), or has some other error (not xfail, so the user can become aware of and proably fix the problem). If we do get to that point, then a file that is almost always present on both WSL 1 and WSL 2 systems and almost always absent on any other system is checked for, to distinguish whether the working bash shell operates in a WSL system, or outside any such system as e.g. Git Bash does. See https://superuser.com/a/1749811 on various techniques for checking for WSL, including the /proc/sys/fs/binfmt_misc/WSLInterop technique used here (which seems overall may be the most reliable). Although the Windows CI runners have Git Bash, and this is even the bash.exe that appears first in PATH (giving rise to the problem with shutil.which detailed above), it would be a bit awkward to test the behavior when Git Bash is actually used to run hooks on CI, because of how Popen selects the System32 bash.exe first, whether or not any WSL distribution is present. However, local testing shows that when Git Bash's bash.exe is selected, all four hook tests in the module pass, both before and after this change, and furthermore that bash.exe is correctly detected as "native", continuing to avoid an erroneous xfail mark in that case.
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