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

Fix pytest.raises handling of unicode exceptions in Python 2 #5479

Merged
merged 5 commits into from Jul 4, 2019

Conversation

Projects
None yet
6 participants
@graingert
Copy link
Contributor

commented Jun 24, 2019

Fixes #5478

@graingert

This comment has been minimized.

Copy link
Contributor Author

commented Jun 24, 2019

I think I've missed the boat on Py2 fixes...

@blueyed

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2019

You could create a PR against the 4.6-maintenace branch I guess.
But it should really have a test then - and/or fix the failing ones (have not looked at CI).

@blueyed blueyed added the type: bug label Jun 24, 2019

@graingert

This comment has been minimized.

Copy link
Contributor Author

commented Jun 24, 2019

I think this is against the 4.6 maintenance branch

@nicoddemus

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

The commit is correctly applied on the top of 4.6-maintenance. 👍

Only linting is failing. To fix it, either:

  1. Run tox -e linting and commit the changes.
  2. Install pre-commit and run pre-commit install so each commit gets linted automatically.
@nicoddemus
Copy link
Member

left a comment

Beside my comment, please add a test (the one you posted in #5478 is fine) in testing\python\raises.py and a CHANGELOG entry. 👍

if not re.search(regexp, text_type(self.value)):
assert 0, u"Pattern '{!s}' not found in '{!s}'".format(regexp, self.value)
return True

if not re.search(regexp, str(self.value)):

This comment has been minimized.

Copy link
@nicoddemus

nicoddemus Jun 24, 2019

Member

Actually the only change required is to use text_type(self.value) instead of str(self.value) here.

This comment has been minimized.

Copy link
@graingert

graingert Jun 24, 2019

Author Contributor

This doesn't work when you have a non-ascii bytes exception message

This comment has been minimized.

Copy link
@nicoddemus

nicoddemus Jun 24, 2019

Member

Oh you are absolutely right, self.value might be already encoded. So to avoid duplication I recommend to change to this:

        __tracebackhide__ = True
        value = text_type(self.value) if isinstance(regexp, text_type) else str(self.value)
        if not re.search(regexp, value):
            assert 0, "Pattern '{!s}' not found in '{!s}'".format(regexp, value)
        return True

This comment has been minimized.

Copy link
@graingert

graingert Jun 24, 2019

Author Contributor

But then the formatting won't be safe

This comment has been minimized.

Copy link
@nicoddemus

nicoddemus Jun 24, 2019

Member

You are right... I think this solves it:

assert 0, u"Pattern '{!s}' not found in '{!s}'".format(regexp, value)

It should be safe to always use unicode messages with the assert statement

This comment has been minimized.

Copy link
@pombredanne

pombredanne Jun 25, 2019

Contributor

or you could use a repr instead?
assert 0, u"Pattern {!r} not found in {!r}".format(regexp, value)

This comment has been minimized.

Copy link
@RonnyPfannschmidt

RonnyPfannschmidt Jun 25, 2019

Member

how about safe_str ? or one of the to ascii helper s we have in the python2 support code?

@graingert

This comment has been minimized.

Copy link
Contributor Author

commented Jun 25, 2019

@graingert graingert force-pushed the graingert:patch-1 branch from 5dbbabd to 78f0427 Jun 25, 2019

@codecov

This comment has been minimized.

Copy link

commented Jun 25, 2019

Codecov Report

Merging #5479 into 4.6-maintenance will increase coverage by 0.27%.
The diff coverage is 100%.

Impacted file tree graph

@@                 Coverage Diff                 @@
##           4.6-maintenance    #5479      +/-   ##
===================================================
+ Coverage            95.92%   96.19%   +0.27%     
===================================================
  Files                  115      115              
  Lines                26438    26447       +9     
  Branches              2610     2610              
===================================================
+ Hits                 25360    25441      +81     
+ Misses                 755      702      -53     
+ Partials               323      304      -19
Impacted Files Coverage Δ
src/_pytest/_code/code.py 95.5% <100%> (ø) ⬆️
testing/python/raises.py 94.87% <100%> (+0.27%) ⬆️
src/_pytest/fixtures.py 97.55% <0%> (+0.27%) ⬆️
testing/test_capture.py 99.25% <0%> (+0.29%) ⬆️
testing/test_terminal.py 99.84% <0%> (+0.31%) ⬆️
testing/test_config.py 99.83% <0%> (+0.33%) ⬆️
testing/test_pdb.py 99.22% <0%> (+0.38%) ⬆️
src/_pytest/pytester.py 91.66% <0%> (+0.42%) ⬆️
src/_pytest/assertion/rewrite.py 95.77% <0%> (+0.48%) ⬆️
testing/python/fixtures.py 99.09% <0%> (+0.56%) ⬆️
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2301fa6...34b4e21. Read the comment docs.

@graingert graingert force-pushed the graingert:patch-1 branch 4 times, most recently from 10d0b8b to 881e06b Jun 25, 2019

@graingert

This comment has been minimized.

Copy link
Contributor Author

commented Jun 25, 2019

ERROR: FAIL could not package project - v = InvocationError(u"/home/travis/build/pytest-dev/pytest/.tox/.package/bin/python -m pip install setuptools 'setuptools>=40.0'", 1)

eh?

@blueyed

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2019

py36-xdist:

_________________________ TestRaises.test_raises_match _________________________
[gw0] linux -- Python 3.6.7 /home/travis/build/pytest-dev/pytest/.tox/py36-xdist/bin/python
self = <raises.TestRaises object at 0x7f9829898d68>
    def test_raises_match(self):
        msg = r"with base \d+"
        with pytest.raises(ValueError, match=msg):
            int("asdf")
    
        msg = "with base 10"
        with pytest.raises(ValueError, match=msg):
            int("asdf")
    
        msg = "with base 16"
        expr = r"Pattern '{}' not found in 'invalid literal for int\(\) with base 10: 'asdf''".format(
            msg
        )
        with pytest.raises(AssertionError, match=expr):
            with pytest.raises(ValueError, match=msg):
>               int("asdf", base=10)
E               AssertionError: Pattern "Pattern 'with base 16' not found in 'invalid literal for int\\(\\) with base 10: 'asdf''" not found in 'Pattern \'with base 16\' not found in "invalid literal for int() with base 10: \'asdf\'"'
@nicoddemus

This comment has been minimized.

Copy link
Member

commented Jun 25, 2019

@graingert the previous failure was due to a bug in tox 3.13.0. 3.13.1 has just been out with a fix, so please try again!

@graingert

This comment has been minimized.

Copy link
Contributor Author

commented Jun 25, 2019

@nicoddemus is this fix still desired?

@nicoddemus

This comment has been minimized.

Copy link
Member

commented Jun 25, 2019

I don't mind it. 😁

@asottile are you against this? I understand some parts of the standard library might break with this, but I'm 👍 because we can do better than unittest and with a small change to boot.

@asottile

This comment has been minimized.

Copy link
Member

commented Jun 25, 2019

I'm conflicted, because exceptions with unicode messages in python 2 are wrong, but this enables something that seems like it should work. so -0 from me, feel free to go ahead

@graingert graingert force-pushed the graingert:patch-1 branch from 881e06b to 89653ca Jun 26, 2019

@graingert graingert changed the title special case text_type matches Fixes #5478 Use safe_str to serialize Exceptions in pytest.raise Fixes #5478 Jun 26, 2019

@graingert graingert changed the title Use safe_str to serialize Exceptions in pytest.raise Fixes #5478 Use safe_str to serialize Exceptions in pytest.raises Fixes #5478 Jun 26, 2019

@graingert graingert force-pushed the graingert:patch-1 branch from 89653ca to 0b626be Jun 26, 2019

@graingert graingert force-pushed the graingert:patch-1 branch from 0b626be to 013d0e6 Jun 26, 2019

@@ -278,3 +278,7 @@ def __class__(self):
with pytest.raises(CrappyClass()):
pass
assert "via __class__" in excinfo.value.args[0]

def test_u(self):

This comment has been minimized.

Copy link
@graingert

graingert Jun 26, 2019

Author Contributor
______________________________ TestRaises.test_u ______________________________

self = <raises.TestRaises object at 0x0000000007B297B8>

    def test_u(self):
        with pytest.raises(AssertionError, match=u"\u2603"):
>           assert False, u"\u2603"
E           UnicodeEncodeError: 'ascii' codec can't encode character u'\u2603' in position 0: ordinal not in range(128)

seems to be failing on windows CI

This comment has been minimized.

Copy link
@nicoddemus

nicoddemus Jun 26, 2019

Member

I've applied this patch:

diff --git a/src/_pytest/_code/code.py b/src/_pytest/_code/code.py
index e621f3ee0..407155373 100644
--- a/src/_pytest/_code/code.py
+++ b/src/_pytest/_code/code.py
@@ -574,7 +574,7 @@ class ExceptionInfo(object):
         __tracebackhide__ = True
         value = safe_str(self.value)
         if not re.search(regexp, value):
-            assert 0, "Pattern '{!s}' not found in '{!s}'".format(regexp, value)
+            assert 0, u"Pattern {!r} not found in {!r}".format(regexp, value)
         return True

The problem is that even so the check still fails, when it should pass:

______________________________ test_u _______________________________

    def test_u():
        with pytest.raises(AssertionError, match=u"\u2603"):
>           assert False, u"\u2603"
E           AssertionError: Pattern u'\u2603' not found in '\xe2\x98\x83\nassert False'

.tmp\test_u.py:6: AssertionError

This will be true for any system which can't handle that unicode string using the default locale.

This solution works for me and the rest of the suite:

diff --git a/src/_pytest/_code/code.py b/src/_pytest/_code/code.py
index e621f3ee0..e04bbc915 100644
--- a/src/_pytest/_code/code.py
+++ b/src/_pytest/_code/code.py
@@ -14,6 +14,7 @@ from weakref import ref
 import attr
 import pluggy
 import py
+import six
 from six import text_type

 import _pytest
@@ -572,9 +573,9 @@ class ExceptionInfo(object):
         raised.
         """
         __tracebackhide__ = True
-        value = safe_str(self.value)
+        value = text_type(self.value) if isinstance(regexp, text_type) else str(self.value)
         if not re.search(regexp, value):
-            assert 0, "Pattern '{!s}' not found in '{!s}'".format(regexp, value)
+            assert 0, u"Pattern '{!s}' not found in '{!s}'".format(regexp, value)
         return True

This comment has been minimized.

Copy link
@asottile

asottile Jun 27, 2019

Member

though this probably triggers the same error I posted in the issue when using --tb=native since now we're raising a (n incorrect) unicode message in python 2 🤔

This comment has been minimized.

Copy link
@nicoddemus

nicoddemus Jun 30, 2019

Member

Thanks, but actually it works:

 λ pytest .tmp\test_u.py  -q
.
1 passed in 0.02 seconds
λ pytest .tmp\test_u.py  -q --tb=native
.
1 passed in 0.02 seconds

so I think we can go with that solution. 👍

Show resolved Hide resolved changelog/5478.bugfix.rst Outdated
assert 0, "Pattern '{!s}' not found in '{!s}'".format(regexp, self.value)
value = safe_str(self.value)
if not re.search(regexp, value):
assert 0, "Pattern '{!s}' not found in '{!s}'".format(regexp, value)

This comment has been minimized.

Copy link
@RonnyPfannschmidt

RonnyPfannschmidt Jun 27, 2019

Member

option or followup: we might want to use raise AssertionError(...) in that case

This comment has been minimized.

Copy link
@nicoddemus

nicoddemus Jun 30, 2019

Member

Done

Out of curiosity, is there any reason other than style to prefer one over the other?

Update changelog/5478.bugfix.rst
Co-Authored-By: Bruno Oliveira <nicoddemus@gmail.com>
@graingert

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2019

@nicoddemus I don't really have the time to put into this pr right now, could you take it over?

@nicoddemus nicoddemus force-pushed the graingert:patch-1 branch from 2cdc20b to 09dee29 Jun 30, 2019

@nicoddemus nicoddemus changed the title Use safe_str to serialize Exceptions in pytest.raises Fixes #5478 Fixes handling of unicode exceptions with pytest.raises in Python 2 Jun 30, 2019

@nicoddemus nicoddemus changed the title Fixes handling of unicode exceptions with pytest.raises in Python 2 Fix pytest.raises handling of unicode exceptions in Python 2 Jun 30, 2019

"""pytest.raises should be able to match unicode messages when using a unicode regex (#5478)
"""
with pytest.raises(AssertionError, match=u"\u2603"):
assert False, u"\u2603"

This comment has been minimized.

Copy link
@asottile

asottile Jun 30, 2019

Member

can you also test a failure of unicode<=>unicode as well?

This comment has been minimized.

Copy link
@nicoddemus

nicoddemus Jun 30, 2019

Member

Sure, but what do you mean by unicode<=>unicode? 🤔

This comment has been minimized.

Copy link
@asottile

asottile Jun 30, 2019

Member

oh yeah that is super unclear, what the heck did I mean 🤔

there's 6(?) combinations I think and maybe need more tests for them:

  • bytes exception bytes regex success
  • bytes exception bytes regex failure
  • unicode exception unicode regex success
  • unicode exception unicode regex failure (I suspect that this currently causes an undisplayable exception in python2 because pytest would be raising a unicode exception)
  • ascii unicode exception ascii unicode bytes success (only in py2 due to implicit py2 conversion)
  • ascii unicode exception ascii unicode bytes failure (only in py2 due to implicit py2 conversion)

This comment has been minimized.

Copy link
@nicoddemus

nicoddemus Jul 1, 2019

Member

I see, thanks. Done.

Surprisingly on Python 3 we don't support raising exceptions using bytes (raise RuntimeError(b"foo")), which I find reasonable actually.

Show resolved Hide resolved src/_pytest/_code/code.py Outdated
@asottile
Copy link
Member

left a comment

looks correct, despite it enabling incorrect exceptions in python2

@nicoddemus nicoddemus merged commit 46a0888 into pytest-dev:4.6-maintenance Jul 4, 2019

5 checks passed

WIP Ready for review
Details
codecov/patch 100% of diff hit (target 95.92%)
Details
codecov/project 96.19% (+0.27%) compared to 2301fa6
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
pytest-CI #20190704.2 succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.