Fix compatibility with Twisted 25

[nicoddemus]

The issue arises because Failure.__init__ in Twisted 25 can receive a non-None exc_value while exc_tb is None. In such cases, ExceptionInfo[BaseException].from_exc_info fails, as it expects a traceback to be present when sys.exc_info() returns a tuple. This leads to the error message 'NoneType' object is not iterable.

Implemented separate workarounds for Twisted 24 and Twisted 25, the later one being simpler and less intrusive in the long run, as it does not require monkeypatching Failure.__init__.

We can drop suppor for Twisted 24 and keep the simpler patch in a later release.

Fixes #13497

[glyph]

Happy to see a fix developed so quickly and I hope this can land in a release soon.

That said, this is very fragile and likely to break again soon. turning Failure into a dataclass, making it frozen, giving it __slots__, rewriting it in Cython or Rust or something, etc, are all things that are likely to be done in service of speeding things up. I am pretty sure we avoid storing a raw exc_info like this intentionally; it's been quite a while but I seem to remember several very annoying memory leaks that would crop up with that strategy (hence Failure.cleanFailure())

[nicoddemus]

Hmmm test_trial_exceptions_with_skips did pass for me locally, will continue to investigate.

[nicoddemus]

Happy to see a fix developed so quickly and I hope this can land in a release soon.

As soon as we merge this and #13495 I will make a new release. 👍

That said, this is very fragile and likely to break again soon. turning Failure into a dataclass, making it frozen, giving it slots, rewriting it in Cython or Rust or something, etc, are all things that are likely to be done in service of speeding things up.

I definitely agree this is fragile. This workaround seems to be due to the fact that twisted calls TestCase.addError with twisted.python.Failure instead of the expected sys.exc_info, so it was decided to wrap its __init__ and store the original sys.exc_info.

I guess we can mitigate the problem a bit by storing the sys.exc_info somewhere else... 🤔

I am pretty sure we avoid storing a raw exc_info like this intentionally; it's been quite a while but I seem to remember several very annoying memory leaks that would crop up with that strategy (hence Failure.cleanFailure())

Well noted: I'm reducing the chances of a memory leak now by removing the attribute created by pytest.

But all this is definitely hacky.

PS: I'm a big fan of your blog. 😁

[glyph]

Happy to see a fix developed so quickly and I hope this can land in a release soon.

As soon as we merge this and #13495 I will make a new release. 👍

Sounds great, thanks.

That said, this is very fragile and likely to break again soon. turning Failure into a dataclass, making it frozen, giving it slots, rewriting it in Cython or Rust or something, etc, are all things that are likely to be done in service of speeding things up.

I definitely agree this is fragile. This workaround seems to be due to the fact that twisted calls TestCase.addError with twisted.python.Failure instead of the expected sys.exc_info, so it was decided to wrap its __init__ and store the original sys.exc_info.

I guess we can mitigate the problem a bit by storing the sys.exc_info somewhere else... 🤔

I probably need to wait for a less hectic day to reflect on this but can Exception.__traceback__ be used to reconstruct it?

I am pretty sure we avoid storing a raw exc_info like this intentionally; it's been quite a while but I seem to remember several very annoying memory leaks that would crop up with that strategy (hence Failure.cleanFailure())

Well noted: I'm reducing the chances of a memory leak now by removing the attribute created by pytest.

Ah, awesome, glad to hear it. I didn't catch that.

It's possible that this is less common with coroutines as it was with manually-managed Deferreds, but i guess it depends if there's a hard reference to the Failure in the traceback frames…

But all this is definitely hacky.

Yeah, I look forward to figuring out some better strategy for this, and I'm happy for twisted & trial to help out here.

PS: I'm a big fan of your blog. 😁

Thanks! Remember to like and subscribe 🙃

[nicoddemus]

this but can Exception.__traceback__ be used to reconstruct it?

That's a good idea, I will try it out later.

[nicoddemus]

Meanwhile this is ready for review @pytest-dev/core

[nicoddemus]

@glyph

this but can Exception.traceback be used to reconstruct it?

That approach did not work, because Failure.__traceback__ at that point is always None. However, this inspired me to try a different solution: extract the sys.exc_info() tuple directly from the Failure object, similar to what was being done in the version where we monkey-patch Failure.__init__.

This change significantly simplified the code:

1. It eliminates the need to monkey-patch Failure.__init__, making the code more future-proof.
2. Because of (1), we no longer need to patch Failure.__init__ during every runtest protocol. This allows to move the code that registers TestCaseFunction as an IReporter to pytest_configure. Previously, this required a global to ensure it was being done only once.

The code is much simpler and more isolated. However, it has two downsides:

1. I'm not entirely sure it is correct, although all tests are passing.
2. It does not work for Twisted <25: in Twisted 24.11.0, at the point addError is called, sys.exc_info() returns (None, None, None), which breaks the patch.

Because of the above, I see two options (@pytest-dev/core):

1. Release the simpler, shorter version, from the second commit. We need to advertise that it requires Twisted>=25, and might break in unexpected ways not caught by our test suite, given the patch is significantly different than the previous code.
2. Release the more direct fix from the first commit. This works for any Twisted version (🤞), but the code is more fragile, relying on monkey patching Failure.__init__; however, not worse than before.

[Zac-HD]

I weakly prefer option (2); breaking compat with older versions is usually a larger impact on users than picking up a fragile workaround - at least when there are active maintainers on all sides, which we have in this case.

[Pierre-Sassoulas]

Agree with Zac. And longterm we could switch to the simpler implementation when twisted download are for version > 25 for ~= 90% of users if twisted according to pypistats.

[glyph]

Agree with Zac. And longterm we could switch to the simpler implementation when twisted download are for version > 25 for ~= 90% of users if twisted according to pypistats.

Por que no los dos?

Specifically: add a toggle at runtime that chooses the simpler implementation when possible (i.e. when Twisted is new enough), and then by the time the old implementation gets deleted, it's just removing dead code, rather than transitioning everyone to a new, as-yet untested implementation all at once?

[nicoddemus]

Por que no los dos?

Good call, will do that.

[RonnyPfannschmidt]

Is there a way to support pre 25. Else this is a breaking change?

[nicoddemus]

Is there a way to support pre 25. Else this is a breaking change?

Indeed, but as @glyph suggested, will implement separate fixes depending on the Twisted version.

[RonnyPfannschmidt]

Should we use the version metadata from importlib?

[nicoddemus]

Should we use the version metadata from importlib?

Hopefully. 😁

[nicoddemus]

Both patches in place now. 👍

[nicoddemus]

Ready for review

[Pierre-Sassoulas]

Great work !

[nicoddemus]

I'm having trouble understanding why Codecov is reporting missing coverage.

Their diff indicates that the code related to Twisted 24 isn't executing:

However, the log files from the py39-unittest-twisted24 job don't show those specific lines as missing:

src\_pytest\unittest.py                276     19     74     10    90%   90, 122, 144-145, 146->155, 157->160, 185-186, 187->189, 190->exit, 215, 262, 320, 423, 490-500

Indeed, lines 490-500 (in the log above) pertain to the fix for Twisted 25, which is expected behavior for the twisted24 job. Moreover, these lines appear to be covered in the screenshot provided.

Also it is clear that coverage is being uploaded.

Given that this fix is long overdue, I'll proceed with merging it.

[patchback]

Backport to 8.4.x: 💚 backport PR created

✅ Backport PR branch: patchback/backports/8.4.x/01dce85a89eb0b3e881303784267f14b94d9691f/pr-13502

Backported as #13531

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.