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

Pytest.raises custom error message #1616

Merged
merged 6 commits into from
Jun 20, 2016

Conversation

palaviv
Copy link
Contributor

@palaviv palaviv commented Jun 16, 2016

This PR change pytest.raises to accept custom made message that will be raised if no exception was raised. For example:

with pytest.raises(KeyError, message="I am expecting a KeyError to be raised"):
    pass
Failed: I am expecting a KeyError to be raised

The use case behind this feature is as follows:

for i in random.shuffle(["a", "b", "c"]):
   with pytest.raises(KeyError):
        if i == "b":
            raise KeyError

As you can see in this example I can't know at which input I have failed from the result of the test. The new message will allow me to change the test as follows:

for input in random.shuffle(["a", "b", "c"]):
   with pytest.raises(KeyError, message="Key error was not raised for input {}".format(i)):
        if input == "b":
            raise KeyError

Now I can know the exact input the test failed.

@coveralls
Copy link

coveralls commented Jun 16, 2016

Coverage Status

Coverage increased (+0.003%) to 92.43% when pulling 8ddbca3 on palaviv:pytest.raises-message into 308396a on pytest-dev:features.

@@ -66,7 +66,8 @@
* Add ``build/`` and ``dist/`` to the default ``--norecursedirs`` list. Thanks
`@mikofski`_ for the report and `@tomviner`_ for the PR (`#1544`_).

*
* pytest.raises accepts a custom message to raise when no exception accord.
Copy link
Member

Choose a reason for hiding this comment

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

accord -> occured

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@coveralls
Copy link

coveralls commented Jun 16, 2016

Coverage Status

Coverage increased (+0.003%) to 92.43% when pulling e6ff01a on palaviv:pytest.raises-message into 308396a on pytest-dev:features.

@nicoddemus
Copy link
Member

Hey @palaviv, may thanks for the PR!

I see one small issue with it thought: since it now accepts a message keyword argument, in the non-context manager usage, how can we differentiate if the keyword argument is meant to the underlying function or to pytest.raises? For example:

def display_warning(*, code, message):
    if not isinstance(code, int):
        raise TypeError
    if not isinstance(code, str):
        raise TypeError

def test_display_warning():
    pytest.raises(TypeError, display_warning, code=[], message='a warning')
    pytest.raises(TypeError, display_warning, code=123, message=[])    

So pytest.raises will always interpret message as the extra message, never forwarding it to the underlying function, which will be confusing and might break existing code.

I suggest we accept and handle the message keyword argument only in the context manager form... I think it is a valid trade-off.

@The-Compiler and @palaviv, what do you guys think?

@palaviv
Copy link
Contributor Author

palaviv commented Jun 19, 2016

This sounds like a good suggestion.

if not args:
return RaisesContext(expected_exception)
if "message" in kwargs:
Copy link
Member

Choose a reason for hiding this comment

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

Please update the docstring to include this new keyword argument support; make sure to mention it is only valid in the context manager form. 😁

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@coveralls
Copy link

coveralls commented Jun 19, 2016

Coverage Status

Coverage increased (+0.003%) to 92.43% when pulling ca09367 on palaviv:pytest.raises-message into 308396a on pytest-dev:features.

@coveralls
Copy link

coveralls commented Jun 19, 2016

Coverage Status

Coverage increased (+0.003%) to 92.43% when pulling c29130d on palaviv:pytest.raises-message into 308396a on pytest-dev:features.

@@ -85,6 +85,13 @@ and if you need to have access to the actual exception info you may use::
the actual exception raised. The main attributes of interest are
``.type``, ``.value`` and ``.traceback``.

In the context manager form you may use the keyword argument
Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

One last thing, please add a versionadded directive before this paragraph:

.. versionadded:: 2.10

Copy link
Contributor Author

Choose a reason for hiding this comment

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

shouldn't it be after the paragraph?

Copy link
Contributor Author

@palaviv palaviv Jun 19, 2016

Choose a reason for hiding this comment

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

Also wouldn't a versionchanged be more appropriate as the function was changed?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm that's a good question. The other places in pytest documentation which use this feature put it before the paragraph...

You are right, that's how Python's own documentation does it at least. 😁

Copy link
Member

Choose a reason for hiding this comment

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

Also wouldn't a versionchanged be more appropriate as the function was changed?

Good point. I was thinking along the lines of adding a new parameter, but I think your suggestion makes more sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think then that I should keep with the project conventions. Consistency is more important.

@nicoddemus
Copy link
Member

LGTM! @The-Compiler, if you are OK with it feel free to merge it. 😄

@coveralls
Copy link

coveralls commented Jun 19, 2016

Coverage Status

Coverage increased (+0.003%) to 92.43% when pulling f8d4cad on palaviv:pytest.raises-message into 308396a on pytest-dev:features.

else:
assert False, "Expected pytest.raises.Exception"

def test_costum_raise_message(self):
Copy link
Member

Choose a reason for hiding this comment

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

costum -> custom, but I'll fix it up when merging

@The-Compiler The-Compiler merged commit 4f2db6c into pytest-dev:features Jun 20, 2016
@The-Compiler
Copy link
Member

Thanks!

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 this pull request may close these issues.

4 participants