pytest.raises could be improved to match exception fields instead / in addition to regex

[ghost]

Suppose you have an exception which carries along information like status + message, or errno + message. Something as trivial as system call or an HTTP request. Trying to match on the message may not be very precise, if, say, HTTP server sends the same message with all 500+ status responses.

Thus, I suggest extending python_api.RaisesContext with matcher key, which would take user function, which, in turn, should expect an exception and return True/False to be later used as suppress_exception.

[pytestbot]

GitMate.io thinks possibly related issues are #467 (Cache fixtures which raise pytest.skip.Exception and pytest.fail.Exception), #767 (pytest.raises() doesn't always return Exception instance in py26), #692 (xfail decorator ignores "raises" parameter when given as single exception (instead of list)), #475 (Misleading pytest exception description), and #150 (improve pylint / pytest compatibility).

[nicoddemus]

Hi @wvxvw, thanks for writing.

Could you please provide an example of how a test using this feature would look like?

[ghost]

I actually did something similar to what I'm suggesting:

import py
import sys
import logging

from _pytest.python_api import RaisesContext
from _pytest.compat import isclass
from _pytest.outcomes import fail
import _pytest._code


def raises(expected_exception, *args, **kwargs):
    '''
    See ``pytest.raises()`` for docs and examples.
    '''
    __tracebackhide__ = True
    msg = ('exceptions must be old-style classes or'
           ' derived from BaseException, not %s')
    if isinstance(expected_exception, tuple):
        for exc in expected_exception:
            if not isclass(exc):
                raise TypeError(msg % type(exc))
    elif not isclass(expected_exception):
        raise TypeError(msg % type(expected_exception))

    message = 'DID NOT RAISE {0}'.format(expected_exception)
    match_expr = None

    if not args:
        if 'message' in kwargs:
            message = kwargs.pop('message')
        if 'match' in kwargs:
            match_expr = kwargs.pop('match')
        return MatcherRaisesContext(
            expected_exception,
            message,
            match_expr,
            kwargs,
        )
    elif isinstance(args[0], str):
        code, = args
        assert isinstance(code, str)
        frame = sys._getframe(1)
        loc = frame.f_locals.copy()
        loc.update(kwargs)
        # print 'raises frame scope: %r' % frame.f_locals
        try:
            code = _pytest._code.Source(code).compile()
            py.builtin.exec_(code, frame.f_globals, loc)
            # XXX didn'T mean f_globals == f_locals something special?
            #     this is destroyed here ...
        except expected_exception:
            return _pytest._code.ExceptionInfo()
    else:
        func = args[0]
        try:
            func(*args[1:], **kwargs)
        except expected_exception:
            return _pytest._code.ExceptionInfo()
    fail(message)


class MatcherRaisesContext(RaisesContext):

    def __init__(self, expected_exception, message, match_expr, extras):
        super().__init__(expected_exception, message, match_expr)
        self.extras = extras

    def __exit__(self, *tp):
        suppress = super().__exit__(*tp)
        if suppress:
            for k, v in self.extras.items():
                actual = getattr(tp[1], k)
                logging.debug('actual: {} == expected: {}'.format(actual, v))
                assert actual == v, 'Expected {}.{} == {}, found {}'.format(
                    type(tp[1]).__name__, k, v, actual,
                )
        return suppress

The test then looks like this:

def test_upload_csv_failure_org(client, user_data, caplog):
    caplog.set_level(logging.DEBUG)
    org_data = json.dumps(... some proprietary stuff ...)
    with raises(DecodedError, status=403):
       client.upload_csv(user_data, org_data)

[RonnyPfannschmidt]

i propose having a matches=lambda exception: exception.status==403) instead alternatively a check=

because this proposal just ensures that all random keyword arguments suddenly have a meaning, which in turn would a) give misspelligns a really strange behaviour and b) would ensure we no longer have a right for new argument names

in addition it would create a general matcher vs speical arugments difference which in terms of api is just not acceptable

[nicoddemus]

because this proposal just ensures that all random keyword arguments suddenly have a meaning, which in turn would a) give misspelligns a really strange behaviour and b) would ensure we no longer have a right for new argument names

Definitely agree, 👎 on letting any arbitrary keyword argument name become an attribute for checking.

The check=func(exception) is a form I could go with, but I'm not sure it gains us much:

def test_upload_csv_failure_org(client, user_data, caplog):
    org_data = json.dumps(... some proprietary stuff ...)
    with raises(DecodedError, check=lambda e: status==403):
       client.upload_csv(user_data, org_data)

over:

def test_upload_csv_failure_org(client, user_data, caplog):
    org_data = json.dumps(... some proprietary stuff ...)
    with raises(DecodedError) as e:
       client.upload_csv(user_data, org_data)
    assert e.status==403

BUT I had the same argument over match when we discussed its introduction, so I might be wrong. 😁

[ghost]

The thing about the last variant, with assert e.status == 403 is that it really scares anyone who's not 100% familiar with the source code of raises() context manager. When I do a code review to anyone and see that the context manager is referenced after it goes out of scope, I automatically change the code to not do that. If I didn't have a particular interest in raises(), and wasn't sure that that's OK to touch e after it should go out of scope, I'd do just the same here.

Another thing about it, is that e actually cannot have status field. e is a dummy object with no useful properties for the user of the test, some sort of "exception info" kind of thing, but it's intended for use inside pytest (its class isn't even exported, not documented etc.). So, users would have to know that they actually need e.value.status == 403, which is even scarier and very difficult to discover unless you already know the implementation of this thing.

[RonnyPfannschmidt]

having the match object be something more general might be interesting the main question is how to nicely express/integrate it
just using keyword arguments directly unfortunately isnt

[ghost]

@RonnyPfannschmidt absolutely agree, that's why in my initial proposal I wrote about a single callable object which fulfills that purpose.

I would argue for having a parameter that specifies an implementation for RaisesContext. If not given, the default RaisesContext is used, otherwise, the user needs to implement their own version of this context manager. This would even make the version of extra keyword arguments available, since the default behavior would be to reject the unknown arguments, but a user-implemented context manager may accept those.

[nicoddemus]

@wvxvw

Ouch sorry, I definitely meant assert e.value.status==403 in my example.

The thing about the last variant, with assert e.status == 403 is that it really scares anyone who's not 100% familiar with the source code of raises() context manager.

I think it is unfair to state that one has to be "100% familiar with the source of of raises()", after all inspecting the exc_info value is well documented.

When I do a code review to anyone and see that the context manager is referenced after it goes out of scope, I automatically change the code to not do that. If I didn't have a particular interest in raises(), and wasn't sure that that's OK to touch e after it should go out of scope, I'd do just the same here.

Agree, indeed usually you use the object returned by a context-manager inside the with block and is considered bad-style/invalid to access it outside.

Another thing about it, is that e actually cannot have status field. e is a dummy object with no useful properties for the user of the test, some sort of "exception info" kind of thing

Indeed about the status field, that was my mistake in the previous example, but again a little unfair saying "some sort of exception info kind of thing" because it is also documented.

I'm just mentioning the above points to emphasize that one does not need to know the actual source code of pytest.raises to use it correctly because its use is well documented, but I do agree with you that it is not natural when comparing with other context managers.

---

I'm not against the general idea per-se, but I guess we all agree with @RonnyPfannschmidt here:

just using keyword arguments directly unfortunately isnt

So it is more of a question of finding a proper API for checking attributes of the exception in a convenient manner.

I would argue for having a parameter that specifies an implementation for RaisesContext. If not given, the default RaisesContext is used, otherwise, the user needs to implement their own version of this context manager.

But if the user went to all the trouble of implementing their own context manager, isn't it more explicit and better to just use that directly? I see little value in doing this:

with pytest.raises(RuntimeError, manager=raises_with_attr_checking(status=404)):
    ...

Over:

with raises_with_attr_checking(RuntimeError, status=404)):
    ...

[bluetech]

I'm -1 on adding special facilities for ad-hoc matching. assert exc_info.value.status == 403 works, is not ambiguous, is more capable (multiple asserts, assertion rewrite works, failure points to the exact assert), and is already the common and documented way to do it.

I think it would have been better to not have match either (looks like @nicoddemus agreed 😃), though that one at least saves a re import occasionally.

[RonnyPfannschmidt]

I consider Match a good addition as it works well for all exceptions

Special checking should use custom context managers that may build on raises

[nicoddemus]

I think it would have been better to not have match either (looks like @nicoddemus agreed 😃),

Heh! FTR, I've since changed my mind on that, because it is a very common case to check the exception message. I've also get feedback that being so "handy" some colleagues have started checking messages more often than before. So all in all I was wrong on thinking this would not be a good addition.

But I agree with your other points about automatically checking attributes. I still think my suggestion in #3362 (comment) is valid, to use a custom context manager instead.

So perhaps we should close this as "won't do"?

Btw even if we close this, we definitely appreciate the suggestion and discussion that it generated @wvxvw!