Double `pytest.skip` leads to failure

[ego-thales]

Hello everyone.

I think the following is a bug.

@pytest.fixture
def fixA():
    yield
    pytest.skip(reason="A")

@pytest.fixture
def fixB():
    yield
    pytest.skip(reason="B")

def test_skip(fixA, fixB):  # FAILS
    assert True

Gives the following error report (removed tracebacks).

____________________________ ERROR at teardown of test_skip _____________________________
  + Exception Group Traceback (most recent call last):
  | <traceback>
  | BaseExceptionGroup: errors while tearing down <Function test_skip> (2 sub-exceptions)
  +-+---------------- 1 ----------------
    | <traceback>
    | Skipped: A
    +---------------- 2 ----------------
    | <traceback>
    | Skipped: B
    +------------------------------------

Maybe pytest checks for the error raised being of the type of skips, and thus here it bugs because it's an ExceptionGroup and not a skip type? I'm just very wildly guessing, I don't know the internals of pytest.

[The-Compiler]

In pytest 7.2.2 (or when removing fixB), this results in 1 passed, 1 skipped (which is somewhat confusing but another long-standing issue).

With pytest 7.3.0 and later, this errors in the same way you mention. Bisected to 3a68c08:

• Use exceptiongroup for multiple errors during teardown #10226

And I think your guess is spot-on, e.g. around here:

pytest/src/_pytest/runner.py

Lines 398 to 410 in b3ee80b

	skip_exceptions = [Skipped]
	unittest = sys.modules.get("unittest")
	if unittest is not None:
	skip_exceptions.append(unittest.SkipTest)
	if isinstance(call.excinfo.value, tuple(skip_exceptions)):
	outcome = "skipped"
	r_ = collector._repr_failure_py(call.excinfo, "line")
	assert isinstance(r_, ExceptionChainRepr), repr(r_)
	r = r_.reprcrash
	assert r
	longrepr = (str(r.path), r.lineno, r.message)
	else:
	outcome = "failed"

I suspect this might also happen with other outcome exceptions. Maybe we should immediately raise the first outcome exception as a special case rather than grouping those? Perhaps conditionally as long as there are no other non-outcome exceptions that happened?

cc @Zac-HD @jakkdl who did a lot of work around exception groups

[RonnyPfannschmidt]

We should consider exception group's with only skip as skip

[ego-thales]

Ok great!

Maybe we should immediately raise the first outcome exception as a special case rather than grouping those? Perhaps conditionally as long as there are no other non-outcome exceptions that happened?

I was thinking about this and I don't think the desired behaviour is trivial to define. For example, we might want to skip if there are only skips, but we might also want to allow fixtures to skip even if an inner fixture failed? Like a kind of "catching fixture". It seems far fetched, but I think there's potentially something to clarify here.

Also, we might want to skip at the first skip encountered, OR we might want to allow outermost fixture to still fail the test even if an inner fixture skipped it?

So as I see it, it's a discussion that needs to happen.

[RonnyPfannschmidt]

The groupings was added so exceptions no longer get lost

Now we need to account for that in places

[Zac-HD]

if isinstance(exc := call.excinfo.value, tuple(skip_exceptions)) or (
    isinstance(exc, BaseExceptionGroup) and exc.subgroup(skip_exceptions)
):

once we drop Python 3.10, the new except* syntax should also make it easier.