Skip to content

Test native Windows on CI #1745

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

Merged
merged 29 commits into from
Nov 29, 2023
Merged
Changes from 1 commit
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
2fd79f4
Add native Windows test jobs to CI matrix
EliahKagan Oct 15, 2023
6e477e3
Add xfail marks for IndexFile.from_tree failures
EliahKagan Nov 14, 2023
cd9d7a9
Mark test_clone_command_injection xfail on Windows
EliahKagan Nov 15, 2023
f72e282
Mark test_diff_submodule xfail on Windows
EliahKagan Nov 15, 2023
42a3d74
Mark TestSubmodule.test_rename xfail on Windows
EliahKagan Nov 15, 2023
4abab92
Mark test_conditional_includes_from_git_dir xfail on Windows
EliahKagan Nov 15, 2023
799c853
Improve ordering/grouping of a few imports
EliahKagan Nov 16, 2023
b284ad7
Mark test_create_remote_unsafe_url_allowed xfail on Windows
EliahKagan Nov 16, 2023
61d1fba
Mark unsafe-options "allowed" tests xfail on Windows
EliahKagan Nov 16, 2023
ad07ecb
Show PATH on CI
EliahKagan Nov 22, 2023
2784e40
Show bash and other WSL-relevant info but not PATH
EliahKagan Nov 22, 2023
9717b8d
Install WSL system on CI for hook tests
EliahKagan Nov 17, 2023
5d11394
Fix and expand bash.exe xfail marks on hook tests
EliahKagan Nov 23, 2023
b215357
Simplify/clarify bash.exe check for hook tests; do it only once
EliahKagan Nov 24, 2023
cabb572
Temporarily don't install WSL system to test xfail
EliahKagan Nov 24, 2023
2875ffa
Put back WSL on Windows CI; pare down debug info
EliahKagan Nov 24, 2023
0f8cd4c
Treat XPASS status as a test failure
EliahKagan Nov 24, 2023
82c361e
Correct TestSubmodule.test_rename xfail condition
EliahKagan Nov 24, 2023
0ae5dd1
Revert "Treat XPASS status as a test failure"
EliahKagan Nov 24, 2023
0b7ee17
Refine TestSubmodule.test_rename xfail condition
EliahKagan Nov 25, 2023
8621e89
Reword comment in _WinBashStatus.check for clarity
EliahKagan Nov 25, 2023
7ff3cee
Make _WinBashStatus instances carry all their info
EliahKagan Nov 25, 2023
d5ed266
Use bytes in bash.exe check; retest no-distro case
EliahKagan Nov 25, 2023
496acaa
Handle multiple encodings for WSL error messages
EliahKagan Nov 26, 2023
d779a75
Don't assume WSL-related bash.exe error is English
EliahKagan Nov 27, 2023
9ac2438
Handle encodings better; make the sum type "public"
EliahKagan Nov 27, 2023
b07e5c7
Put back WSL on Windows CI
EliahKagan Nov 28, 2023
3303c74
Improve readability of WinBashStatus class
EliahKagan Nov 28, 2023
e00fffc
Shorten comments on _decode steps
EliahKagan Nov 28, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Simplify/clarify bash.exe check for hook tests; do it only once
- Make the design of the enum simpler.
- Move some informaton to the check method's docstring.
- Call the method once instead of in each decoration.

Doing the check only once makes things a little faster, but the
more important reason is that the checks can become stale very
quickly in some situations. This is unlikely to be an issue on CI,
but locally WSL may fail (or not fail) to start up when bash.exe
is used in a check, then not fail (or fail) when actaully needed
in the test, if the reason it fails is due to resource usage, such
as most RAM being used on the system.

Although this seems like a reason to do the check multiple times,
doing it multiple times in the decorations still does it ahead of
when any of the tests is actually run. In contrast, with the change
here, we may get outdated results by the time the tests run, but
the xfail effects of the check are always consistent with each
other and are no longer written in a way that suggests they are
more current than they really are.
  • Loading branch information
EliahKagan committed Nov 25, 2023
commit b21535729ca695e47c52086abe390ca4e6792ae3
73 changes: 38 additions & 35 deletions test/test_index.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,32 +39,11 @@
log = logging.getLogger(__name__)


class _WinBashMeta(enum.EnumMeta):
"""Metaclass allowing :class:`_WinBash` custom behavior when called."""

def __call__(cls):
return cls._check()


@enum.unique
class _WinBash(enum.Enum, metaclass=_WinBashMeta):
class _WinBashStatus(enum.Enum):
"""Status of bash.exe for native Windows. Affects which commit hook tests can pass.

Call ``_WinBash()`` to check the status.

This can't be reliably discovered using :func:`shutil.which`, as that approximates
how a shell is expected to search for an executable. On Windows, there are major
differences between how executables are found by a shell and otherwise. (This is the
cmd.exe Windows shell and should not be confused with bash.exe.) Our run_commit_hook
function in GitPython uses subprocess.Popen, including to run bash.exe on Windows.
It does not pass shell=True (and should not). Popen calls CreateProcessW, which
searches several locations prior to using the PATH environment variable. It is
expected to search the System32 directory, even if another directory containing the
executable precedes it in PATH. (There are other differences, less relevant here.)
When WSL is installed, even if no WSL *systems* are installed, bash.exe exists in
System32, and Popen finds it even if another bash.exe precedes it in PATH, as
happens on CI. If WSL is absent, System32 may still have bash.exe, as Windows users
and administrators occasionally copy executables there in lieu of extending PATH.
Call :meth:`check` to check the status.
"""

INAPPLICABLE = enum.auto()
Expand All @@ -74,13 +53,13 @@ class _WinBash(enum.Enum, metaclass=_WinBashMeta):
"""No command for ``bash.exe`` is found on the system."""

NATIVE = enum.auto()
"""Running ``bash.exe`` operates outside any WSL environment (as with Git Bash)."""
"""Running ``bash.exe`` operates outside any WSL distribution (as with Git Bash)."""

WSL = enum.auto()
"""Running ``bash.exe`` runs bash on a WSL system."""
"""Running ``bash.exe`` calls ``bash`` in a WSL distribution."""

WSL_NO_DISTRO = enum.auto()
"""Running ``bash.exe` tries to run bash on a WSL system, but none exists."""
"""Running ``bash.exe` tries to run bash on a WSL distribution, but none exists."""

ERROR_WHILE_CHECKING = enum.auto()
"""Could not determine the status.
Expand All @@ -92,13 +71,34 @@ class _WinBash(enum.Enum, metaclass=_WinBashMeta):
"""

@classmethod
def _check(cls):
def check(cls):
"""Check the status of the ``bash.exe`` :func:`index.fun.run_commit_hook` uses.

This uses EAFP, attempting to run a command via ``bash.exe``. Which ``bash.exe``
is used can't be reliably discovered by :func:`shutil.which`, which approximates
how a shell is expected to search for an executable. On Windows, there are major
differences between how executables are found by a shell and otherwise. (This is
the cmd.exe Windows shell, and shouldn't be confused with bash.exe itself. That
the command being looked up also happens to be an interpreter is not relevant.)

:func:`index.fun.run_commit_hook` uses :class:`subprocess.Popen`, including when
it runs ``bash.exe`` on Windows. It doesn't pass ``shell=True`` (and shouldn't).
On Windows, `Popen` calls ``CreateProcessW``, which searches several locations
prior to using the ``PATH`` environment variable. It is expected to search the
``System32`` directory, even if another directory containing the executable
precedes it in ``PATH``. (Other differences are less relevant here.) When WSL is
installed, even with no distributions, ``bash.exe`` exists in ``System32``, and
`Popen` finds it even if another ``bash.exe`` precedes it in ``PATH``, as on CI.
If WSL is absent, ``System32`` may still have ``bash.exe``, as Windows users and
administrators occasionally put executables there in lieu of extending ``PATH``.
"""
if os.name != "nt":
return cls.INAPPLICABLE

try:
# Print rather than returning the test command's exit status so that if a
# failure occurs before we even get to this point, we will detect it.
# failure occurs before we even get to this point, we will detect it. See
# https://superuser.com/a/1749811 for information on ways to check for WSL.
script = 'test -e /proc/sys/fs/binfmt_misc/WSLInterop; echo "$?"'
command = ["bash.exe", "-c", script]
proc = subprocess.run(command, capture_output=True, check=True, text=True)
Expand Down Expand Up @@ -129,6 +129,9 @@ def _error(cls, error_or_process):
return cls.ERROR_WHILE_CHECKING


_win_bash_status = _WinBashStatus.check()


def _make_hook(git_dir, name, content, make_exec=True):
"""A helper to create a hook"""
hp = hook_path(name, git_dir)
Expand Down Expand Up @@ -998,7 +1001,7 @@ class Mocked:
self.assertEqual(rel, os.path.relpath(path, root))

@pytest.mark.xfail(
_WinBash() is _WinBash.WSL_NO_DISTRO,
_win_bash_status is _WinBashStatus.WSL_NO_DISTRO,
reason="Currently uses the bash.exe for WSL even with no WSL distro installed",
raises=HookExecutionError,
)
Expand All @@ -1009,7 +1012,7 @@ def test_pre_commit_hook_success(self, rw_repo):
index.commit("This should not fail")

@pytest.mark.xfail(
_WinBash() is _WinBash.WSL_NO_DISTRO,
_win_bash_status is _WinBashStatus.WSL_NO_DISTRO,
reason="Currently uses the bash.exe for WSL even with no WSL distro installed",
raises=AssertionError,
)
Expand All @@ -1020,7 +1023,7 @@ def test_pre_commit_hook_fail(self, rw_repo):
try:
index.commit("This should fail")
except HookExecutionError as err:
if _WinBash() is _WinBash.ABSENT:
if _win_bash_status is _WinBashStatus.ABSENT:
self.assertIsInstance(err.status, OSError)
self.assertEqual(err.command, [hp])
self.assertEqual(err.stdout, "")
Expand All @@ -1036,12 +1039,12 @@ def test_pre_commit_hook_fail(self, rw_repo):
raise AssertionError("Should have caught a HookExecutionError")

@pytest.mark.xfail(
_WinBash() in {_WinBash.ABSENT, _WinBash.WSL},
_win_bash_status in {_WinBashStatus.ABSENT, _WinBashStatus.WSL},
reason="Specifically seems to fail on WSL bash (in spite of #1399)",
raises=AssertionError,
)
@pytest.mark.xfail(
_WinBash() is _WinBash.WSL_NO_DISTRO,
_win_bash_status is _WinBashStatus.WSL_NO_DISTRO,
reason="Currently uses the bash.exe for WSL even with no WSL distro installed",
raises=HookExecutionError,
)
Expand All @@ -1059,7 +1062,7 @@ def test_commit_msg_hook_success(self, rw_repo):
self.assertEqual(new_commit.message, "{} {}".format(commit_message, from_hook_message))

@pytest.mark.xfail(
_WinBash() is _WinBash.WSL_NO_DISTRO,
_win_bash_status is _WinBashStatus.WSL_NO_DISTRO,
reason="Currently uses the bash.exe for WSL even with no WSL distro installed",
raises=AssertionError,
)
Expand All @@ -1070,7 +1073,7 @@ def test_commit_msg_hook_fail(self, rw_repo):
try:
index.commit("This should fail")
except HookExecutionError as err:
if _WinBash() is _WinBash.ABSENT:
if _win_bash_status is _WinBashStatus.ABSENT:
self.assertIsInstance(err.status, OSError)
self.assertEqual(err.command, [hp])
self.assertEqual(err.stdout, "")
Expand Down