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

Please make verbose XFAIL tracebacks optional #12231

Closed
mgorny opened this issue Apr 21, 2024 · 17 comments
Closed

Please make verbose XFAIL tracebacks optional #12231

mgorny opened this issue Apr 21, 2024 · 17 comments

Comments

@mgorny
Copy link
Contributor

mgorny commented Apr 21, 2024

What's the problem this feature will solve?

Since #11735, -rx causes pytest to output tracebacks for expected failures. This is sometimes undesirable with projects that have a lot of xfail-ing tests. For example, with pandas outputting XFAIL tracebacks takes a few minutes and results in 80M of output.

Describe the solution you'd like

I think the cleanest solution would be to have another option to control whether tracebacks are displayed for XFAILs. This would make it possible to retain XFAIL in summary output (via -rx or -ra), without the extra overhead of printing all the tracebacks.

Alternative Solutions

We could avoid passing -rx, but then we'd lose the list of XFAILs from test summary, and that one is actually desirable.

Additional context

n/a

@nicoddemus
Copy link
Member

@mgorny I agree, for projects with a lot of xfail tests it produces too much output.

Do you have a suggestion of what the option would be?

@mgorny
Copy link
Contributor Author

mgorny commented Apr 21, 2024

I'm terrible at naming, so I'd go for --xfail-verbose=.

@nicoddemus
Copy link
Member

nicoddemus commented Apr 21, 2024

Oh you mean a separate flag? I would prefer not.

Perhaps we can change the feature itself to require a separate -r letter to show the xfail tracebacks. It would be a "breaking change" from #11735/#11574, but perhaps it would be for the best.

cc @okken @sturmf

@mgorny
Copy link
Contributor Author

mgorny commented Apr 21, 2024

Yeah, sure, that would work for me too.

@okken
Copy link
Contributor

okken commented Apr 21, 2024

I’d be ok with a separate -r letter.

Also, if a separate option, --xfail-tb with no/short/long, etc, like the existing --tb flag, wouldn’t be terrible. But it might be harder to implement.

@kpfleming
Copy link

Yes, please! I just spent 10 minutes trying to track down why my CI job was reporting a failure, and assumed it was because of the XFAIL tracebacks (but it was not, coverage dropped below the required threshold).

@nicoddemus
Copy link
Member

I like --xfail-tb. If it is simple to implement no/short/long etc that is good, if not, the first implementation can just enable tracebacks when --xfail-tb is passed, then later we can improve on that to allow the no/short/long option.

Would love to see a PR in this direction -- should be a simple extension of the work done in #11574.

@okken
Copy link
Contributor

okken commented Apr 27, 2024

I can take a look, probably next week.

@okken
Copy link
Contributor

okken commented Apr 28, 2024

Initial implementation looking pretty good so far.
I've added --xfail=tb flag, which defaults to auto.

So turning off xfail tracebacks could be done with --xfail-tb=no.
I'll probably regularly run with --xfail-tb=short.

@nicoddemus Should we default --xfail-tb to no?
It seems more natural to me to leave it defaulting to auto, like --tb, but I'm really ok with defaulting off.
That would be a behavior break, though.

@bluetech
Copy link
Member

It seems more natural to me to leave it defaulting to auto, like --tb, but I'm really ok with defaulting off.
That would be a behavior break, though.

My opinion is to default to off, because xfails are expected so their traceback is not normally interesting.

@nicoddemus
Copy link
Member

I agree with defaulting to no. Even if it is a behavior change, I think it is for the best, I consider it was not a good decision to leave it on by default, after seeing it working in the wild. I think in this case it makes sense because we will be reverting to the previous and expected behavior.

@okken
Copy link
Contributor

okken commented Apr 28, 2024

Thanks everyone for your rapid feedback on this.

@okken
Copy link
Contributor

okken commented Apr 29, 2024

Ok. This is more difficult than I imagined at first.
tbstyle and functions that use it are kinda all over the code base.

Perhaps a change of plans.

How about, we leave the styling of tracebacks to --tb and just have --xfail-tb be a boolean, and it just controls if they on or off?

I've modified the changelog entry, tests, and code to treat --xfail-tb as a boolean flag. It's cleaner and less invasive to the code base than trying to have it behave like --tb with independent styles.

@nicoddemus
Copy link
Member

How about, we leave the styling of tracebacks to --tb and just have --xfail-tb be a boolean, and it just controls if they on or off?

I agree, that was what I meant by this:

if not, the first implementation can just enable tracebacks when --xfail-tb is passed, then later we can improve on that to allow the no/short/long option.

I see you closed the PR, was that intentional?

@okken
Copy link
Contributor

okken commented May 2, 2024

No. Not intentional. I’ll try to recover PR Later tonight.

okken added a commit to okken/pytest that referenced this issue May 2, 2024
@okken
Copy link
Contributor

okken commented May 2, 2024

Accidentally deleted my fork, along with this branch.
I've recreated the PR as #12280

@mgorny
Copy link
Contributor Author

mgorny commented May 7, 2024

Thanks a lot!

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

No branches or pull requests

5 participants