-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Fix pytrace=False and --tb=line reports None
#10905
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 pytrace=False and --tb=line reports None
#10905
Conversation
src/_pytest/nodes.py
Outdated
| 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": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason "value" doesn't end up working for this seems to be this line
pytest/src/_pytest/_code/code.py
Lines 980 to 982 in 7834b3b
| reprcrash: Optional[ReprFileLocation] = ( | |
| excinfo_._getreprcrash() if self.style != "value" else None | |
| ) |
which says that for style = "value", the reprcrash is None. The reprcrash is what --tb=line uses:
pytest/src/_pytest/terminal.py
Lines 1011 to 1014 in 7834b3b
| if self.config.option.tbstyle == "line": | |
| for rep in reports: | |
| line = self._getcrashline(rep) | |
| self.write_line(line) |
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 94c7b8b 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:
- For
--tb=line. This is the one we want to affect so ✔️ - Shown in the short test summary info. Seems good to show the reason message here so ✔️ .
- In junitxml output. Not an expert but the change seems pretty minor - adding the
Failed:prefix. ✔️ - Some other irrelevant stuff to "value" (skips, KeyboardInterruprt). ✔️
So I think instead of this change, we can replace
reprcrash: Optional[ReprFileLocation] = (
excinfo_._getreprcrash() if self.style != "value" else None
) to
reprcrash = excinfo_._getreprcrash()Can you try this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't realize we were on the cutting edge of development here:
pytest/src/_pytest/_code/code.py
Lines 980 to 982 in 7834b3b
| reprcrash: Optional[ReprFileLocation] = ( | |
| excinfo_._getreprcrash() if self.style != "value" else None | |
| ) |
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes we've recently had some crash due to reprcrash :)
bluetech
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @acjreno, LGTM. Will merge in the next few days if there are no other comments.
testing/test_terminal.py
Outdated
| ) | ||
| result = pytester.runpytest("--tb=line") | ||
| s = result.stdout.str() | ||
| assert "None" not in s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without the context of the issue we're fixing, it's a bit odd to test that specifically. I'd rather remove this assertion. It's not the "None" that troubled me, as the lack of the original message.
for more information, see https://pre-commit.ci
| """ | ||
| ) | ||
| result = pytester.runpytest("--tb=line") | ||
| result.stdout.str() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw @acjreno, this line probably does nothing
Closes #10831.
This fixes a small bug where running tests that contained
pytest.fail(pytrace=False)with the--tb=lineflag 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!