Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test cases not garbage collected after run #56007

Closed
fabioz mannequin opened this issue Apr 7, 2011 · 68 comments
Closed

Test cases not garbage collected after run #56007

fabioz mannequin opened this issue Apr 7, 2011 · 68 comments
Assignees
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@fabioz
Copy link
Mannequin

fabioz mannequin commented Apr 7, 2011

BPO 11798
Nosy @tim-one, @rhettinger, @terryjreedy, @vstinner, @fabioz, @benjaminp, @ezio-melotti, @bitdancer, @voidspace, @asvetlov, @meadori, @xdegaye, @charettes
PRs
  • bpo-30813: Fix unittest when hunting refleaks #2502
  • [3.6] bpo-30813: Fix unittest when hunting refleaks (#2502) #2505
  • [3.5] bpo-30813: Fix unittest when hunting refleaks (#2502) #2506
  • Files
  • 11798.patch
  • 11798-20130803-matthewlmcclure.patch
  • issue11798.diff
  • countTestCases.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/voidspace'
    closed_at = <Date 2014-09-21.08:15:26.276>
    created_at = <Date 2011-04-07.16:15:56.969>
    labels = ['type-bug', 'library']
    title = 'Test cases not garbage collected after run'
    updated_at = <Date 2017-06-30.16:52:46.736>
    user = 'https://github.com/fabioz'

    bugs.python.org fields:

    activity = <Date 2017-06-30.16:52:46.736>
    actor = 'gvanrossum'
    assignee = 'michael.foord'
    closed = True
    closed_date = <Date 2014-09-21.08:15:26.276>
    closer = 'berker.peksag'
    components = ['Library (Lib)']
    creation = <Date 2011-04-07.16:15:56.969>
    creator = 'fabioz'
    dependencies = []
    files = ['27352', '31154', '31483', '33131']
    hgrepos = []
    issue_num = 11798
    keywords = ['patch']
    message_count = 68.0
    messages = ['133225', '133226', '133227', '133228', '133229', '133231', '133232', '133236', '133237', '133239', '133241', '136961', '136967', '137464', '171623', '171837', '188424', '194228', '194268', '194398', '194430', '194432', '196273', '196275', '196281', '196282', '196297', '196300', '196301', '196402', '196404', '196406', '196431', '196433', '196607', '196618', '196659', '196665', '196701', '196702', '196703', '196705', '196742', '196766', '197712', '197713', '197714', '197734', '197735', '197888', '198060', '198062', '198066', '198068', '198069', '198071', '198072', '198076', '198080', '206177', '207046', '207047', '207057', '207070', '207081', '297374', '297384', '297388']
    nosy_count = 18.0
    nosy_names = ['tim.peters', 'rhettinger', 'terry.reedy', 'vstinner', 'fabioz', 'benjamin.peterson', 'ezio.melotti', 'r.david.murray', 'michael.foord', 'matthewlmcclure', 'asvetlov', 'meador.inge', 'xdegaye', 'tshepang', 'python-dev', 'tomwardill', 'matthewlmcclure-gmail', 'charettes']
    pr_nums = ['2502', '2505', '2506']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue11798'
    versions = ['Python 3.4']

    @fabioz
    Copy link
    Mannequin Author

    fabioz mannequin commented Apr 7, 2011

    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.

    @fabioz fabioz mannequin added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Apr 7, 2011
    @benjaminp
    Copy link
    Contributor

    benjaminp commented Apr 7, 2011

    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.

    @exarkun
    Copy link
    Mannequin

    exarkun mannequin commented Apr 7, 2011

    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?

    @fabioz
    Copy link
    Mannequin Author

    fabioz mannequin commented Apr 7, 2011

    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).

    @benjaminp
    Copy link
    Contributor

    benjaminp commented Apr 7, 2011

    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.

    @exarkun
    Copy link
    Mannequin

    exarkun mannequin commented Apr 7, 2011

    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.

    @voidspace
    Copy link
    Contributor

    voidspace commented Apr 7, 2011

    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.

    @voidspace voidspace self-assigned this Apr 7, 2011
    @fabioz
    Copy link
    Mannequin Author

    fabioz mannequin commented Apr 7, 2011

    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 :)

    @exarkun
    Copy link
    Mannequin

    exarkun mannequin commented Apr 7, 2011

    @voidspace
    Copy link
    Contributor

    voidspace commented Apr 7, 2011

    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.

    @rhettinger
    Copy link
    Contributor

    rhettinger commented Apr 7, 2011

    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).

    @fabioz
    Copy link
    Mannequin Author

    fabioz mannequin commented May 26, 2011

    So Michal, it seems no objections were raised?

    @voidspace
    Copy link
    Contributor

    voidspace commented May 26, 2011

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

    @fabioz
    Copy link
    Mannequin Author

    fabioz mannequin commented Jun 1, 2011

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

    @tomwardill
    Copy link
    Mannequin

    tomwardill mannequin commented Sep 30, 2012

    Patch attached using setting test to None after execution.

    @voidspace
    Copy link
    Contributor

    voidspace commented Oct 2, 2012

    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.

    @gvanrossum
    Copy link
    Member

    gvanrossum commented May 5, 2013

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

    @terryjreedy
    Copy link
    Member

    terryjreedy commented Aug 3, 2013

    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.

    @bitdancer
    Copy link
    Member

    bitdancer commented Aug 3, 2013

    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.

    @matthewlmcclure-gmail
    Copy link
    Mannequin

    matthewlmcclure-gmail mannequin commented Aug 4, 2013

    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.

    @voidspace
    Copy link
    Contributor

    voidspace commented Aug 4, 2013

    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.

    @voidspace
    Copy link
    Contributor

    voidspace commented Aug 4, 2013

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

    @asvetlov
    Copy link
    Contributor

    asvetlov commented Aug 27, 2013

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

    @matthewlmcclure-gmail
    Copy link
    Mannequin

    matthewlmcclure-gmail mannequin commented Aug 27, 2013

    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

    @asvetlov
    Copy link
    Contributor

    asvetlov commented Aug 27, 2013

    Matt, I've added new patch.

    Will commit it after tomorrow if nobody object.

    @voidspace
    Copy link
    Contributor

    voidspace commented Aug 27, 2013

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

    @asvetlov
    Copy link
    Contributor

    asvetlov commented Aug 27, 2013

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

    @voidspace
    Copy link
    Contributor

    voidspace commented Sep 1, 2013

    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.

    @asvetlov
    Copy link
    Contributor

    asvetlov commented Sep 2, 2013

    Ok. Let's close issue.

    @asvetlov asvetlov closed this as completed Sep 2, 2013
    @pitrou
    Copy link
    Member

    pitrou commented Sep 14, 2013

    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?

    @pitrou pitrou reopened this Sep 14, 2013
    @pitrou
    Copy link
    Member

    pitrou commented Sep 14, 2013

    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.

    @pitrou
    Copy link
    Member

    pitrou commented Sep 14, 2013

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

    @voidspace
    Copy link
    Contributor

    voidspace commented Sep 14, 2013

    Can that be fixed in tulip?

    @pitrou
    Copy link
    Member

    pitrou commented Sep 14, 2013

    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.

    @voidspace
    Copy link
    Contributor

    voidspace commented Sep 16, 2013

    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...

    @pitrou
    Copy link
    Member

    pitrou commented Sep 19, 2013

    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?

    @voidspace
    Copy link
    Contributor

    voidspace commented Sep 19, 2013

    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.

    @pitrou
    Copy link
    Member

    pitrou commented Sep 19, 2013

    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?

    @voidspace
    Copy link
    Contributor

    voidspace commented Sep 19, 2013

    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\>


    @pitrou
    Copy link
    Member

    pitrou commented Sep 19, 2013

    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.

    @voidspace
    Copy link
    Contributor

    voidspace commented Sep 19, 2013

    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.

    @pitrou
    Copy link
    Member

    pitrou commented Sep 19, 2013

    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().

    @voidspace
    Copy link
    Contributor

    voidspace commented Sep 19, 2013

    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.

    @pitrou
    Copy link
    Member

    pitrou commented Sep 19, 2013

    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.

    @xdegaye
    Copy link
    Mannequin

    xdegaye mannequin commented Dec 14, 2013

    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.

    @pitrou
    Copy link
    Member

    pitrou commented Dec 28, 2013

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

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Dec 28, 2013

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

    @voidspace
    Copy link
    Contributor

    voidspace commented Dec 28, 2013

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

    @pitrou
    Copy link
    Member

    pitrou commented Dec 29, 2013

    It is used, see countTestCases().

    @voidspace
    Copy link
    Contributor

    voidspace commented Dec 29, 2013

    Ah yes, I see - sorry.

    @vstinner
    Copy link
    Member

    vstinner commented Jun 30, 2017

    New changeset e4f9a2d by Victor Stinner in branch 'master':
    bpo-30813: Fix unittest when hunting refleaks (bpo-2502)
    e4f9a2d

    @vstinner
    Copy link
    Member

    vstinner commented Jun 30, 2017

    New changeset 714afcc by Victor Stinner in branch '3.5':
    bpo-30813: Fix unittest when hunting refleaks (bpo-2502) (bpo-2506)
    714afcc

    @vstinner
    Copy link
    Member

    vstinner commented Jun 30, 2017

    New changeset 22d4e8f by Victor Stinner in branch '3.6':
    bpo-30813: Fix unittest when hunting refleaks (bpo-2502) (bpo-2505)
    22d4e8f

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests