Fix test_exceptions_dont_leak in Python 3.12 #535
Conversation
| @@ -65,6 +65,8 @@ def fail_if_exceptions_occurred(self, when): | |||
| prefix = "%s ERROR: " % when | |||
| msg = prefix + format_captured_exceptions(exceptions) | |||
| del exceptions[:] # Don't keep exceptions alive longer. | |||
| if hasattr(sys, "last_exc"): | |||
| sys.last_exc = None | |||
There was a problem hiding this comment.
I don't think this is correct... 🤔
There was a problem hiding this comment.
I think it's intentional now in 3.12 that this last_exc exists. Perhaps it would be better to raise another exception without a reference to the widget in the test instead?
There was a problem hiding this comment.
Ahh yes, I wondered the same thing -- see the PR description.
However the problem is not that the exception references the widget, but that the exception references the traceback, which in turn references the locals, which contains a reference to the widget.
That was reported originally in #187.
Perhaps we should update the test to ensure the internal exceptions are cleared, but it seems now the "leak" will happen because of Python, not because of pytest-qt.
There was a problem hiding this comment.
Will sleep on it, meanwhile suggestions are welcome. 👍
There was a problem hiding this comment.
I've been thinking about this, and it seems that clearing sys.last_exc is probably OK. This is only done in the context of reporting that a test failed with an exception, and then clearing the exception information before the next test is performed. So clearing sys.last_exc as well would make sense. If there is some context in which preserving this might be wanted, there is a clunky way to do it: have a pytest marker saying save_last_exc, and don't clear the exception if that is set. But my guess is that any developer knowledgeable enough to use the Python debugger to understand why a test has failed with an exception has been triggered will be able to insert a breakpoint before this line and trace from there.
Alternatively, if it doesn't matter that the object remains accessible just through this sys.last_exc reference, then this code could be left unaltered and just drop the failing test.
|
Thought about this some more, I think this is fine because we are clearing the attribute past the lifetime of the test, so it should be fine and goes with the purpose of the original issue of avoiding leaking data from one test to another. We can revisit this if it proves problematic in the future. Thanks everyone for chiming in. 👍 |
I wonder if clearing it is the right call, after all the purpose of
sys.last_excis for post-mortem debugging -- however I'm not sure that at that point is possible to breakpoint at that point.Fixes #532