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鈥檒l 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

Conversation

Projects
None yet
4 participants
@The-Compiler
Member

The-Compiler commented Feb 14, 2018

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! 馃槈

Remove "matching '...'" part from the pytest.raises message
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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Member

The-Compiler commented Feb 15, 2018

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

This comment has been minimized.

Member

nicoddemus commented Feb 17, 2018

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

@nicoddemus nicoddemus merged commit b486e12 into pytest-dev:master Feb 17, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment