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

Some xfail markings fail to validate their exception types #1893

Closed
EliahKagan opened this issue Apr 1, 2024 · 1 comment
Closed

Some xfail markings fail to validate their exception types #1893

EliahKagan opened this issue Apr 1, 2024 · 1 comment

Comments

@EliahKagan
Copy link
Contributor

This bug was discovered by @Borda in #1888. Because that PR's central focus is tooling to catch typos, I'm creating this issue so that this bug in the test suite is clear, and so it will not be forgotten regardless of how development proceeds there. This affects three test cases in test_index.py.

In #1777, I introduced test_run_commit_hook. This is expected to fail in a couple of situations where it should nonetheless be feasible to run the test suite. One of these is when the tests are run on a (native) Windows system that has no bash that can be found in a path search, neither in System32 or otherwise and neither WSL-related or otherwise. The test included an xfail marking to cover that case:

GitPython/test/test_index.py

Lines 1018 to 1022 in 5364053

@pytest.mark.xfail(
type(_win_bash_status) is WinBashStatus.Absent,
reason="Can't run a hook on Windows without bash.exe.",
rasies=HookExecutionError,
)

In that PR I also added those xfail markings to other tests to which they applied, and I used copy and paste to reproduce the parts that should be exactly the same across them.

But that contains a typo, passing a rasies argument when constructing the mark, rather than the correct raises argument. The wrong argument is apparently ignored, so the marking is unconstrained, and even if those tests fail with a different exception -- one that is not covered by other xfail markings, and that is not intended to be covered by the ones with the misspelling, including AssertionError from an assert statement in the test -- the problem would not be caught.

This is distinct from the more common problem of misspelling a mark, which is also not an error, but which pytest does have the ability to warn about.

Fortunately, the way those tests fail is as the xfail markings attempt to describe and will actually describe once fixed. I have retested the cases not covered on CI, and they work with the changes to test_index.py appearing in a20d5db (#1888). I only checked out the changes to test_index.py in testing this, not the other files. In a virtual machine running Windows Server 2022 (version 10.0.20348), I have just tested the cases where:

  • bash.exe exists, is the WSL-related command in System32, but there is no WSL system installed.
  • bash.exe exists, is not the WSL-related command in System32 but is instead Git Bash, and is working.
  • bash.exe does not exist anywhere found in a Popen call, not even in System32.

The latter case I believe still occurs on some Windows systems that nothing unusual has been done to, but is rare because usually a WSL-related bash.exe appears in System32 even if WSL is not installed, and (non-shell) path search on Windows generally includes System32 automatically, and even places it before PATH directories even if it appears late or not at all in PATH.

(I retested it the same way I had done originally: by saving a snapshot to restore, then using NanaRun MinSudo to run a shell as the special TrustedInstaller user, which is permitted to delete even specially protected files in System32. I suggest against doing such a thing outside a virtual machine; I snapshotted the VM first and restored it after the test.)

@EliahKagan
Copy link
Contributor Author

This was fixed in #1888.

EliahKagan added a commit to EliahKagan/GitPython that referenced this issue Jul 24, 2024
Some of the CI tests use WSL. This switches the WSL distribution
from Debian to Alpine, which might be slightly faster. For the way
it is being used here, the main expected speed improvement would be
to how long the image would take to download, as Alpine is smaller.

(The reason for this is thus unrelated to the reason for the Alpine
docker CI test job added in gitpython-developers#1826. There, the goal was to test on a
wider variety of systems and environments, and that runs the whole
test suite in Alpine. This just changes the WSL distro, used by a
few tests on Windows, from Debian to Alpine.)

Two things have changed that, taken together, have unblocked this:

- Vampire/setup-wsl#50 was fixed, so the
  action we are using is able to install Alpine Linux. See:
  gitpython-developers#1917 (review)

- gitpython-developers#1893 was fixed in gitpython-developers#1888. So if switching the WSL distro from
  Debian to Alpine breaks any tests, including by making them fail
  in an unexpected way that raises the wrong exception, we are
  likely to find out.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants