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

Remove "matching '...'" part from the pytest.raises message #3222

Merged
merged 2 commits into from
Feb 17, 2018
Merged

Remove "matching '...'" part from the pytest.raises message #3222

merged 2 commits into from
Feb 17, 2018

Conversation

The-Compiler
Copy link
Member

When a test with pytest.raises(ValueError, match='foo') doesn't raise, the
following error is printed:

Failed: DID NOT RAISE <class 'ValueError'> matching 'foo'

This error message is confusing as it implies a ValueError was raised, but the
message wasn't matching 'foo'.

I first considered rewording it somehow to preserve the match pattern in it, but
I don't think that's worthwhile as the pattern should usually be apparent from
the stacktrace anyways (hard-coded, as parametrization, or with --showlocals for
more sophisticated cases).

Targetting master as I think this is trivial - let me know if you disagree.

cc @Kriechi who originally added the message (together with the feature) - if you had a reason to include it which I didn't think about, please speak up! 😉

When a test with pytest.raises(ValueError, match='foo') doesn't raise, the
following error is printed:

    Failed: DID NOT RAISE <class 'ValueError'> matching 'foo'

This error message is confusing as it implies a ValueError was raised, but the
message wasn't matching 'foo'.

I first considered rewording it somehow to preserve the match pattern in it, but
I don't think that's worthwhile as the pattern should usually be apparent from
the stacktrace anyways (hard-coded, as parametrization, or with --showlocals for
more sophisticated cases).
@Kriechi
Copy link
Contributor

Kriechi commented Feb 14, 2018

I agree that this message might be ambiguous. The main intention was to be able to distinguish between

  • nothing raised at all
  • wrong exception class was raised
  • correct exception class, but the error message didn't match

As far as I know, at least in my projects, I do not rely on the actual error message of pytest. So removing the matching part should be ok. However, I think we should still include a proper message "why" it failed (i.e., the three cases above).

@coveralls
Copy link

coveralls commented Feb 14, 2018

Coverage Status

Coverage decreased (-0.0008%) to 92.631% when pulling 3cbf0c8 on The-Compiler:match-msg into e7bcc85 on pytest-dev:master.

@The-Compiler
Copy link
Member Author

However, I think we should still include a proper message "why" it failed (i.e., the three cases above).

Indeed! I feel like we're closer to that with the commit I already pushed, and I just pushed a second one for the case when a different/wrong exception is raised.

Test script

import pytest

def test_nothing_no_match():
    with pytest.raises(ValueError):
        pass

def test_nothing():
    with pytest.raises(ValueError, match='foo'):
        pass

def test_wrong_class_no_match():
    with pytest.raises(ValueError):
        raise IndexError('bar')

def test_wrong_class():
    with pytest.raises(ValueError, match='foo'):
        raise IndexError('bar')

def test_wrong_message():
    with pytest.raises(ValueError, match='foo'):
        raise ValueError('bar')

Output without this PR

No exception

Without match=

    def test_nothing_no_match():
        with pytest.raises(ValueError):
>           pass
E           Failed: DID NOT RAISE <type 'exceptions.ValueError'>

With match=

    def test_nothing():
        with pytest.raises(ValueError, match='foo'):
>           pass
E           Failed: DID NOT RAISE <type 'exceptions.ValueError'> matching 'foo'

This is the case fixed with my first commit, so this message doesn't imply that something was raised.

Wrong exception class

Without match

    def test_wrong_class_no_match():
        with pytest.raises(ValueError):
>           raise IndexError('bar')
E           IndexError: bar

With match

    def test_wrong_class():
        with pytest.raises(ValueError, match='foo'):
>           raise IndexError('bar')
E           AssertionError: Pattern 'foo' not found in 'bar'

This is the case fixed with my second commit, as I don't think the pattern is relevant when we got a different exception.

Wrong message

    def test_wrong_message():
        with pytest.raises(ValueError, match='foo'):
>           raise ValueError('bar')
E           AssertionError: Pattern 'foo' not found in 'bar'

Changed output with this PR

No exception

With match

    def test_nothing():
        with pytest.raises(ValueError, match='foo'):
>           pass
E           Failed: DID NOT RAISE <type 'exceptions.ValueError'>

This is the case fixed with my first commit, so this message doesn't imply that something was raised.

Wrong exception class

With match

    def test_wrong_class():
        with pytest.raises(ValueError, match='foo'):
>           raise IndexError('bar')
E           IndexError: bar

This is the case fixed with my second commit, as I don't think the pattern is relevant when we got a different exception.

@nicoddemus
Copy link
Member

Restarted the Travis job that failed, it will probably be fine.

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

Thanks @The-Compiler!

@nicoddemus nicoddemus merged commit b486e12 into pytest-dev:master Feb 17, 2018
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