Repository: pytest-dev/pytest
Pull request: Issue 1316 - longrepr is a string when pytrace=False
This pull request fixes #1316. I've added new `_TracebackStyle` - `no_pytrace`. For that style `ReprExceptionInfo` has as few information as possible. For example `report.longrepr.chain` is
```
[(ReprTraceback(reprentries=[ReprEntry(lines=['Hello World'], reprfuncargs=None, reprlocals=None, reprfileloc=None, style='no_pytrace')], extraline=None, style='no_pytrace'), None, None)]
```
for code contained in issue #1316.

Problem statement:
TestReport.longrepr is a string when pytest.fail(..., pytrace=False) is used
I'd expect the `assert` in my `conftest.py` to pass, but it doesn't:

``` python
# test_foo.py

import pytest

def test_foo(foo):
    pytest.fail("Hello World", pytrace=False)
```

``` python
# conftest.py
import pytest

@pytest.hookimpl(hookwrapper=True)
def pytest_runtest_makereport(item, call):
    outcome = yield
    report = outcome.get_result()
    assert not isinstance(report.longrepr, str)
```

Without `pytrace=False`, it's a `py._code.code.ReprExceptionInfo` instance, which means I can use `report.longrepr.addsection` to add additional information.

With `pytrace=False`, it's a `str`, and even appending something to that string won't get pytest to show the output I'd like to show.

Hints from issue discussion:
I've also faced this situation in some other occasion... it would be nice to have `longrepr` always be an `ReprExceptionInfo` instance. Hopefully this will be easier to fix once we merge `py,code` in.

imho longrepr is a bit problematic, i hope we can get reporting work different in the long term

Commit message: Minor adjustment to CHANGELOG

Patch to review:
diff --git a/changelog/1316.breaking.rst b/changelog/1316.breaking.rst
new file mode 100644
index 00000000000..4c01de728e8
--- /dev/null
+++ b/changelog/1316.breaking.rst
@@ -0,0 +1 @@
+``TestReport.longrepr`` is now always an instance of ``ReprExceptionInfo``. Previously it was a ``str`` when a test failed with ``pytest.fail(..., pytrace=False)``.
diff --git a/src/_pytest/_code/code.py b/src/_pytest/_code/code.py
index 6102084f0b8..82b193e2104 100644
--- a/src/_pytest/_code/code.py
+++ b/src/_pytest/_code/code.py
@@ -46,7 +46,7 @@
     from typing_extensions import Literal
     from weakref import ReferenceType  # noqa: F401
 
-    _TracebackStyle = Literal["long", "short", "line", "no", "native"]
+    _TracebackStyle = Literal["long", "short", "line", "no", "native", "value"]
 
 
 class Code:
@@ -587,7 +587,7 @@ def getrepr(
             Show locals per traceback entry.
             Ignored if ``style=="native"``.
 
-        :param str style: long|short|no|native traceback style
+        :param str style: long|short|no|native|value traceback style
 
         :param bool abspath:
             If paths should be changed to absolute or left unchanged.
@@ -762,16 +762,19 @@ def repr_locals(self, locals: Dict[str, object]) -> Optional["ReprLocals"]:
     def repr_traceback_entry(
         self, entry: TracebackEntry, excinfo: Optional[ExceptionInfo] = None
     ) -> "ReprEntry":
-        source = self._getentrysource(entry)
-        if source is None:
-            source = Source("???")
-            line_index = 0
-        else:
-            line_index = entry.lineno - entry.getfirstlinesource()
-
         lines = []  # type: List[str]
-        style = entry._repr_style if entry._repr_style is not None else self.style
+        style = (
+            entry._repr_style
+            if entry is not None and entry._repr_style is not None
+            else self.style
+        )
         if style in ("short", "long"):
+            source = self._getentrysource(entry)
+            if source is None:
+                source = Source("???")
+                line_index = 0
+            else:
+                line_index = entry.lineno - entry.getfirstlinesource()
             short = style == "short"
             reprargs = self.repr_args(entry) if not short else None
             s = self.get_source(source, line_index, excinfo, short=short)
@@ -785,7 +788,10 @@ def repr_traceback_entry(
             localsrepr = self.repr_locals(entry.locals)
             return ReprEntry(lines, reprargs, localsrepr, reprfileloc, style)
         if excinfo:
-            lines.extend(self.get_exconly(excinfo, indent=4))
+            if style == "value":
+                lines.extend(str(excinfo.value).split("\n"))
+            else:
+                lines.extend(self.get_exconly(excinfo, indent=4))
         return ReprEntry(lines, None, None, None, style)
 
     def _makepath(self, path):
@@ -810,6 +816,11 @@ def repr_traceback(self, excinfo: ExceptionInfo) -> "ReprTraceback":
 
         last = traceback[-1]
         entries = []
+        if self.style == "value":
+            reprentry = self.repr_traceback_entry(last, excinfo)
+            entries.append(reprentry)
+            return ReprTraceback(entries, None, style=self.style)
+
         for index, entry in enumerate(traceback):
             einfo = (last == entry) and excinfo or None
             reprentry = self.repr_traceback_entry(entry, einfo)
@@ -869,7 +880,9 @@ def repr_excinfo(self, excinfo: ExceptionInfo) -> "ExceptionChainRepr":
             seen.add(id(e))
             if excinfo_:
                 reprtraceback = self.repr_traceback(excinfo_)
-                reprcrash = excinfo_._getreprcrash()  # type: Optional[ReprFileLocation]
+                reprcrash = (
+                    excinfo_._getreprcrash() if self.style != "value" else None
+                )  # type: Optional[ReprFileLocation]
             else:
                 # fallback to native repr if the exception doesn't have a traceback:
                 # ExceptionInfo objects require a full traceback to work
@@ -1052,8 +1065,11 @@ def _write_entry_lines(self, tw: TerminalWriter) -> None:
                     "Unexpected failure lines between source lines:\n"
                     + "\n".join(self.lines)
                 )
-                indents.append(line[:indent_size])
-                source_lines.append(line[indent_size:])
+                if self.style == "value":
+                    source_lines.append(line)
+                else:
+                    indents.append(line[:indent_size])
+                    source_lines.append(line[indent_size:])
             else:
                 seeing_failures = True
                 failure_lines.append(line)
diff --git a/src/_pytest/junitxml.py b/src/_pytest/junitxml.py
index 77e1843127a..6a81bb608d0 100644
--- a/src/_pytest/junitxml.py
+++ b/src/_pytest/junitxml.py
@@ -202,10 +202,11 @@ def append_failure(self, report):
         if hasattr(report, "wasxfail"):
             self._add_simple(Junit.skipped, "xfail-marked test passes unexpectedly")
         else:
-            if hasattr(report.longrepr, "reprcrash"):
+            if (
+                hasattr(report.longrepr, "reprcrash")
+                and report.longrepr.reprcrash is not None
+            ):
                 message = report.longrepr.reprcrash.message
-            elif isinstance(report.longrepr, str):
-                message = report.longrepr
             else:
                 message = str(report.longrepr)
             message = bin_xml_escape(message)
diff --git a/src/_pytest/nodes.py b/src/_pytest/nodes.py
index d55e000340a..cc305a58658 100644
--- a/src/_pytest/nodes.py
+++ b/src/_pytest/nodes.py
@@ -335,7 +335,7 @@ def _repr_failure_py(
     ) -> Union[str, ReprExceptionInfo, ExceptionChainRepr, FixtureLookupErrorRepr]:
         if isinstance(excinfo.value, fail.Exception):
             if not excinfo.value.pytrace:
-                return str(excinfo.value)
+                style = "value"
         if isinstance(excinfo.value, FixtureLookupError):
             return excinfo.value.formatrepr()
         if self.config.getoption("fulltrace", False):
diff --git a/testing/test_runner.py b/testing/test_runner.py
index 00732d03b26..7b0b27a4b0b 100644
--- a/testing/test_runner.py
+++ b/testing/test_runner.py
@@ -1002,6 +1002,17 @@ def test_func():
         assert rep.capstdout == ""
         assert rep.capstderr == ""
 
+    def test_longrepr_type(self, testdir) -> None:
+        reports = testdir.runitem(
+            """
+            import pytest
+            def test_func():
+                pytest.fail(pytrace=False)
+        """
+        )
+        rep = reports[1]
+        assert isinstance(rep.longrepr, _pytest._code.code.ExceptionRepr)
+
 
 def test_outcome_exception_bad_msg() -> None:
     """Check that OutcomeExceptions validate their input to prevent confusing errors (#5578)"""
diff --git a/testing/test_skipping.py b/testing/test_skipping.py
index 8b1cdd527ad..aa4aae109ff 100644
--- a/testing/test_skipping.py
+++ b/testing/test_skipping.py
@@ -194,7 +194,7 @@ def test_func():
         assert len(reports) == 3
         callreport = reports[1]
         assert callreport.failed
-        assert callreport.longrepr == "[XPASS(strict)] nope"
+        assert str(callreport.longrepr) == "[XPASS(strict)] nope"
         assert not hasattr(callreport, "wasxfail")
 
     def test_xfail_run_anyway(self, testdir):


Code review comments:
diff_hunk: @@ -202,10 +202,11 @@ def append_failure(self, report):
         if hasattr(report, "wasxfail"):
             self._add_simple(Junit.skipped, "xfail-marked test passes unexpectedly")
         else:
-            if hasattr(report.longrepr, "reprcrash"):
+            if (
+                hasattr(report.longrepr, "reprcrash")
+                and report.longrepr.reprcrash is not None
+            ):
path: src/_pytest/junitxml.py
text: @user1:
```suggestion
            if getattr(report.longrepr, "reprcrash", None) is not None:
```

diff_hunk: @@ -762,16 +762,19 @@ def repr_locals(self, locals: Dict[str, object]) -> Optional["ReprLocals"]:
     def repr_traceback_entry(
         self, entry: TracebackEntry, excinfo: Optional[ExceptionInfo] = None
     ) -> "ReprEntry":
-        source = self._getentrysource(entry)
-        if source is None:
-            source = Source("???")
-            line_index = 0
-        else:
-            line_index = entry.lineno - entry.getfirstlinesource()
-
         lines = []  # type: List[str]
-        style = entry._repr_style if entry._repr_style is not None else self.style
+        style = (
+            entry._repr_style
+            if entry is not None and entry._repr_style is not None
path: src/_pytest/_code/code.py
text: @user1:
Can `entry` be `None` now? If so, the type of the `entry` parameter should be changed to `Optional[TracebackEntry]`. Though it would be quite strange to have a function named `repr_traceback_entry` accepts a `None` entry :thinking:

@author:
No, ``entry`` can't be ``None``. Sorry about that. Thank you @user1 for catching defect.

diff_hunk: @@ -785,7 +788,10 @@ def repr_traceback_entry(
             localsrepr = self.repr_locals(entry.locals)
             return ReprEntry(lines, reprargs, localsrepr, reprfileloc, style)
         if excinfo:
path: src/_pytest/_code/code.py
text: @user1:
I'd prefer if this is phrased as

```py
elif style == "value":
    if excinfo:
        lines.extend(str(excinfo.value).split("\n"))
    return ReprEntry(lines, None, None, None, style)
else:
    if excinfo:
        lines.extend(self.get_exconly(excinfo, indent=4))
   return ReprEntry(lines, None, None, None, style)
```

I think it shows the structure better and is more extensible.

@author:
Your construction really looks better. Thanks!