Overbroad xfail marks will eventually make CI fail

[EliahKagan]

Background

#1679 included improvements to a number of tests that are known to fail on some platforms, by marking them xfail instead of skip so they are still run and their status is reported, but without a failing status causing the whole test run to fail. However, it applied xfail to too many tests, due to limitations on granularity when applying pytest marks to unittest test cases generated by @ddt parameterization.

GitPython/test/test_util.py

Lines 221 to 228 in 340da6d

	# FIXME: Mark only the /proc-prefixing cases xfail, somehow (or fix them).
	@pytest.mark.xfail(
	reason="Many return paths prefixed /proc/cygdrive instead.",
	raises=AssertionError,
	)
	@skipUnless(sys.platform == "cygwin", "Paths specifically for Cygwin.")
	@ddt.idata(_norm_cygpath_pairs + _unc_cygpath_pairs)
	def test_cygpath_ok(self, case):

GitPython/test/test_util.py

Lines 233 to 245 in 340da6d

	@pytest.mark.xfail(
	reason=R'2nd example r".\bar" -> "bar" fails, returns "./bar"',
	raises=AssertionError,
	)
	@skipUnless(sys.platform == "cygwin", "Paths specifically for Cygwin.")
	@ddt.data(
	(R"./bar", "bar"),
	(R".\bar", "bar"), # FIXME: Mark only this one xfail, somehow (or fix it).
	(R"../bar", "../bar"),
	(R"..\bar", "../bar"),
	(R"../bar/.\foo/../chu", "../bar/chu"),
	)
	def test_cygpath_norm_ok(self, case):

Upcoming impact

Although this was known and discussed in #1679, and FIXME comments about it were included in the code, the problem turns out to be somewhat more serious than I had anticipated: if not addressed, it will eventually lead to test failures in a future version of pytest. This is because the default behavior of an unexpectedly passing test--one that is marked xfail but passes--will most likely change in pytest 8. Because GitPython does not specify upper bounds on most of its development dependencies, and pytest is one of the development dependencies for which no upper bound is specified, pytest 8 will be automatically installed once it is (stably) released.

Specifically, and in the absence of configuration or command-line options to pytest that override the behavior:

• A test marked xfail that fails, and fails in the expected way, produces an XFAIL status, which is treated similarly to PASS. We always want this.
• A test marked xfail that fails in a detectably unexpected way--where a different exception results than the one that was expected--produces a FAIL status. We always want this.
• A test marked xfail that passes produces an XPASS status. How this status is treated is more complicated. The xfail mark supports an optional strict parameter. Where present, it determines whether the XPASS fails the test run like a FAIL status would, or does not fail the test run (thus behaving like PASS or XFAIL). If absent, the xfail_strict configuration option provides the default. Currently, as of pytest 7, xfail_strict defaults to False when not specified.

As noted in pytest-dev/pytest#11467, which was opened by a pytest maintainer and is listed for pytest's 8.0 milestone, the default is planned to be changed from False to True starting in pytest 8.0. (See also pytest-dev/pytest#11499.)

Possible fixes

Breakage could be avoided (at least for a while, since strict=False may eventually be removed as a feature) by passing strict=False or setting xfail_strict=false for pytest in pyproject.toml. It is also possible to set an upper bound like <8 for pytest in test-requirements.txt.

However, I recommend this instead be fixed by reorganizing the tests in test_util.py so that the tests of cygpath and decygpath--which are the ones that have the insufficiently precise xfail markings that mark some generated test cases xfail even though they are known to pass--can be pure pytest tests. Because they are currently unittest tests, they cannot be generated by @pytest.mark.parametrize (hence @ddt is used). But if they could be generated with the parametrize mark then they could have per-case markings, because parametrize supports an optional marks argument. They could then have the xfail mark applied to exactly the cases where failure is really expected.

That approach – which I mentioned in #1679 itself and in #1700 (comment), and more recently alluded to in #1725 and #1726 (comment) – has the following advantages over other approaches that effectively just suppress the problem:

• Any XPASS will be a sign that something has changed and should be looked into, thereby building on the improvements in #1679.
• Although we have FIXME comments, the current situation is still misleading in the test results themselves, which indicate that some tests are unexpectedly passing.
• When the default treatment of XPASS in pytest changes--but also even before that, once it is documented to change--the presence of expected XPASSes will be more misleading than it is already, even if GitPython is not using a version of pytest affected by the change. This is because that change will further solidify people's expectations about what XPASS indicates, including for people who are trying to become familiar with GitPython.
• Reorganizing the tests in test_util.py can also help clarify the tests of rmtree behavior, and help make them easier to modify. This is useful because it will allow building on #1700 toward an eventual complete fix for #790. (In addition, I want to make sure the planned native Windows CI jobs don't have the effect of calcifying cleanup logic in rmtree that otherwise could or should change, or at least that this does not happen in ways that impinge on non-Windows platforms. I think such a reorganization will help with that, too.)

I have opened #1729, which fixes this issue by reorganizing tests in test_util.py in this way.

[Byron]

I am sorry not to have more feedback, but this part of the tests is way over my head 😅.

[EliahKagan]

Did it become so as a result of this change? If so, then probably a further change should be made to add comments, docstrings, something in doc/, or to rework something to be clearer. Alternatively, if it became so due to this change but for a pytest-related reason, then I may be able to provide information about that, and then whether any further change should be made to clarify it could be determined. On the other hand, if it was already fully that way and did not become so as a result of this change, then maybe none of that has to be done but I could continually look for opportunities to make the test code clearer next time I work on it.

[Byron]

I think it's that many of these tests generally are containing multiple steps and thus take some state to keep track of, which makes them harder to follow. But in this case I think the issue was that the change was so broad that the diff didn't actually help this time around. It would have been better for me to look at it in an editor, I then again, I do trust that you leave the place better than you found it and overall, the complexity of the code is lower now. So, all good, it's me, I am having one of these days as well I guess, in a good way, yet, I am more fuzzy than usual.

[EliahKagan]

I think it's that many of these tests generally are containing multiple steps and thus take some state to keep track of, which makes them harder to follow.

Yes, many tests are long and test many things, and one of my worries about the situation with native Windows tests is that, in some cases, when a test fails, it never tests many of the numerous other things it was written to test. Changing skip to xfail markings runs more tests and tracks information about known-failing tests' first failures, but many tests in GitPython's test suite will, in effect, skip most of themselves on the first failure, no matter how they are run. Sometimes this is arguably valuable, in that, after some failures, the subsequent checks don't make much sense. But often it would not have to be done this way.

But in this case I think the issue was that the change was so broad that the diff didn't actually help this time around.

When I looked over #1729 in the GitHub interface (this was just a brief look before I marked the PR ready for review), I found the default "Unified" view, which is often useful, to be incomprehensible, but the "Split" view usable. However, even for the "Split" view, it may be that my knowledge of the changes--having myself made them, and recently--was a big factor.

It would have been better for me to look at it in an editor

The web editor, which can be opened for a PR by pressing . from the PR page, can sometimes be a convenient way to get a richer review experience without leaving the browser and without needing to be on a machine with an editor or other development tools. I'm not sure if that's one of the tools you sometimes use, but if so, then in this specific case, it might've contributed to making your experience worse, because GitHub suffered an outage around the time #1729 was approved (showing 404 pages for repositories, and with some interruption to accessing GitHub-hosted remotes and various other services). Usually, though, I think it works fairly well... though whether or not one likes it is a separate question.

then again, I do trust that you leave the place better than you found it [...]

Thanks! Of course, it is quite possible for me to make mistakes. It is also possible for me to introduce intermediate states of improvement without full insight into whether they are worth merging at that time. For example, #1661 was greatly improved due to #1661 (review). The area where I think my pull requests might carry the greatest risk of making GitPython worse is when they embody consequential design decisions that might be wrong (not applicable in #1729, which is fairly narrow in its effects and affects only the test suite).

So, all good, it's me, I am having one of these days as well I guess [...]

No problem!

[Byron]

The web editor, which can be opened for a PR by pressing . from the PR page, can sometimes be a convenient way to get a richer review experience without leaving the browser and without needing to be on a machine with an editor or other development tools.

🙀 That's amazing! How could I not know that? Thanks so much, I will definitely use that more. I actually use VSCode for GitPython, mainly because getting some level of code-intelligence is easy and it has a usable vim mode as well.

The area where I think my pull requests might carry the greatest risk of making GitPython worse is when they embody consequential design decisions that might be wrong (not applicable in #1729, which is fairly narrow in its effects and affects only the test suite).

It's true, the test-suite is the only place where changes can even be made, and there I think it's just about hitting a spot where it's idiomatic and modern, maybe along with change to the away the tests actually work, i.e. make them more focussed where possible and properly isolated. An no matter what, I don't think it can ever be worse :D.