What should raises and RaisesGroup raise?

[jakkdl]

The current behavior on the released pytest version is:

1. if match fails, pytest.raises raises an AssertionError
2. if the type differs, the exception propagates through.
3. if no exception was raised, pytest.raises raises pytest.Failed

#13192 complicated this for several reasons, and having RaisesGroup adhere to point 2 was untenable. It became non-trivial to figure out why the "type" of an exceptiongroup with several nested exceptions and groups couldn't get matched against another group. It also became very weird when the "type" started to include matches itself, and it would've necessitated extra logic to figure out if RaisesGroup(RaisesExc(ValueError, match="foo")) failed because of the type or the match in RaisesExc. Or in even weirder cases where it fails for both reasons:

with RaisesGroup(RaisesExc(ValueError), RaisesExc(match="foo")):
  raise ExceptionGroup(TypeError("bar"), TypeError("bar"))

So RaisesGroup will raise an exception in all cases.

We're then faced with how this should affect pytest.raises. #13192 made it raise an AssertionError upon type difference (and if check fails), but this would likely be considered a breaking change. I personally think it's very different if a test fails because I got a completely unexpected exception, versus a different exception than the one I specifically expected in a raises block; but I don't think that's a huge win.
The other downside of keeping current behavior is it becoming an arbitrary-but-for-historic-reasons difference between raises and RaisesGroup.
In either case, the original exception is accessible in the __cause__ attribute and adapting code should be straightforward.

I do also find the distinction between raising pytest.Fail versus AssertionError quite arbitrary, and think they should be unified. My instinct would be for all failures to be pytest.Fail, as AssertionError usually signals to users that an explicit assert has failed. We could make this change backward-compatible by having the exception raised inherit from both AssertionError and pytest.Fail if we really want to.

This also matters for ExceptionInfo.match, which ideally should share logic with raises for string matching - but currently doesn't. It currently raises an AssertionError on failure.

tl;dr

1. Should raises propagate the exception, or raise AssertionError, or Fail?
2. Is the distinction between raising different exception types useful, or confusing?
3. Should RaisesGroup raise pytest.Fail or AssertionError when failing a match, regardless of reason?

[bluetech]

assert vs. fail

First on AssertionError vs Failed. Ideally we treat this as an implementation detail, i.e. we don't specify what we raise, only that the test fails. I'd say pytest.fail is just a little nicer assert False, <message>, so makes sense for the DID NOT RAISE to use that, while match is better with an assert, since there's a condition.

(To be more precise, there is a difference - we have pytest_assertrepr_compare and pytest_assertion_pass hooks for asserts. But these don't apply to match because the relevant file doesn't undergo assert rewriting.)

That said, there is something to be said for consistency here. If we could make it consistent I would like it just because consistency is good. But both options are a bit problematic:

• ExceptionInfo.match which is the public API underlying raises(match=...) explicitly documents raises AssertionError. So changing that would be breaking.
• On the other hand, switching DID NOT RAISE to AssertionError feels less nice than fail, will probably also break some stuff, and then there's also raises.Exception which is Failed.

I don't like the "inherit from both AssertionError and pytest.Fail" solution, feels too icky. But maybe?

My conclusion is to leave it...

Propagate vs. fail-fast

Let me try to weigh which behavior is better.

For propagate:

• If thinking of with pytest.raises(MyExc) as analogous to try .. except MyExec then propagate is expected.
• It enables stuff like nesting with pytest.raises within a with pytest.raises. It might sound unrealistic, but I can imagine it happening when a pytest.raises is in some helper function, which a test calls but then also has a pytest.raises for some other unrelated exception.
• When the exception propagates and fails the test, hooks get a hold of it and can run specific logic on it. With fail-fast they'll need to know to unwrap it which they probably won't.
• Entrenched behavior

For fail-fast:

• Can show a nice message
• Distinguishes between "didn't expect this exception" and "didn't expect any exception".
• Consistent with match/check/DID NOT RAISE failure behavior
• Consistent with RaisesGroup (unless it's changed to propagate as well, which per your explanation is bad ux)

Overall, as far as I considered it, at least for raise I prefer propagate.

I haven't thought about RaisesGroup. I assume it's indeed very difficult to have it propagate but still have a decent error message?

[jakkdl]

assert vs fail

On the other hand, switching DID NOT RAISE to AssertionError feels less nice than fail, will probably also break some stuff, and then there's also raises.Exception which is Failed.

I agree it feels less nice, and personally when I get an AssertionError I assume it's gonna be an assert statement that I've myself written. I think it's non-obvious at a glance that raises and ExceptionInfo.match has assert statements inside them.
But for raises.Exception we can just ... set that to a different value, that's what its for.

the case for a new exception

_pytest.outcomes.Failed is not public API, so users shouldn't be catching that - they should be catching pytest.raises.Exception. (They could be catching pytest.fail.Exception though). This means that a new exception type wouldn't really need to inherit from it - we just update pytest.raises.Exception. If we introduce a new RaisesFailure we get:

• it can inherit from AssertionError, so we keep backwards compatibility
• we can make this the exception raised on all failures, except raises propagation
• it's more obvious that the error isn't from a visible assert statement
• for ExceptionInfo.match we can give that a different error with a little bit of logic. We could also do a MatchFailure that handles both it and raises, but that feels less fitting for the DID NOT RAISE case.

Propagate vs. fail-fast

If thinking of with pytest.raises(MyExc) as analogous to try .. except MyExc then propagate is expected.

though there is an alternate way of conceptualizing it:

try:
  ...
except BaseException as exc:
  assert isinstance(exc, MyExc)

It enables stuff like nesting with pytest.raises within a with pytest.raises. It might sound unrealistic, but I can imagine it happening when a pytest.raises is in some helper function, which a test calls but then also has a pytest.raises for some other unrelated exception.

No yeah this is probably a thing in the wild. Though it also feels a bit footgunny:

with pytest.raises(TypeError):
    # ensure this block raises ValueError
    with pytest.raises(ValueError):
        if random.randrange(1000) > 0:
            raise ValueError
        else:
            # oops sometimes this raises a TypeError, we didn't know that
            raise TypeError
    # this will always raise TypeError
    raise TypeError

but they'd be getting linter warnings about not using match, and raises being across a wide scope, so probably a rare scenario.

When the exception propagates and fails the test, hooks get a hold of it and can run specific logic on it. With fail-fast they'll need to know to unwrap it which they probably won't.

This is a good point, esp for stuff like KeyboardInterrupt. The way they would do it would be checking for pytest.raises.Exception, but nobody will be doing that currently.

Overall, as far as I considered it, at least for raise I prefer propagate.

Yeah I guess fail-fast for raises would simple break too much stuff, and the gain isn't terribly big. The only way I see of transitioning would be a flag/config option, but kinda unclear if worth the effort.

RaisesGroup

I haven't thought about RaisesGroup. I assume it's indeed very difficult to have it propagate but still have a decent error message?

Yeah I mean I just don't know where else to put it. I could of course just dump the error to stderr, maybe there's some pytest-internal way of saying "please output this text string if/when the test ultimately fails"

But that loops back to the problem of deciding if it is a type-style failure vs a match-style failure. I think the only way of making it comprehensible to users is to always fail in the same way in both cases, since it can fail for both reasons at once, and RaisesGroup(RaisesExc(match="foo")) is ambiguous which one it is, etc.

Though with RaisesGroup being nigh-impossible to adhere to propagate, the points about hooks and helpers will already be a problem (except more niche)....

BUTWAIT there's a third option of

def __exit__(self, exc_type, exc_val, exc_tb):
  if not_match:
    raise ExceptionGroup("...", [RaisesFailure("<reason why match failed>"), exc_val])

which would allow except* in hooks/helpers/etc to handle the propagated errors. Nesting RaisesGroup would be kinda madness to do here, but maybe somebody would try.

But we would need to improve the display of exceptiongroups in order to do this, the current state of play is not great (#10209 and #12975 did kinda hacky solutions, also see #12255)

[Zac-HD]

Probably at this point any change to pytest.raises() will break someone, and the net result will be that they don't update Pytest. The current behavior isn't ideal, but it's not that bad, so I'd leave it as-is.

"_pytest.outcomes.Failed is not public API, so users shouldn't be catching that" -> unfortunately we have very many creative users, downstream libraries hoping to remain compatible with older versions of pytest, etc. Hyrum's Law again.

I'd just decide on the cleanest behavior for RaisesGroup, implement and document that with a note that raises() differs a bit for historical reasons and due to the complexity of groups, and be done.