Skip to content
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

Add summary for xfails with -rxX option #11574

Merged
merged 18 commits into from
Jan 5, 2024

Conversation

sturmf
Copy link
Contributor

@sturmf sturmf commented Nov 1, 2023

This is an early implementation which closes #11233.

It duplicates the code of the passes and failures summary implementation right now but I didn't want to refactor it before I know if something like this could be accepted.

@nicoddemus
Copy link
Member

Hi @sturmf,

Thanks for this, yes definitely this would be something we want to incorporate. Please go ahead with the refactoring and tests.

Regarding this closing #11233, one aspect brought up in that issue is the traceback of xfail/xpass tests, which are not handled here, correct?

If so I'm fine with leaving that out from this PR, we just need to update the issue later to mention that aspect is still not implemented.

@okken
Copy link
Contributor

okken commented Nov 3, 2023

Hey @sturmf, cool to see you working on this.

@okken
Copy link
Contributor

okken commented Dec 22, 2023

@nicoddemus Regarding " traceback of xfail/xpass tests, which are not handled here, correct?"

  • xfail - tracebacks are handled in summary_xfailures()
  • xpass - there is no traceback, but the output is captured

The one thing I see missing is the assert output is not reported in the summary at the end, but the reason for xfail is
I propose reporting the assertion line, then adding a reason if there is one, like this:

XFAIL test name - assertion - reason
as in XFAIL test_xfail.py::test_xfail_reason - assert 1 == 2 - reason 1

In context with Fail and Xpass:

...
=============================== short test summary info ===============================
FAILED test_xfail.py::test_fail - assert 1 == 2
XFAIL test_xfail.py::test_xfail - assert 1 == 2
XFAIL test_xfail.py::test_xfail_reason - assert 1 == 2 - reason 1
XPASS test_xfail.py::test_xpass - reason 2
================== 1 failed, 1 passed, 2 xfailed, 1 xpassed in 0.05s ==================

@nicoddemus Should the reason be before the assert? Should a separator be more obvious, or is a dash fine?

@sturmf I have a modification for this, are you ok with me pushing to the branch?

The remaining work seems to be:

  • add test cases and modify existing test cases as necessary
  • refactor (I think this is optional, and could be done with a followup)

@okken
Copy link
Contributor

okken commented Dec 23, 2023

I went ahead and pushed a couple changes to the merge branch.. I hope everyone is cool with that.

@okken
Copy link
Contributor

okken commented Dec 23, 2023

Well, I tried to push some tests. and I think I broke the whole thing. My apologies.

@sturmf
Copy link
Contributor Author

sturmf commented Dec 23, 2023

@okken thanks for picking up that change! I hope I can get a bit to it during the Christmas holidays, if there is anything left to do.

@okken
Copy link
Contributor

okken commented Dec 23, 2023

Awesome. I started a new branch starting with mainline and adding your changes and mine, mostly so that I could update it freely without too much damage. :)
I see that I’ve broken some existing tests, so I’ll work on fixing those after Xmas.

@okken
Copy link
Contributor

okken commented Dec 26, 2023

I think #11735 is about ready to go. There's no refactor yet, but tests are added.

@okken
Copy link
Contributor

okken commented Jan 2, 2024

Attempted to merge my pr (#11735) into this one

@okken
Copy link
Contributor

okken commented Jan 2, 2024

Cool. I think this merge worked. Merging tests from my PR into this one.
I'll delete the other PR.

@nicoddemus What is left to get this merged into main?

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks folks for working on this.

Besides my comments, I think what is missing is a CHANGELOG feature entry describing this feature.

src/_pytest/terminal.py Outdated Show resolved Hide resolved
src/_pytest/terminal.py Outdated Show resolved Hide resolved
src/_pytest/terminal.py Show resolved Hide resolved
@okken
Copy link
Contributor

okken commented Jan 4, 2024

Ugghh. Looks like I broke pytest-sugar, I’ll take a look

@okken
Copy link
Contributor

okken commented Jan 4, 2024

@nicoddemus Ok. A not-the-best-refactor-but-one-that-doesn't-break-pytest-sugar is in place.
And a changelog snippet (feel free to reword).
And other code review questions answered. :)

Let me know if there's anything else.

@sturmf Thanks for getting the ball rolling on this.

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks folks, looks great!

changelog/11233.feature.rst Outdated Show resolved Hide resolved
@nicoddemus
Copy link
Member

@bluetech this is a fairly small change, do you think we should backport this to 8.0, given we only released a rc1 so far? Or would you rather play safe and release this in 8.1?

@nicoddemus nicoddemus merged commit 13eacda into pytest-dev:main Jan 5, 2024
24 checks passed
@bluetech
Copy link
Member

bluetech commented Jan 5, 2024

Fine by me to make an exception and backport it

@okken
Copy link
Contributor

okken commented Jan 6, 2024

That’s very exciting. Is there an approximate timeframe for 8.0 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

-rx and -rX should turn on output for xfail/xpass
4 participants