Fix handle repr error with showlocals and verbose output #5133

kondratyev-nv · 2019-04-16T05:25:32Z

RonnyPfannschmidt

How does this change the display style in case of an error value?

codecov · 2019-04-16T05:46:59Z

Codecov Report

Merging #5133 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #5133      +/-   ##
==========================================
+ Coverage   96.05%   96.07%   +0.01%     
==========================================
  Files         114      114              
  Lines       25784    25812      +28     
  Branches     2550     2550              
==========================================
+ Hits        24768    24798      +30     
+ Misses        705      704       -1     
+ Partials      311      310       -1

Impacted Files	Coverage Δ
src/_pytest/_io/saferepr.py	`81.25% <100%> (+2.18%)`	⬆️
testing/test_session.py	`100% <100%> (ø)`	⬆️
src/_pytest/_code/code.py	`95.53% <100%> (+0.01%)`	⬆️
testing/code/test_excinfo.py	`96.82% <100%> (+0.03%)`	⬆️
testing/acceptance_test.py	`98.03% <0%> (+0.39%)`	⬆️

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 9eac473...d99d859. Read the comment docs.

codecov · 2019-04-16T05:47:00Z

Codecov Report

Merging #5133 into master will increase coverage by <.01%.
The diff coverage is 98.24%.

@@            Coverage Diff             @@
##           master    #5133      +/-   ##
==========================================
+ Coverage   96.06%   96.06%   +<.01%     
==========================================
  Files         114      114              
  Lines       25790    25825      +35     
  Branches     2550     2550              
==========================================
+ Hits        24776    24810      +34     
- Misses        704      705       +1     
  Partials      310      310

Impacted Files	Coverage Δ
src/_pytest/_io/saferepr.py	`80.43% <100%> (+1.36%)`	⬆️
testing/test_session.py	`100% <100%> (ø)`	⬆️
src/_pytest/_code/code.py	`95.5% <100%> (-0.02%)`	⬇️
testing/code/test_excinfo.py	`96.76% <95.65%> (-0.03%)`	⬇️

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 e87d3d7...bc00d0f. Read the comment docs.

RonnyPfannschmidt · 2019-04-16T05:57:42Z

nevermind, i missed a key part of the invocation site, its fine

RonnyPfannschmidt · 2019-04-16T05:59:10Z

src/_pytest/_io/saferepr.py

+    try:
+        # Try the vanilla repr and make sure that the result is a string
+        return call(x, *args)
+    except Exception:


if the trackback is not used, just use except Exception as e

Just to make it clear, I have not changed this part, I've moved it from _callhelper. I thought exc_info was used to get the class of an exception. Would it be ok to use e.__class__.__name__ instead?

@RonnyPfannschmidt Can you have a look, please? With e.__class__.__name__ it looks like

try: exc_name = e.__class__.__name__ except Exception: exc_name = "unknown" try: exc_info = str(e) except Exception: exc_info = "unknown"

Makes sense. I'd use as exc though.

blueyed

Thanks for working on this. Please see my comments.

blueyed · 2019-04-17T12:48:31Z

src/_pytest/_code/code.py

@@ -676,7 +679,7 @@ def repr_locals(self, locals):
                    if self.truncate_locals:
                        str_repr = self._saferepr(value)
                    else:
-                        str_repr = pprint.pformat(value)
+                        str_repr = self._safeformat(value)


Why not use it directly?
I.e. not via _safeformat method - I see that this is done for _saferepr, but I do not think it is good to follow.

Yeah, I've just followed the way it has been called. I will change this to a direct call of safeformat and saferepr. Thank you.

src/_pytest/_io/saferepr.py

blueyed · 2019-04-17T12:52:56Z

src/_pytest/_io/saferepr.py

 import sys

 from six.moves import reprlib


+def _safe_format_call(call, x, *args):


This should be renamed to something more explicit like _call_and_format_exception.

blueyed · 2019-04-17T12:54:32Z

src/_pytest/_io/saferepr.py

+    try:
+        # Try the vanilla repr and make sure that the result is a string
+        return call(x, *args)
+    except Exception:


Makes sense. I'd use as exc though.

blueyed · 2019-04-18T21:51:40Z

src/_pytest/_io/saferepr.py

+        try:
+            exc_name = exc.__class__.__name__
+        except Exception:
+            exc_name = "unknown"


This is not covered in tests (https://codecov.io/gh/pytest-dev/pytest/compare/9eac4733c56cd1bac8abb6639e19976710fee139...4024a121e446f348c5ea839e002f3362b8c4978a/diff#D1-13).
And I think it is not possible to get an exception via this, is it? (/cc @asottile)

sadly it is:

class CryParty(object): @property def __class__(self): raise TypeError('boom!')

>>> CryParty() <__main__.CryParty object at 0x7f2e3c41b8d0> >>> CryParty().__class__ Traceback (most recent call last): File "<stdin>", line 1, in <module> File "t.py", line 4, in __class__ raise TypeError('boom!') TypeError: boom! >>> type(CryParty()).__name__ 'CryParty'

Awesome! Thank you so much! I saw a decrease in the coverage, and I was looking without any luck for the whole evening to find a way to raise an exception with __class__. So I was even going to remove try .. except block.

asottile

looks good! maybe add a test with my broken __class__ example, and switch to type(...).__name__ since we don't need to support old-style-classes for exceptions (py27+)

asottile · 2019-04-18T22:13:40Z

src/_pytest/_io/saferepr.py

+        try:
+            exc_name = exc.__class__.__name__
+        except Exception:
+            exc_name = "unknown"


sadly it is:

class CryParty(object): @property def __class__(self): raise TypeError('boom!')

>>> CryParty() <__main__.CryParty object at 0x7f2e3c41b8d0> >>> CryParty().__class__ Traceback (most recent call last): File "<stdin>", line 1, in <module> File "t.py", line 4, in __class__ raise TypeError('boom!') TypeError: boom! >>> type(CryParty()).__name__ 'CryParty'

asottile · 2019-04-18T22:14:35Z

testing/test_session.py

@@ -134,6 +134,30 @@ def test_implicit_bad_repr1(self):
            != -1
        )

+    def test_broken_repr_with(self, testdir):


with_showlocals_verbose perhaps?

kondratyev-nv · 2019-04-19T06:45:54Z

I have not found any way for type(exc).__name__ to raise an exception, so I've removed the corresponding try .. except block in _call_and_format_exception in src/_pytest/_io/saferepr.py. If there is any possibility for this code to fail, I will be glad to return it and add more tests.

asottile

🎉 this is great!

blueyed

Awesome.

Please squash the commits and force-push.

blueyed · 2019-04-19T17:23:48Z

Thank you!

RonnyPfannschmidt reviewed Apr 16, 2019

View reviewed changes

RonnyPfannschmidt approved these changes Apr 16, 2019

View reviewed changes

RonnyPfannschmidt reviewed Apr 16, 2019

View reviewed changes

blueyed suggested changes Apr 17, 2019

View reviewed changes

blueyed reviewed Apr 18, 2019

View reviewed changes

asottile reviewed Apr 18, 2019

View reviewed changes

asottile approved these changes Apr 19, 2019

View reviewed changes

blueyed approved these changes Apr 19, 2019

View reviewed changes

nicoddemus approved these changes Apr 19, 2019

View reviewed changes

Fix handle repr error with showlocals and verbose output

bc00d0f

kondratyev-nv force-pushed the fix-handle-repr-error-with-showlocals branch from b9dbe2c to bc00d0f Compare April 19, 2019 15:55

blueyed merged commit 34bc594 into pytest-dev:master Apr 19, 2019

kondratyev-nv deleted the fix-handle-repr-error-with-showlocals branch April 20, 2019 07:02

Uh oh!

Fix handle repr error with showlocals and verbose output #5133

Fix handle repr error with showlocals and verbose output #5133

Uh oh!

Conversation

kondratyev-nv commented Apr 16, 2019

Uh oh!

RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Apr 16, 2019

Codecov Report

Uh oh!

codecov bot commented Apr 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

RonnyPfannschmidt commented Apr 16, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blueyed left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asottile left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kondratyev-nv commented Apr 19, 2019

Uh oh!

asottile left a comment

Choose a reason for hiding this comment

Uh oh!

blueyed left a comment

Choose a reason for hiding this comment

Uh oh!

blueyed commented Apr 19, 2019

Uh oh!

Uh oh!

codecov bot commented Apr 16, 2019 •

edited

Loading