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

bkeyvani · 2022-05-02T17:17:01Z

Improved :py:func:pytest.raises. Previously passing an empty tuple would give a confusing error. We now raise immediately with a more helpful message.
this PR closes Improved error when () (empty tuple) is passed to pytest.raises() or pytest.warns() #8646

RonnyPfannschmidt

good idea !, lets give the exact message a few more iterations

RonnyPfannschmidt · 2022-05-02T20:33:41Z

src/_pytest/python_api.py

+    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'."
+        )


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

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

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

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 [].

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.

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.

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

nicoddemus · 2022-05-06T00:23:46Z

src/_pytest/python_api.py

+    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'."
+        )


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 [].

nicoddemus · 2022-05-06T00:25:57Z

testing/python/raises.py

@@ -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):


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"):

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.

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?

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.

a few more iterations on error message and related tests.

Zac-HD

This looks great to me - thanks again @bkeyvani 🎉

Requested changes to the error message have been addressed in subsequent discussion.

bkeyvani · 2022-05-11T23:36:11Z

Thank you @RonnyPfannschmidt, @nicoddemus, @The-Compiler for your helpful comments, and @Zac-HD for your help and mentorship.
I look forward to contributing more.
Best.

improve pytest.raises

3444d35

RonnyPfannschmidt previously requested changes May 2, 2022

View reviewed changes

nicoddemus reviewed May 6, 2022

View reviewed changes

bkeyvani and others added 2 commits May 9, 2022 21:15

improve pytest.raises - cont'd

31a9c5c

a few more iterations on error message and related tests.

type-ignore in error-message test

29462b1

Zac-HD approved these changes May 11, 2022

View reviewed changes

Zac-HD added 2 commits May 10, 2022 23:27

Move changelog file

58af761

Update 8646.improvement.rst

a29f4af

Zac-HD merged commit ccdee08 into pytest-dev:main May 11, 2022

bkeyvani deleted the fix-issue-8646 branch May 11, 2022 23:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

bkeyvani commented May 2, 2022

RonnyPfannschmidt left a comment

RonnyPfannschmidt May 2, 2022

bkeyvani May 4, 2022

RonnyPfannschmidt May 4, 2022

nicoddemus May 6, 2022

Zac-HD May 7, 2022

The-Compiler May 7, 2022

nicoddemus May 9, 2022

nicoddemus May 6, 2022

nicoddemus May 6, 2022

bkeyvani May 10, 2022

bkeyvani May 10, 2022

Zac-HD May 11, 2022

Zac-HD left a comment

bkeyvani commented May 11, 2022

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

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

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

Conversation

bkeyvani commented May 2, 2022

RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zac-HD left a comment

Choose a reason for hiding this comment

bkeyvani commented May 11, 2022

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

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