Improvement: extend pytest.raises to support Exception having __repr__ method and initialized with kwargs

[BoWuGit]

Issue

I met this issue when using FastAPI's HTTPException, which defines as follow. And when I use pytest.raises to assert HTTPException initialized with kwargs as follows, it does't work, so I extended it.

with pytest.raises(HTTPException, match=r"404"):
    kwargs_exception = HTTPException(status_code=404)
    raise kwargs_exception

class HTTPException(Exception):
    def __init__(
        self,
        status_code: int,
        detail: typing.Optional[str] = None,
        headers: typing.Optional[dict] = None,
    ) -> None:
        if detail is None:
            detail = http.HTTPStatus(status_code).phrase
        self.status_code = status_code
        self.detail = detail
        self.headers = headers

    def __repr__(self) -> str:
        class_name = self.__class__.__name__
        return f"{class_name}(status_code={self.status_code!r}, detail={self.detail!r})"

Backward compatibility

I think this pr would not break current behavior, because we generally use pytest.raises match to check details, it defaults to Exception.__str__ method, which prints args. What I extend is to support Exception.__repr__ method and kwargs when Exception is initialized without args, so it's intelligent than ever.

Thanks for your attention.

[bluetech]

Hi @BoWuGit,

Do you know why FastAPI decided to use __repr__ for the exceptions, when __str__ is the usual choice?

Instinctively I'm somewhat negative on this change, because it replaces a clear assertion (matching how an exception "renders", which is usually its __str__) with something more ambiguous, and clarity is good to have for such things.

For you specific example, an alternative would be:

1. If you specifically wants to test the repr as a "UI test", do the assert directly: assert re.match("404", repr(excinfo.value))
2. If you want to logically match the status code, assert that: assert excinfo.value.status_code == 404.

[The-Compiler]

This seems like very unexpected behavior to me as well - the last thing I'd want from a test framework is it guessing what I might have meant if something about my implementation is strange. If I forget to implement a __str__, pytest should tell me there's something wrong, and not silently fall back to something else.

[RonnyPfannschmidt]

It seems necessary to reject misbehaved exception type support here

Imho fastapi should fix the exception classes

Alternatively a fastapi specific wrapper could match values on correct types instead of yolo on repr

[BoWuGit]

@The-Compiler If I forget to implement a str, pytest should tell me there's something wrong, and not silently fall back to something else.

What you said makes sense, so how about print some warning info when str(elf.value) is empty？For example when Exception is initialized with kwargs, we can check args attr of Exception as follows:

        value = str(self.value)
        msg = f"Regex pattern did not match.\n Regex: {regexp!r}\n Input: {value!r}"
        if regexp == value:
            msg += "\n Did you mean to `re.escape()` the regex?"
        elif not value and not self.value.args:
            msg += "\n Did you initialize Exception with kwargs? If so, you can try to initialize with args for `str()` to get the message."

Personally I really puzzled when HTTPException didn't work suddenly, only because that I initialized it with kwargs. And only when I debugged source code of pytest can I know where went wrong finally.

Thanks for all your attention.

[The-Compiler]

I see what you mean now... whoa, that seems like very magical Python behavior, I never noticed this before:

>>> class WeirdException(Exception):
...     def __init__(self, code):
...         pass
... 
>>> str(WeirdException(42))
'42'
>>> str(WeirdException(code=42))
''

because

>>> WeirdException(42).args
(42,)
>>> WeirdException(code=42).args
()

notably, without WeirdException ever calling the Exception.__init__!

---

I'm however not very convinced about that message either. Like @bluetech said, it doesn't look like what you actually wanted to test is the string representation of the exception. If you want to test the .status_code, you should check that, and not jump through all those hoops and rely on the autogenerated Python string representation in the first place. I feel like such a hint would encourage what I'd consider a bad practice.

I'd be open to making the existing output a bit more clear (say, maybe displaying __str__: or str(exc): instead of Value:?) - but I don't think pytest should guess where an empty __str__ value could possibly be coming from.

[BoWuGit]

@The-Compiler notably, without WeirdException ever calling the Exception.init!

And if you call super().__init__, it works, but generally we don't for sub class of Exception.

>>> class WeirdException(Exception):
...     def __init__(self, code):
...         print(code)
...         super().__init__(code)
... 
>>> str(WeirdException(code=42))
42
'42'

If you want to test the .status_code, you should check that

What I really want to check is detail of HTTPException, which is just the use case of pytest.raises match, .status_code is mocked to show the case.

I'd be open to making the existing output a bit more clear (say, maybe displaying str: or str(exc): instead of Value:?)

OK, I agree, if that's the case, then others meeting this issue would not spend time to figure why like I did.

[The-Compiler]

@The-Compiler notably, without WeirdException ever calling the Exception.init!

And if you call super().__init__, it works, but generally we don't for sub class of Exception.

Genuine question: Why not? Not calling the super-class __init__ often leads to funky behavior - and all Python tutorials I've been able to find about the topic do call super().__init__(message) in some way or another when writing a custom __init__ for exceptions.

IMHO, starlette not doing that is rather questionable.

If you want to test the .status_code, you should check that

What I really want to check is detail of HTTPException, which is just the use case of pytest.raises match, .status_code is mocked to show the case.

I disagree. If you want to check the detail attribute of an exception object, you should check excinfo.value.detail, and not rely on a string representation which might or might not be there. If the detail was explicitly in the exception message (e.g. via starlette doing a super().__init__(detail), or having a __str__ doing return self.detail), then that'd be another story. But right now, you seem to be testing an implementation detail of Python exceptions rather than testing what you actually want to test.

I'd be open to making the existing output a bit more clear (say, maybe displaying str: or str(exc): instead of Value:?)

OK, I agree, if that's the case, then others meeting this issue would not spend time to figure why like I did.

So you think that would have helped in your case? If so, happy to review a change for that.

[BoWuGit]

IMHO, starlette not doing that is rather questionable.

OK, so I'll try to submit a pr to starlette.

If you want to check the detail attribute of an exception object, you should check excinfo.value.detail, and not rely on a string representation which might or might not be there.

As a user not so familiar with pytest, I admit that pytest.raises is more approachable than excinfo, which likes unittest.assertRaises of standard library.

So you think that would have helped in your case? If so, happy to review a change for that.

I think that's helpful, because from pytest.raises match we can't go to definition of match method directly.

[The-Compiler]

Seems fine by me. Let's see what others think!

[ivirshup]

As a heads up, #11227 just changed how raises match against exceptions so it's no longer exactly str(exception).

[RonnyPfannschmidt]

Based on the upstream fix I propose we close this

[BoWuGit]

OK, I updated document of pytest.raises to be more clear by adding PEP-678 __notes__.