`pytrace=False` and "--tb=line" reports "None"

[ikonst]

Given test changed as

 import pytest

 def test_foo():
-    pytest.fail('my message')
+    pytest.fail('my message', pytrace=False) 

and invoked as

pytest --tb=line test_foo.py

results in output

 == FAILURES ==
-.../test_foo.py:4: Failed: my message
+None

Expected: Output remains the same

== FAILURES ==
.../test_foo.py:4: Failed: my message

[acjreno]

Hello, first-time contributor here, so I'm very open to any suggestions on any of the following!

The Issue

I've managed to track down this issue to lines 448 and 449 in the function _repr_failure_py in the nodes.py file:

if not excinfo.value.pytrace:
    style = "value"

It seems that the test case above creates an excinfo instance with value.pytrace set to false, which results in an override of the style to "value", even though the style parameter of _repr_failiure_py is correctly passed in as "line" at the beginning of the function.

Because the style is reset to "value", the repr_chain is appended to by the following code (Line 953-956 of code.py)

if self.style == "value":
    repr_chain += [(reprtraceback, None, descr)] # This runs, resulting in the None value for the test
else:
    repr_chain += [(reprtraceback, reprcrash, descr)]

This propagates through a trail of function calls, eventually causing the bug in a call to _getcrashline in terminal.py that fails to throw an AttributeError here:

try: 
    return str(rep.longrepr.reprcrash) # this returns str(None) = "None", not the expected AttributeError
except AttributeError:
    try:
        return str(rep.longrepr)[:50] # this contains the expected value "foo", but doesn't currently run
    except AttributeError:
        return ""

Suggested Fix

The easiest solution I could find is to add a simple check in nodes.py that prevents the "line" style from being overridden when excinfo.value.pytrace is False, i.e.

if not excinfo.value.pytrace and style != "line":
    style = "value"

I'm planning on opening a PR for this issue tomorrow (as soon as I can get a test or two set up to double-check this works) but of course any feedback is welcome in the meantime.

[ikonst]

causing the bug in a call to _getcrashline in terminal.py that fails to throw an AttributeError here

I don't think it would be right to call AttributeError "expected". It's fallback code in case something goes wrong with regard to reprcrash being available.

[ikonst]

I'd probably do something like:

• change _getcrashline to return None or '' (rather than "None") when rep.longrepr.reprcrash is None`
• replace line = self._getcrashline(rep) with line = self._getcrashline(rep) or rep.longreprtext

I'm not a core contributor though, so just my opinion here.

[acjreno]

Very good point on the expectedness of the AttributeError, definitely not intended behavior! I did some extra checking to ensure that my suggested change wasn't actually causing that error to be thrown.

However, I think it might still be better to make sure that _getcrashline returns a non-None value for rep.longrepr.reprcrash by making sure that the Node style isn't overridden earlier on in the execution, as it seems to better match the intended behavior that can be observed when pytrace=True.

rep.longreprtext seems to contain different values ("E" in my testing for pytest.fail), and returning None from _getcrashline throws errors while returning '' would cause an empty line to be printed for each test with pytrace=False.

[nicoddemus]

Hey folks,

Thanks for the discussion!

But before we think on a fix, what did you expect the result would be in that case? I agree None is certainly not what one expects, but explicitly stating what you expect would be a good first step before working on a solution.

[ikonst]

@nicoddemus I'd expect the output to remain the same (updated in the issue body):

.../test_foo.py:4: Failed: my message

[bluetech]

I think the None is a bug, but the expected output is my message, not .../test_foo.py:4: Failed: my message. That's how pytrace=False behaves for the other --tb values.

[acjreno]

I see you've done some similar work on issues related to this in the past @bluetech, and looking into it I definitely agree that seems to be the norm for other --tb values.

I would assume that pytrace=False takes precedence over --tb=line (pytrace: "If False, msg represents the full failure information and no python traceback will be reported.", according to the API) since it is specified on a per-test-case basis, which I can definitely update my PR to reflect if that's the case.

[bluetech]

Looking at it again, I take my previous comment back. I think --tb=line should take precedence over pytrace=False, otherwise it's entirely unclear which test failed. With --tb=line the .../test_foo.py:4: is absolutely necessary because there's no other info pointing to the test. So your output seems good 👍. I'll go review the PR now.