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:55
This 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.
Don't stop after the first 10.
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.
+ Reorder pytest arguments so both workflows are consistent.
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.
As it seems to be working now on Cygwin (maybe not native Windows).
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.
Rather than skipping, so it becomes known if the situation changes.
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.
The current cause of failure is different from what is documented
in the skip reason.
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.
This stops skipping them, as they are now working.
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.
+ Style tweak and comment to clarify the "Limit $PATH" step.
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 makes the two CI test workflows more similar in a couple of
the remaining ways they differ unnecessarily.

This could be extended, and otherwise improved upon, in the future.

renovate bot referenced this pull request in allenporter/flux-local

Oct 20, 2023

This was referenced

Nov 3, 2023

EliahKagan added a commit to EliahKagan/GitPython that referenced this pull request

Nov 20, 2023
This 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, 2023
This 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, 2023
This 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, 2023
This 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, 2023
This 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, 2023
This 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, 2023
881456b (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, 2023
881456b (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.