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 repr_traceback to work with unicode errors #1506

Merged

Conversation

prusse-martin
Copy link
Contributor

Properly handle errors with unicode messages (could cause unicode decode errors when converting to str).
Added test to check if different encodings in raised error will not cause the code to fail.

@nicoddemus
Copy link
Member

Thanks for porting this PR from bitbucket @prusse-martin!

Could you take a look at the failing tests?

Also, please add a changelog entry and yourself to AUTHORS. 😁

@prusse-martin
Copy link
Contributor Author

My bad. When checking travis-ci to see if some new error have been introduced got confused and ended looking at the build results of master.

@nicoddemus
Copy link
Member

No worries!

(note, make sure to comment something here when you think the PR is ready for review again, because GitHub does not send new notification e-mails when commits are added, only for new comments on the PR).

@prusse-martin
Copy link
Contributor Author

The tests are passing. I actually have mixed felling about the way I did it. The other possible way is to use something like:

...
try:
  msg = str(error)  # python 3 (will also work in python 2 if the error message is ascii convertible)
except UnicodeDecodeError:
  if hasattr(error, 'message'):  # if just to be safe
    msg = error.message  # no 'message' attr in python 3
    if isinstance(msg, bytes):
      msg = msg.decode(sys.getdefaultencoding(), 'replace')  # python 2.6
  else:
    msg = ''
if "maximum recursion depth exceeded" in msg:
  ...
...

Any suggestions?

@nicoddemus
Copy link
Member

After some thought I realized that we only need to check if the exception was a "maximum recursion depth" exception, we don't need to try to come up with a generic solution to convert a solution to string first.

With that in mind how about:

# code.py
if sys.version_info[:2] >= (3, 5):  # RecursionError introduced in 3.5
    def is_recursion_error(exception):
        return isinstance(exception, RecursionError)
else:
    def is_recursion_error(exception):
        if not isinstance(exception, RuntimeError):
            return False
        try:
            return "maximum recursion depth exceeded" in str(exception)
        except UnicodeDecodeError:
            return False

class FormattedExcinfo(object):
    ...
    def repr_traceback(self, excinfo):
        traceback = excinfo.traceback
        if self.tbfilter:
            traceback = traceback.filter()
        recursionindex = None
        if is_recursion_error(excinfo.value):        
            recursionindex = traceback.recursionindex()
        ...

The line return "maximum recursion depth exceeded" in str(exception) will always successfully check for recursion error in both Python 2 and 3 because the error message for the recursion will always be the same, ascii-compatible, message.

What do you think?

@@ -7,6 +7,9 @@

*

* Fix maximum recursion depth detection when raised error class is not aware
Copy link
Member

Choose a reason for hiding this comment

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

Please add a reference back to the PR, something along the lines:

* Fix maximum recursion depth detection when raised error class is not aware
  of unicode/encoded bytes. Thanks `@prusse-martin`_ for the PR (`#1506`_). 

.. _#1506: https://github.com/pytest-dev/pytest/pull/1506
.. _@prusse-martin: https://github.com/prusse-martin

@prusse-martin
Copy link
Contributor Author

I thought something like your proposed code. I just felt a bit awkward about explicitly checking the version at that moment.

@nicoddemus
Copy link
Member

I would say that's OK, specially because RecursionError was included in 3.5 only.

Avoid errors `UnicodeErrosr`s due non maximum recursion depth errors
when checking for those errors.
@prusse-martin prusse-martin force-pushed the fix-repr-tb-with-unicode branch from bc62d51 to 7ce5873 Compare April 9, 2016 02:52
@prusse-martin
Copy link
Contributor Author

Done. The coverage results got down a bit due the version dependent implementation (the tests for coverage have been run with python 3.4).

@nicoddemus
Copy link
Member

Thanks a lot Martin!

@nicoddemus nicoddemus merged commit fe6e1b2 into pytest-dev:master Apr 9, 2016
jsonn pushed a commit to jsonn/pkgsrc that referenced this pull request Jun 6, 2016
2.9.2
=====

**Bug Fixes**

* fix `#510`_: skip tests where one parameterize dimension was empty
  thanks Alex Stapleton for the Report and `@RonnyPfannschmidt`_ for the PR

* Fix Xfail does not work with condition keyword argument.
  Thanks `@astraw38`_ for reporting the issue (`#1496`_) and `@tomviner`_
  for PR the (`#1524`_).

* Fix win32 path issue when puttinging custom config file with absolute path
  in ``pytest.main("-c your_absolute_path")``.

* Fix maximum recursion depth detection when raised error class is not aware
  of unicode/encoded bytes.
  Thanks `@prusse-martin`_ for the PR (`#1506`_).

* Fix ``pytest.mark.skip`` mark when used in strict mode.
  Thanks `@pquentin`_ for the PR and `@RonnyPfannschmidt`_ for
  showing how to fix the bug.

* Minor improvements and fixes to the documentation.
  Thanks `@omarkohl`_ for the PR.

* Fix ``--fixtures`` to show all fixture definitions as opposed to just
  one per fixture name.
  Thanks to `@hackebrot`_ for the PR.

.. _#510: pytest-dev/pytest#510
.. _#1506: pytest-dev/pytest#1506
.. _#1496: https://github.com/pytest-dev/pytest/issue/1496
.. _#1524: https://github.com/pytest-dev/pytest/issue/1524

.. _@prusse-martin: https://github.com/prusse-martin
.. _@astraw38: https://github.com/astraw38
@pyup-bot pyup-bot mentioned this pull request Sep 12, 2016
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.

2 participants