Repository: pytest-dev/pytest
Pull request: Fix `pytrace=False` and `--tb=line` reports `None`
<!--
Thanks for submitting a PR, your contribution is really appreciated!

Here is a quick checklist that should be present in PRs.

- [ ] Include documentation when adding new features.
- [ ] Include new tests or update existing tests when applicable.
- [X] Allow maintainers to push and squash when merging my commits. Please uncheck this if you prefer to squash the commits yourself.

If this change fixes an issue, please:

- [ ] Add text like ``closes #XYZW`` to the PR description and/or commits (where ``XYZW`` is the issue number). See the [github docs](https://help.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword) for more information.

Unless your change is trivial or a small documentation fix (e.g., a typo or reword of a small section) please:

- [ ] Create a new changelog file in the `changelog` folder, with a name like `<ISSUE NUMBER>.<TYPE>.rst`. See [changelog/README.rst](https://github.com/pytest-dev/pytest/blob/main/changelog/README.rst) for details.

  Write sentences in the **past or present tense**, examples:

  * *Improved verbose diff output with sequences.*
  * *Terminal summary statistics now use multiple colors.*

  Also make sure to end the sentence with a `.`.

- [ ] Add yourself to `AUTHORS` in alphabetical order.
-->

Closes #10831.

This fixes a small bug where running tests that contained `pytest.fail(pytrace=False)` with the `--tb=line` flag set results in an output of "None" in the Failures section of the output, and adds a test to ensure the behavior is correct. 

First time contributor so any advice/suggestions are welcome and encouraged!

Problem statement:
`pytrace=False` and "--tb=line" reports "None"
Given test changed as
```diff
 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
```diff
 == FAILURES ==
-.../test_foo.py:4: Failed: my message
+None
```

**Expected:** Output remains the same
```
== FAILURES ==
.../test_foo.py:4: Failed: my message
```

Hints from issue discussion:
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.
> 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.
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.
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`.
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.
@nicoddemus I'd expect the output to remain the same (updated in the issue body):
```
.../test_foo.py:4: Failed: my message
```

Commit message: Update test to confirm expected behavior

Patch to review:
diff --git a/AUTHORS b/AUTHORS
index 1aa5265e62e..dee73514a59 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -13,6 +13,7 @@ Ahn Ki-Wook
 Akiomi Kamakura
 Alan Velasco
 Alessio Izzo
+Alex Jones
 Alexander Johnson
 Alexander King
 Alexei Kozlenok
diff --git a/changelog/10831.bugfix.rst b/changelog/10831.bugfix.rst
new file mode 100644
index 00000000000..ea641dee60e
--- /dev/null
+++ b/changelog/10831.bugfix.rst
@@ -0,0 +1 @@
+Terminal Reporting: Fixed bug when running in ``--tb=line`` mode where ``pytest.fail(pytrace=False)`` tests report ``None``.
diff --git a/src/_pytest/_code/code.py b/src/_pytest/_code/code.py
index 032b83beb02..aa953d98565 100644
--- a/src/_pytest/_code/code.py
+++ b/src/_pytest/_code/code.py
@@ -627,7 +627,7 @@ def getrepr(
             Ignored if ``style=="native"``.
 
         :param str style:
-            long|short|no|native|value traceback style.
+            long|short|line|no|native|value traceback style.
 
         :param bool abspath:
             If paths should be changed to absolute or left unchanged.
diff --git a/src/_pytest/nodes.py b/src/_pytest/nodes.py
index c74740dbc8a..b9af84c126e 100644
--- a/src/_pytest/nodes.py
+++ b/src/_pytest/nodes.py
@@ -445,7 +445,7 @@ def _repr_failure_py(
         if isinstance(excinfo.value, ConftestImportFailure):
             excinfo = ExceptionInfo.from_exc_info(excinfo.value.excinfo)
         if isinstance(excinfo.value, fail.Exception):
-            if not excinfo.value.pytrace:
+            if not excinfo.value.pytrace and style != "line":
                 style = "value"
         if isinstance(excinfo.value, FixtureLookupError):
             return excinfo.value.formatrepr()
diff --git a/testing/test_terminal.py b/testing/test_terminal.py
index fe325b72d29..ca683283499 100644
--- a/testing/test_terminal.py
+++ b/testing/test_terminal.py
@@ -1539,6 +1539,20 @@ def test_func2():
         s = result.stdout.str()
         assert "def test_func2" not in s
 
+    def test_tb_crashline_pytrace_false(self, pytester: Pytester, option) -> None:
+        p = pytester.makepyfile(
+            """
+            import pytest
+            def test_func1():
+                pytest.fail('test_func1', pytrace=False)
+        """
+        )
+        result = pytester.runpytest("--tb=line")
+        s = result.stdout.str()
+        assert "None" not in s
+        bn = p.name
+        result.stdout.fnmatch_lines(["*%s:3: Failed: test_func1" % bn])
+
     def test_pytest_report_header(self, pytester: Pytester, option) -> None:
         pytester.makeconftest(
             """


Code review comments:
diff_hunk: @@ -445,7 +445,7 @@ def _repr_failure_py(
         if isinstance(excinfo.value, ConftestImportFailure):
             excinfo = ExceptionInfo.from_exc_info(excinfo.value.excinfo)
         if isinstance(excinfo.value, fail.Exception):
-            if not excinfo.value.pytrace:
+            if not excinfo.value.pytrace and style != "line":
path: src/_pytest/nodes.py
text: @user1:
The reason `"value"` doesn't end up working for this seems to be this line

https://github.com/pytest-dev/pytest/blob/7834b3b07f7a7b2409331a8f14141c6ee8546215/src/_pytest/_code/code.py#L980-L982

which says that for `style = "value"`, the reprcrash is None. The reprcrash is what `--tb=line` uses:

https://github.com/pytest-dev/pytest/blob/7834b3b07f7a7b2409331a8f14141c6ee8546215/src/_pytest/terminal.py#L1011-L1014

this code is currently the only place where `--tb=line` condition appears which seems like a nice property to me.

So what I wonder is why "value" doesn't want a reprcrash. This comes from 94c7b8b47cd6b5b14f463731e473929b42881073 which introduced "line". I think the idea was to maintain the previous behavior where there was also no reprcrash for `pytrace=False`.

What is `reprcrash` anyway? TBH I'm not sure (pytest is full of mysteries like that). [...After looking at the code...] It seems to be used for the following:

1. For `--tb=line`. This is the one we want to affect so :heavy_check_mark: 
2. Shown in the short test summary info. Seems good to show the reason message here so :heavy_check_mark: .
3. In junitxml output. Not an expert but the change seems pretty minor - adding the `Failed: ` prefix. :heavy_check_mark: 
4. Some other irrelevant stuff to "value" (skips, KeyboardInterruprt). :heavy_check_mark: 

So I think instead of this change, we can replace

```py
reprcrash: Optional[ReprFileLocation] = ( 
    excinfo_._getreprcrash() if self.style != "value" else None 
) 
```

to

```py
reprcrash = excinfo_._getreprcrash()
```

Can you try this?

@author:
Didn't realize we were on the cutting edge of development here:
https://github.com/pytest-dev/pytest/blob/7834b3b07f7a7b2409331a8f14141c6ee8546215/src/_pytest/_code/code.py#L980-L982
was added as of the latest release, one day after I authored my PR!

I removed my changes and changed those three lines to
```
reprcrash = excinfo_._getreprcrash()
```
and all of the tests, including the one I added, passed! Pushing the changes in a new commit now.

@user1:
Yes we've recently had some crash due to reprcrash :)