Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support __notes__ in pytest.raises #11223

Closed
ivirshup opened this issue Jul 17, 2023 · 10 comments · Fixed by #11227
Closed

Support __notes__ in pytest.raises #11223

ivirshup opened this issue Jul 17, 2023 · 10 comments · Fixed by #11227

Comments

@ivirshup
Copy link
Contributor

I would like pytest.raises to have some form of support for __notes__.

What's the problem this feature will solve?

I am currently looking into replacing dynamic patching of error messages with the new __notes__ feature added in PEP-678.

However, once I use __notes__ my checks using pytest.raises now fail, and I'd like them to not. Or at least, I would like for pytest to have some features for matching these errors. Since this is a language level feature for python, I think it's usage should be covered by pytest.

Describe the solution you'd like

I would like the regex from pytest.raises to match against the notes as well as the body of the error. It would be fine for this to be specified with a keyword argument.

Example of usage:

import pytest

def foo():
    assert False

def bar():
    try:
        foo()
    except AssertionError as e:
        e.add_note("Raised by bar")
        raise

with pytest.raises(AssertionError, match="Raised by bar"):
    bar()

Alternative Solutions

Do nothing

I could manually check for notes, but I don't think python makes it obvious which part of an exception is from a note. Here is a small example of what a note looks like:

setup
# Python 3.11.4 | packaged by conda-forge | (main, Jun 10 2023, 18:10:28) [Clang 15.0.7 ] on darwin
# Type "help", "copyright", "credits" or "license" for more information.
>>> import sys
>>> 
>>> def add_note(err: BaseException, msg: str) -> None:
...     if sys.version_info < (3, 11):
...         err.__notes__ = getattr(err, "__notes__", []) + [msg]
...     else:
...         err.add_note(msg)
... 
>>> def foo():
...     raise AssertionError("foo is a bad function")
... 
>>> try:
...     foo()
... except AssertionError as e:
...     add_note(e, "yeah, foo is awful")
...     raise e
Traceback (most recent call last):
  File "<stdin>", line 5, in <module>
  File "<stdin>", line 2, in <module>
  File "<stdin>", line 2, in foo
AssertionError: foo is a bad function
yeah, foo is awful

I think this would make it quite difficult for someone receiving an exception with notes from an upstream library to know how to match against it.

I'm currently handling this with a helper function:

def check_error_or_notes_match(e: pytest.ExceptionInfo, pattern: str):
    """
    Checks whether the printed error message or the notes contains the given pattern.

    DOES NOT WORK IN IPYTHON - because of the way IPython handles exceptions
    """
    import traceback

    message = "".join(traceback.format_exception_only(e.type, e.value))
    assert re.search(
        pattern, message
    ), f"Could not find pattern: '{pattern}' in error:\n\n{message}\n"

Additional context

This line would have to change:

value = str(self.value)
.

@RonnyPfannschmidt
Copy link
Member

We need a design decision on whether match plainly should apply to notes or if we should use a new name

@ivirshup
Copy link
Contributor Author

Given that the standard library's error formatting does not make it explicit which parts of the error message were from notes, I think it would make sense to match against the notes by default.

@RonnyPfannschmidt
Copy link
Member

It's not necessarily a good idea to inform checks of data details from presentation formatting

@nicoddemus
Copy link
Member

nicoddemus commented Jul 17, 2023

It would be nice to get @Zac-HD's feedback, as the original PEP author. 😁

I would lean towards a new match_notes: str keyword-only parameter, seems like the safest approach, but I have not used the notes feature yet so I wouldn't know for sure.

@ivirshup
Copy link
Contributor Author

It's not necessarily a good idea to inform checks of data details from presentation formatting

I have a mild preference for matching against the traceback formatted exception by default. But I think an opt-in solution would be fine too. I would really like to be able to match both the exception message and the notes with a single pattern. E.g. I would like to be able to test what the user ends up seeing.

My specific use case is that I am doing IO on a hierarchical format. If an exception occurs, I want the path that was being operated on to be included in the error message. Tests that I am writing here include:

  • The correct error is thrown
  • The correct path is being included
  • A note is only added for the current path, and not for each preceding level in the hierarchy.

I would lean towards a new match_notes: str keyword-only parameter,

I don't love passing a separate pattern for the notes vs the "body". I think this tests the structure of the error, but not in a very precise way. Does this match any note? All notes? The n-th note?

@Zac-HD
Copy link
Member

Zac-HD commented Jul 17, 2023

I agree with @ivirshup; it seems most natural to me for the existing match= argument to be passed the exception string including any notes.

I expect more complex use cases to be very rare, and they can always inspect the attributes directly.

@nicoddemus
Copy link
Member

Cool, thanks for the explanation.

I'm 👍 then for the existing match parameter to also match notes.

@ivirshup
Copy link
Contributor Author

ivirshup commented Jul 17, 2023

I've started a proof of concept here: https://github.com/ivirshup/pytest/tree/match-exception-notes

Still needs:

  • Some way of enabling/ disabling the behavior
  • Docs
  • Testing support for python<3.11
  • Better tests

@Zac-HD
Copy link
Member

Zac-HD commented Jul 17, 2023

  • I'd leave it unconditionally enabled
  • docs are good
  • the exceptiongroup backport provides support on older Pythons, and we already depend on it
  • tests are also good

@ivirshup
Copy link
Contributor Author

ivirshup commented Jul 17, 2023

Opened a draft PR, but happy to continue discussing design here in case any significant changes would be made.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants