Right now, when doing a test case, one must clear all the variables created in the test class, and I believe this shouldn't be needed...

E.g.:

class Test(TestCase):
  def setUp(self):
    self.obj1 = MyObject()

  ...

  def tearDown(self):
    del self.obj1

Ideally (in my view), right after running the test, it should be garbage-collected and the explicit tearDown wouldn't be needed (as the test would garbage-collected, that reference would automatically die), because this is currently very error prone... (and probably a source of leaks for any sufficiently big test suite).

If that's accepted, I can provide a patch.

You don't have to clear them; you just have to finalize them. Anyway, this is essentially impossible to do in a backward compatible way given that TestCases are expected to stay around.

Trial lets test cases get garbaged collected.  When we noticed this wasn't happening, we treated it as a bug and fixed it.  No one ever complained about the change.  I don't see any obvious way in which an application would even be able to tell the difference (a user can tell the difference by looking at top).  In what case do you think this change would result in broken application code?

I do get the idea of the backward incompatibility, although I think it's really minor in this case.

Just for some data, the pydev test runner has had a fix to clear those test cases for quite a while already and no one has complained about it (it actually makes each of the tests None after run, so, if someone tries to access it after that, it would be pretty clear that it's not there anymore).

2011/4/7 Jean-Paul Calderone <report@bugs.python.org>:
>
> Jean-Paul Calderone <invalid@example.invalid> added the comment:
>
> Trial lets test cases get garbaged collected.  When we noticed this wasn't happening, we treated it as a bug and fixed it.  No one ever complained about the change.  I don't see any obvious way in which an application would even be able to tell the difference (a user can tell the difference by looking at top).  In what case do you think this change would result in broken application code?

I thought unittest was just handed a bunch of TestCase instances and
couldn't do much about insuring they were garbage collected.

> I thought unittest was just handed a bunch of TestCase instances and couldn't do much about insuring they were garbage collected.

True.  But unittest could ensure that it doesn't keep a reference to each TestCase instance after it finishes running it.  Then, if no one else has a reference either, it can be garbage collected.

A TestSuite (which is how tests are collected to run) holds all the tests and therefore keeps them all alive for the duration of the test run. (I presume this is the issue anyway.)

How would you solve this - by having calling a TestSuite (which is how a test run is executed) remove members from themselves after each test execution? (Any failure tracebacks etc stored by the TestResult would also have to not keep the test alive.)

My only concern would be backwards compatibility due to the change in behaviour.

The current code I use in PyDev is below -- another option could be not adding the None to the list of tests, but removing it, but I find that a bit worse because in the current way if someone tries to access it after it's ran, it'll become clear it was removed.

def run(self, result):
    for index, test in enumerate(self._tests):
        if result.shouldStop:
            break
        test(result)

        # Let the memory be released! 
        self._tests[index] = None

    return result
    
    
I think the issue with the test result storing the test is much more difficult to deal with (because currently most unit test frameworks probably rely on having it there), so, that's probably not something I'd try to fix as it'd probably break many clients... in which case it should be expected that the test is kept alive if it fails -- but as the idea is that all has to turn green anyways, I don't see this as a big issue :)

Here's Trial's implementation: http://twistedmatrix.com/trac/browser/trunk/twisted/trial/runner.py#L138

Not keeping tests alive for the whole run seems like a good thing and either implementation seems fine to me. I'd be interested to hear if anyone else had any backwards compatibility concerns though.

> Not keeping tests alive for the whole run seems like a
> good thing 

+1

> and either implementation seems fine to me.

I slightly prefer Fabio;s assignment to None approach (for subtle reasons that I can't articulate at the moment).

So Michal, it seems no objections were raised?

Sure, let's do it. Fabio, care to provide patch with tests and doc changes? (For 3.3.)

Sure, will try to get a patch for next week...

Patch attached using setting test to None after execution.

The patch looks good to me, although there probably needs to be a note in the TestSuite docs too. I'll apply this to Python 3.4, which leaves plenty of time for people to object.

Note that people needing the old behaviour can subclass TestSuite and provide a dummy implementation of _removeTestAtIndex.

Just to be self-referential here's a link to #17908.

If the iterator for 'self' were de-structive, if it removed (popped) the test from whatever structure holds it before yielding it, the messiness of enumerate and the new ._removeTestAtIndex method would not be needed and 'for test in self' would work as desired. If considered necessary,  new method .pop_iter, used in 'for test in self.pop_iter', would make it obvious that the iteration empties the collection.

Terry: I would not be in favor of using the normal iter, since iterating a collection doesn't normally empty it, and there may be tools that iterate a test suite outside of test execution.  Adding a pop_iter method would be a backward compatibility issue, since "replacement" test suites would not have that method.  I think the current patch is the best bet for maintaining backward compatibility.

Michael Foord <fuzzyman <at> voidspace.org.uk> writes:
> On 2 Aug 2013, at 19:19, Antoine Pitrou <solipsis <at> pitrou.net> wrote:
> > The patch is basically ready for commit, except for a possible doc
> > addition, no?
> 
> Looks to be the case, reading the patch it looks fine. I'm currently on
> holiday until Monday. If anyone is motivated to do the docs too and 
> commit that would be great. Otherwise I'll get to it on my return.

It looks like the patch is based on what will become 3.4. Would backporting it to 2.7 be feasible?  What's involved in doing so?

I took a crack at the docs. I'm attaching an updated patch.

This smells like a new feature to me (it's certainly a fairly significant change in behaviour) and isn't appropriate for backporting to 2.7.

It can however go into unittest2.

I agree with David that a destructive iteration using pop is more likely to cause backwards-compatibility issues.

The doc patch looks good, thanks Matt. I'll read it through properly before committing.

Looks good but review comments worth to be applied or rejected with reasonable note.

Andrew,

I didn't understand your message. Are you asking me to change the patch somehow? Or asking Michael to review and apply it?

Best,
Matt

Matt, I've added new patch.

Will commit it after tomorrow if nobody object.

Go ahead and commit. The functionality and patch are good.

Matt, would you sign licence agreement http://www.python.org/psf/contrib/ ?
The Python Software Fondation is asking all contributors to sign it.
Thanks.

Andrew,

I signed the agreement as matthewlmcclure and as matthewlmcclure-gmail. Is there any way I can merge those two user accounts?

I believe the original patch was Tom Wardill's. I just updated his patch.

There is no easy way to merge accounts in roundup.  If you've submitted the agreement, your "*" should show up in a bit :)

New changeset 1c2a37459c70 by Andrew Svetlov in branch 'default':
Issue #11798: TestSuite now drops references to own tests after execution.
http://hg.python.org/cpython/rev/1c2a37459c70

This seems to be producing a test failure in test_doctest.  eg:

 http://buildbot.python.org/all/builders/AMD64%20FreeBSD%209.0%20dtrace%203.x/builds/1920

This might fix it (untested):

diff -r d748d7020192 Lib/test/test_doctest.py
--- a/Lib/test/test_doctest.py	Sat Aug 03 10:09:25 2013 -0400
+++ b/Lib/test/test_doctest.py	Wed Aug 28 15:35:58 2013 -0400
@@ -2329,6 +2329,8 @@
 
     Now, when we run the test:
 
+      >>> suite = doctest.DocFileSuite('test_doctest.txt',
+      ...                          optionflags=doctest.DONT_ACCEPT_BLANKLINE)
       >>> result = suite.run(unittest.TestResult())
       >>> print(result.failures[0][1]) # doctest: +ELLIPSIS
       Traceback ...

New changeset 17f23cf029cf by Andrew Svetlov in branch 'default':
Fix tests for #11798
http://hg.python.org/cpython/rev/17f23cf029cf

Sorry. Tests are fixed now.

I see some regressions when reference leak hunting with -j './python -j8 -R :'

test test_ast crashed -- Traceback (most recent call last):
  File "/home/meadori/src/cpython/Lib/test/regrtest.py", line 1265, in runtest_inne
r
    huntrleaks)  File "/home/meadori/src/cpython/Lib/test/regrtest.py", line 1381, in dash_R
    indirect_test()
  File "/home/meadori/src/cpython/Lib/test/regrtest.py", line 1261, in <lambda>    test_runner = lambda: support.run_unittest(tests)
  File "/home/meadori/src/cpython/Lib/test/support/__init__.py", line 1683, in run_
unittest
    _run_suite(suite)
  File "/home/meadori/src/cpython/Lib/test/support/__init__.py", line 1649, in _run
_suite
    result = runner.run(suite)
  File "/home/meadori/src/cpython/Lib/test/support/__init__.py", line 1548, in run
    test(result)
  File "/home/meadori/src/cpython/Lib/unittest/suite.py", line 76, in __call__
    return self.run(*args, **kwds)
  File "/home/meadori/src/cpython/Lib/unittest/suite.py", line 114, in run
    test(result)
  File "/home/meadori/src/cpython/Lib/unittest/suite.py", line 76, in __call__
    return self.run(*args, **kwds)
  File "/home/meadori/src/cpython/Lib/unittest/suite.py", line 114, in run
    test(result)
TypeError: 'NoneType' object is not callable

Good catch!
That's because -R run the same test suite several times.
I'm working on patch.

New changeset 868ad6fa8e68 by Andrew Svetlov in branch 'default':
Temporary disable tests cleanup (issue 11798).
http://hg.python.org/cpython/rev/868ad6fa8e68

Er... your latest commit broke this issue's own tests!

All the buildbots are failing due to changeset 868ad6fa8e68 - I'm going to back it out.

New changeset 7035b5d8fc0f by Tim Peters in branch 'default':
Back out 868ad6fa8e68 - it left all the buildbots failing.
http://hg.python.org/cpython/rev/7035b5d8fc0f

New changeset 39781c3737f8 by Andrew Svetlov in branch 'default':
Issue #11798: fix tests for regrtest -R :
http://hg.python.org/cpython/rev/39781c3737f8

Now 'regrtest.py -j4 -R : ' passes.
Do we need to add parameter for disabling tests cleanup to TestSuite, TestLoader and TestProgrm constructors?

I'd rather not propagate more options all the way through, especially as this is some thing that should be decided by the test framework and is unlikely to be something you want to turn on and off per test run (which is what command line options are for). Frameworks that want to disable this behaviour should use a TestSuite that overrides _removeAtIndex.

Ok. Let's close issue.

> I'd rather not propagate more options all the way through, especially 
> as this is some thing that should be decided by the test framework and 
> is unlikely to be something you want to turn on and off per test run
> (which is what command line options are for). Frameworks that want to 
> disable this behaviour should use a TestSuite that overrides
> _removeAtIndex.

That sounds like a completely disproportionate solution. Why would you have to override the TestSuite class just to change an option and restore old behaviour? Why don't you simply expose the cleanup flag on TestSuite instances?

For the record, this change broke the --forever option in Tulip's test script, which is why I'm caring. Setting the _cleanup flag to False seems to restore old behaviour, except that _cleanup is (obviously) a private API.

Note: ideally, the --forever flag wouldn't reuse TestCase instances but rather create new ones.

Can that be fixed in tulip?

Yes, but that's not the point. Legitimate use cases can be broken by the change, so at least there should be an easy way to disable the new behaviour.

If we're sure suite._cleanupis a *good* api for this then fine to expose it (and document) it as a public api. I'll take a look at it in a bit. 

Test suites will still have to do *some* monkeying around to set suite.cleanup (presumably in load_tests), so I'm not sure it's much more convenient...

Ideally, test specification should be separate from test execution. That is, it should be possible to keep the TestCase around (or whatever instantiates it, e.g. a factory) but get rid of its per-test-execution attributes.

Perhaps restoring the original __dict__ contents would do the trick?

That would only be a shallow copy, so I'm not sure it's worth the effort. The test has the opportunity in the setUp to ensure that initial state is correct - so I would leave that per test. Obviously sharing state between tests is prima facie bad, but any framework reusing test suites is doing that already.

> That would only be a shallow copy, so I'm not sure it's worth the
> effort. The test has the opportunity in the setUp to ensure that
> initial state is correct - so I would leave that per test.

I don't understand your objection. The concern is to get rid of old
state after test execution.

> Obviously
> sharing state between tests is prima facie bad, but any framework
> reusing test suites is doing that already.

What do you mean?

On 19 Sep 2013, at 14:06, Antoine Pitrou <report@bugs.python.org> wrote:

> 
> Antoine Pitrou added the comment:
> 
>> That would only be a shallow copy, so I'm not sure it's worth the
>> effort. The test has the opportunity in the setUp to ensure that
>> initial state is correct - so I would leave that per test.
> 
> I don't understand your objection. The concern is to get rid of old
> state after test execution.
> 

If the object state includes mutable objects then restoring the previous dictionary will just restore the same mutable (and likely mutated) object. To *properly* restore state you'd either need to deepcopy the dictionary or reinstantiate the testcase (not reuse it in other words). I'd rather leave it up to each test to ensure it reinitialises attributes in setUp than add further complexity that only does part of the job.

>> Obviously
>> sharing state between tests is prima facie bad, but any framework
>> reusing test suites is doing that already.
> 
> What do you mean?

Any framework that is currently reusing test suites is re-using testcase instances. They are already sharing state between the runs.

In fact messing with testcase dictionaries is a further possible cause of backwards incompatibility for those suites. 

> 
> ----------
> 
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue11798>
> _______________________________________

> If the object state includes mutable objects then restoring the
> previous dictionary will just restore the same mutable (and likely
> mutated) object.

I don't understand what you're talking about. Which mutable objects
exactly? I'm talking about copying the dict before setUp.

> >> Obviously
> >> sharing state between tests is prima facie bad, but any framework
> >> reusing test suites is doing that already.
> > 
> > What do you mean?
> 
> Any framework that is currently reusing test suites is re-using
> testcase instances. They are already sharing state between the runs.

They are not sharing it, since setUp will usually create the state
anew. What we're talking about is cleaning up the state after tearDown
is run, instead of waiting for the next setUp call.

Ah right, my mistake. Before setUp there shouldn't be test state. (Although tests are free to do whatever they want in __init__ too and I've seen plenty of TestCase subclasses using __init__ when they should be using setUp.)

Essentially though _cleanup is a backwards compatibility feature - and suites that need _cleanup as a public api are already living without testcase dict cleanup.

That said, I agree that the __dict__ proposal is a hack, but as is the current _removetestAtIndex mechanism.

The only clean solution I can think of would be to have two separate classes:
- a TestSpec which contains execution-independent data about a test case, and knows how to instantiate it
- a TestCase that is used for the actual test execution, but isn't saved in the test suite

Maybe it's possible to do this without any backwards compat problem by making TestSuite.__iter__ always return TestCases (but freshly-created ones, from the inner test specs). The main point of adaptation would be TestLoader.loadTestsFromTestCase().

Having TestLoader.loadTestsFromTestCase() return a "lazy suite" that defers testcase instantiation until iteration is a nice idea.

Unfortunately the TestSuite.addTests api iterates over a suite to add new tests. i.e. the code that builds a TestSuite for module probably already iterates over the suites returned by TestLoader.loadTestsFromTestCase - so the change would need to be more pervasive.

> Unfortunately the TestSuite.addTests api iterates over a suite to add
> new tests. i.e. the code that builds a TestSuite for module probably
> already iterates over the suites returned by
> TestLoader.loadTestsFromTestCase - so the change would need to be
> more pervasive.

addTests() could easily be tweaked to recognize that it gets passed a
TestSuite, and special-case that.

Also, TestCase objects could probably get an optional "spec" attribute
pointing to their TestSpec.

This seems to break BaseTestSuite.countTestCases when invoked after the TestSuite has been run:
  ...
  File "Lib/unittest/suite.py", line 42, in countTestCases
    cases += test.countTestCases()
AttributeError: 'NoneType' object has no attribute 'countTestCases'

Attached patch attempts to fix it.

No answer to Xavier's regression? The way this issue is being treated is a bit worrying.

New changeset b668c409c10a by Antoine Pitrou in branch 'default':
Fix breakage in TestSuite.countTestCases() introduced by issue #11798.
http://hg.python.org/cpython/rev/b668c409c10a

What's the purpose of _removed_tests in your fix, it doesn't appear to be used?

It is used, see countTestCases().

Ah yes, I see - sorry.

New changeset e4f9a2d2be42d5a2cdd624f8ed7cdf5028c5fbc3 by Victor Stinner in branch 'master':
bpo-30813: Fix unittest when hunting refleaks (#2502)
https://github.com/python/cpython/commit/e4f9a2d2be42d5a2cdd624f8ed7cdf5028c5fbc3

New changeset 714afccf6e7644d21ce1a39e90bf83cb0c9a74f1 by Victor Stinner in branch '3.5':
bpo-30813: Fix unittest when hunting refleaks (#2502) (#2506)
https://github.com/python/cpython/commit/714afccf6e7644d21ce1a39e90bf83cb0c9a74f1

New changeset 22d4e8fb99b16657eabfe7f9fee2d40a5ef882f6 by Victor Stinner in branch '3.6':
bpo-30813: Fix unittest when hunting refleaks (#2502) (#2505)
https://github.com/python/cpython/commit/22d4e8fb99b16657eabfe7f9fee2d40a5ef882f6