search

nvim-neotest / neotest-python

MIT License
115 stars 34 forks source link

pytest: implement pytest_runtest_makereport to decouple exception handling from --tb argument value #27

Closed OddBloke closed 1 year ago

OddBloke commented 1 year ago

And a minor improvement to error output.

rcarriga commented 1 year ago

Thanks for the PR! Do you have an example that can cause a different type of exception, just so I can verify?

OddBloke commented 1 year ago

Thanks for the PR! Do you have an example that can cause a different type of exception, just so I can verify?

It looks like it's to do with the traceback format configured, let me see if I can make a minimal reproducer.

OddBloke commented 1 year ago

Yep, that's it, adding a pytest.ini which changes the traceback output should reproduce the error I was seeing:

[pytest]
addopts=--tb=native
OddBloke commented 1 year ago

I've tested with all the --tb options: long and short work with current trunk, this PR adds support for native: native shows the error against the test def line, not the line of the assertion failure. Both line and no cause tracebacks either way.

Perhaps neotest-python should specify --tb long when it invokes pytest? (I think this PR is still worth landing: if people are reconfiguring --tb via a plugin or something, then the CLI specified setting might be ignored, resulting in the other data structures being returned.)

OddBloke commented 1 year ago

And I just pushed up a commit which fixes the line and no exceptions.

OddBloke commented 1 year ago

I spent a bit longer looking at this, and found a better solution which sidesteps the value of --tb entirely: I've just pushed that up.

OddBloke commented 1 year ago

(We could actually take this further and move the entire implementation into pytest_runtest_makereport: I'll let you review this code before proposing yet more changes!)

rcarriga commented 1 year ago

Very cool, thanks for all of this work!

(We could actually take this further and move the entire implementation into pytest_runtest_makereport: I'll let you review this code before proposing yet more changes!)

I'm curious to hear more, is the advantage of this just to remove the need for storing state between makereport and logreport or is there something else to it? I'd probably be interested in making the change (depending on complexity) for just that but want to make sure I understand as well

OddBloke commented 1 year ago

I've opened #29 with the switch of hook: I htink it's simpler than what I've done here, so I'm gonna close this out in favour of that. (We can always reopen!)