classification
Title: object lifetime fragility in unittest tests
Type: behavior Stage: resolved
Components: Versions: Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: iritkatriel, pitrou, rbcollins
Priority: normal Keywords: patch

Created on 2014-10-30 09:57 by rbcollins, last changed 2021-09-02 09:57 by iritkatriel. This issue is now closed.

Files
File name Uploaded Description Edit
issue22764.patch rbcollins, 2014-10-30 10:03 review
issue22764.patch rbcollins, 2014-10-30 23:50
Messages (10)
msg230258 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2014-10-30 09:57
test_assertRaises_frames_survival (unittest2.test.test_assertions.Test_Assertions
Depends on refcount behaviour to pass - adding a gc.collect() before the weakref checks is sufficient to fix things on pypy.

test_no_exception_leak (unittest2.test.test_case.Test_TestCase
similarly here adding a juidicious gc.collect solves the issue.
msg230259 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2014-10-30 10:03
Herewith a patch. Applied to unittest2 for backport to unbreak its tests on pypy.
msg230260 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-10-30 10:55
I think the change in test_assertions is wrong: it doesn't test what the change was meant to fix in the first place.
msg230261 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-10-30 10:57
You could instead mark the test as cpython-specific.
msg230262 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2014-10-30 11:00
Hmm? I must have misunderstood the test. Here's my understanding: the test is testing that a an object ref only held in the traceback object of the exception is cleaned up such that it can be collected. In a refcount system that collection is immediate, in a gc world its after the next gc run.

Ah, perhaps the test was flawed to start with, since no effort is made in it to capture the error and then process it. If we pass a result in, the traceback will have a ref and gc.collect() can't possibly be discarding the entire structure.
msg230263 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-10-30 11:03
> Here's my understanding: the test is testing that a an object ref only held in the traceback object of the exception is cleaned up such that it can be collected

The best way to check would be to revert the case.py changes in 6ab3193e890e. My intuition is that the test would then fail, but succeed with your changes.
msg230264 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-10-30 11:04
Le 30/10/2014 12:00, Robert Collins a écrit :
> 
> Ah, perhaps the test was flawed to start with, since no effort is
> made
in it to capture the error and then process it. If we pass a result in,
the traceback will have a ref and gc.collect() can't possibly be
discarding the entire structure.

Yes, that would be another way to fix it, probably.
msg230311 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2014-10-30 23:43
I've done some experimentation. Yes, the updated test passes when the clear_frames call is removed in cPython 3.5

But the original test also passes when clear_frames is not called if one drops into pdb and steps through, which still fits the symptoms of having a loop.

The problem is then, that gc-only interpreters make no guarantees about lifetime of objects and this test needs a guarantee. As such I think conditional skipping is probably best.
msg230313 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-10-30 23:54
There's the cpython_only decorator in test.support. I think CPython is the only popular refcounting implementation.
(of course I don't know which implementations may qualify as "popular" exactly :-))
msg400300 - (view) Author: Irit Katriel (iritkatriel) * (Python committer) Date: 2021-08-25 22:22
I don't think this test still exists - I tried to search for it and found nothing. In any case, from the discussion it seems that that is left is to tweak the skip mechanism (assuming the patches were applied).

I'll close this soon if nobody objects.
History
Date User Action Args
2021-09-02 09:57:13iritkatrielsetstatus: pending -> closed
stage: resolved
2021-08-25 22:22:53iritkatrielsetstatus: open -> pending

nosy: + iritkatriel
messages: + msg400300

resolution: fixed
2014-10-30 23:54:09pitrousetmessages: + msg230313
2014-10-30 23:50:04rbcollinssetfiles: + issue22764.patch
2014-10-30 23:43:20rbcollinssetmessages: + msg230311
2014-10-30 11:04:52pitrousetmessages: + msg230264
2014-10-30 11:03:37pitrousetmessages: + msg230263
2014-10-30 11:00:10rbcollinssetmessages: + msg230262
2014-10-30 10:57:52pitrousetmessages: + msg230261
2014-10-30 10:55:37pitrousetnosy: + pitrou
messages: + msg230260
2014-10-30 10:03:56rbcollinssettype: behavior
2014-10-30 10:03:44rbcollinssetfiles: + issue22764.patch
keywords: + patch
messages: + msg230259
2014-10-30 09:57:45rbcollinscreate