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.

Sign up for GitHub

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

Jump to bottom

Better error message for pytest.raises() or pytest.warns() with empty tuple #9911

Merged
merged 5 commits into from May 11, 2022
Merged

Better error message for pytest.raises() or pytest.warns() with empty tuple #9911

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3444d35
improve `pytest.raises`
bkeyvani Apr 30, 2022
31a9c5c
improve `pytest.raises` - cont'd
bkeyvani May 10, 2022
29462b1
type-ignore in error-message test
Zac-HD May 11, 2022
58af761
Move changelog file
Zac-HD May 11, 2022
a29f4af
Update 8646.improvement.rst
Zac-HD May 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions AUTHORS
View file
Open in desktop
Expand Up @@ -44,6 +44,7 @@ Aron Coyle
Aron Curzon
Aviral Verma
Aviv Palivoda
Babak Keyvani
Barney Gale
Ben Gartner
Ben Webb
Expand Down
2 changes: 2 additions & 0 deletions changelog/8646.improvments.rst
View file
Open in desktop
@@ -0,0 +1,2 @@
Improve :py:func:`pytest.raises`. Previously passing an empty tuple would give a confusing
error. We now raise immediately with a more helpful message.
7 changes: 7 additions & 0 deletions src/_pytest/python_api.py
View file
Open in desktop
Expand Up @@ -899,6 +899,13 @@ def raises(
"""
__tracebackhide__ = True

if expected_exception == ():
raise ValueError(
"Passing expected_exception=() is an error, because it's impossible to "
"raise an exception which is not an instance of any type. Raising exceptions "
"is already understood as failing the test, so you don't need any special "
"code to say 'this should never raise an exception'."
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this message seems a but too elaborate to me, a first idea for a shorter version may be something like (might need another 1-2 iterations)

Suggested change
if expected_exception == ():
raise ValueError(
"Passing expected_exception=() is an error, because it's impossible to "
"raise an exception which is not an instance of any type. Raising exceptions "
"is already understood as failing the test, so you don't need any special "
"code to say 'this should never raise an exception'."
)
if expected_exception == ():
raise ValueError(
"The parameter ``expected_exception`` was ``()`` instead of a exception type or a tuple of exception types.\n"
"Please pass a Exception subclass or a Tuple of them to pytest.raises."
)

it seems surprisingly hard to make this one short and to the point

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion @RonnyPfannschmidt. should I apply and commit the suggested change, or allow some time for other team members to review and comment on this?
Also, I'm thinking of making the following minor changes to the last line:

"Please pass an Exception subclass or a tuple of them to ``pytest.raises``."
              ^                         ^                ^              ^

Please let me know what you think :)
Thanks

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my suggestion was merely a starting point - maybe @nicoddemus has another insight

your fixed one is certainly better than my first hastily typed suggestion

i propose you do the change, and i approve

if anyone else has another suggestion they can add it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The suggestion looks good; while I appreciate the initial take to explain the ins and outs, I think being brief here is a bit better.

Also, I suggest removing the double ticks around the names, as this text is printed to the console, I don't think they make the message easier to read/understand.

Also reading it a few more times, I think we can be even briefer:

Suggested change
if expected_exception == ():
raise ValueError(
"Passing expected_exception=() is an error, because it's impossible to "
"raise an exception which is not an instance of any type. Raising exceptions "
"is already understood as failing the test, so you don't need any special "
"code to say 'this should never raise an exception'."
)
if not expected_exception:
raise ValueError(
"Expected an exception type or a tuple a tuple of exceptions types, but got {:r}"
)

The not expected_exception will also catch other problems, like passing None or [].

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll argue for keeping "Raising exceptions is already understood as failing the test, so you don't need any special code to say 'this should never raise an exception'."

I originally proposed this as a final sentence because it dissolves a remarkably common beginner misconception, and helps to teach the way that pytest works differently from unit testing tools in other languages which often require something like an assertRaises method. @nicoddemus' shorter message is a better opener, but I'd like to add the beginner's advice too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As another data point: I've seen this question (how to make sure it doesn't raise an exception) come up from time to time in my pytest trainings as well. I don't think anyone has tried () so far, but I agree it would probably be helpful to add that explanation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem, makes sense! Indeed that's a common beginner misconception.

if isinstance(expected_exception, type):
excepted_exceptions: Tuple[Type[E], ...] = (expected_exception,)
else:
Expand Down
4 changes: 4 additions & 0 deletions testing/python/raises.py
View file
Open in desktop
Expand Up @@ -19,6 +19,10 @@ def test_raises_function(self):
excinfo = pytest.raises(ValueError, int, "hello")
assert "invalid literal" in str(excinfo.value)

def test_raises_does_not_allow_empty_tuple(self):
with pytest.raises(ValueError):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's match the message partially to ensure we don't regress this by accident:

Suggested change
with pytest.raises(ValueError):
with pytest.raises(ValueError, match="Expected an exception type or"):

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @nicoddemus and @Zac-HD for the feedbacks.
@nicoddemus, I wasn't too sure about the following in your comment on the error message:

"Expected an exception type or a tuple a tuple of exceptions types, but got {:r}"
                                                                            ^^^^

I used f-strings instead. Hope it's ok, and passes the tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure why the this pre-commit (https://results.pre-commit.ci/run/github/37489525/1652147464.KX3rmadkSXKYLprqHEnP1A) failed.
it's complaining about a test case I added to test for expected_exception=None.

testing/python/raises.py:24: error: No overload variant of "raises" matches argument type "None"  [call-overload]

The test passes when I run the tests locally. Should I remove that test case?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually kinda nice - mypy is also warning us that None is invalid here. Since I do want the runtime check to give us a nice error message, I've just pushed a type: ignore marker and explanatory comment.

pytest.raises(expected_exception=())

def test_raises_callable_no_exception(self) -> None:
class A:
def __call__(self):
Expand Down