René's URL Explorer Experiment

{"@context":"https://schema.org","@type":"DiscussionForumPosting","headline":"Overbroad xfail marks will eventually make CI fail","articleBody":"### Background\r\n\r\n#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`](https://docs.pytest.org/en/7.1.x/how-to/skipping.html#xfail-mark-test-functions-as-expected-to-fail) to too many tests, due to limitations on granularity when applying [`pytest` marks](https://docs.pytest.org/en/latest/how-to/mark.html) to `unittest` test cases generated by `@ddt` parameterization.\r\n\r\nhttps://github.com/gitpython-developers/GitPython/blob/340da6d39397253da8d0807179b0ecb5952effca/test/test_util.py#L221-L228\r\n\r\nhttps://github.com/gitpython-developers/GitPython/blob/340da6d39397253da8d0807179b0ecb5952effca/test/test_util.py#L233-L245\r\n\r\n### Upcoming impact\r\n\r\nAlthough 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.\r\n\r\n Specifically, and in the absence of configuration or command-line options to `pytest` that override the behavior:\r\n\r\n- 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.\r\n- A test marked `xfail` that fails in a detectably unexpected way--where a different exception results than the one that was [expected](https://docs.pytest.org/en/7.1.x/how-to/skipping.html#raises-parameter)--produces a FAIL status. We always want this.\r\n- 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](https://docs.pytest.org/en/7.1.x/how-to/skipping.html#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.\r\n\r\nAs noted in https://github.com/pytest-dev/pytest/issues/11467, which was opened by a pytest maintainer and is listed for pytest's [8.0 milestone](https://github.com/pytest-dev/pytest/milestone/35), the default is planned to be changed from `False` to `True` starting in pytest 8.0. (See also https://github.com/pytest-dev/pytest/pull/11499.)\r\n\r\n### Possible fixes\r\n\r\nBreakage could be avoided (at least for a while, since `strict=False` [may eventually](https://github.com/pytest-dev/pytest/issues/11467#issuecomment-1733852526) 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 `\u003c8` for `pytest` in `test-requirements.txt`.\r\n\r\nHowever, 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.\r\n\r\nThat approach – which I mentioned in #1679 itself and in https://github.com/gitpython-developers/GitPython/pull/1700#discussion_r1353654395, and more recently alluded to in #1725 and https://github.com/gitpython-developers/GitPython/pull/1726#issuecomment-1791541248 – has the following advantages over other approaches that effectively just suppress the problem:\r\n\r\n- Any XPASS will be a sign that something has changed and should be looked into, thereby building on the improvements in [#1679](https://github.com/gitpython-developers/GitPython/pull/1679).\r\n- Although we have FIXME comments, the current situation is still misleading in the test results themselves, which indicate that some tests are unexpectedly passing.\r\n- 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.\r\n- 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](https://github.com/gitpython-developers/GitPython/pull/1700) toward an eventual complete fix for [#790](https://github.com/gitpython-developers/GitPython/issues/790). (In addition, I want to make sure the [planned](https://github.com/gitpython-developers/GitPython/pull/1654#issuecomment-1717239735) 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.)\r\n\r\nI have opened #1729, which fixes this issue by reorganizing tests in `test_util.py` in this way.","author":{"url":"https://github.com/EliahKagan","@type":"Person","name":"EliahKagan"},"datePublished":"2023-11-03T15:20:03.000Z","interactionStatistic":{"@type":"InteractionCounter","interactionType":"https://schema.org/CommentAction","userInteractionCount":7},"url":"https://github.com/1728/GitPython/issues/1728"}