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

Clean up unittest TestCase objects after tests are complete (#1649). #2046

Merged
merged 1 commit into from
Nov 8, 2016

Conversation

d-b-w
Copy link

@d-b-w d-b-w commented Nov 8, 2016

Fix #1649

Users of unittest style TestCases will create expensive objects
in setUp. We should clean up TestCase instances that are lying
around so that they don't fill up memory.

Here's a quick checklist that should be present in PRs:

  • Target: for bug or doc fixes, target master; for new features, target features; (master)
  • Make sure to include one or more tests for your change; (test_teardown_issue1649(), fails before and passes after)
  • Add yourself to AUTHORS; (as Daniel Wandschneider)
  • Add a new entry to CHANGELOG.rst (Added first line of this description)

…ev#1649).

Fix pytest-dev#1649

Users of unittest style TestCases will create expensive objects
in setUp. We should clean up TestCase instances that are lying
around so that they don't fill up memory.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.002%) to 92.798% when pulling e46e653 on d-b-w:clean-up-unittest-issue1649 into 07af307 on pytest-dev:master.

@The-Compiler
Copy link
Member

Looking at http://stackoverflow.com/questions/40478600/mem-leak-in-pyqt-test-with-pytest should we do the same with other test classes too?

@d-b-w
Copy link
Author

d-b-w commented Nov 8, 2016

Oh, yeah, almost certainly.

I didn't think people would do add attributes to the test instances for pytest style test cases because they could just use fixtures. I'll put together a second PR for non-unittest cases.

@nicoddemus
Copy link
Member

@d-b-w, I did try out your branch with this code:

import unittest

class ExpensiveObject(object):

    def __del__(self):
        print('ExpensiveObject deleted')

class TestCaseObjectsShouldBeCleanedUp(unittest.TestCase):

    def setUp(self):
        print('Created')
        self.an_expensive_object = ExpensiveObject()

    def test_demo(self):
        pass

    def test_demo2(self):
        pass    

On master:

$ pytest foo.py -s -q
Created
.Created
.
2 passed in 0.03 seconds

On your branch:

$ pytest foo.py -s -q
Created
.ExpensiveObject deleted
Created
.ExpensiveObject deleted

2 passed in 0.03 seconds

So it seems to work just fine, thanks for this. 😁

@The-Compiler, you got a point:

class ExpensiveObject(object):

    def __del__(self):
        print('ExpensiveObject deleted')

class TestCaseObjectsShouldBeCleanedUp(object):

    def setup_method(self, m):
        print('Created')
        self.an_expensive_object = ExpensiveObject()

    def test_demo(self):
        pass

    def test_demo2(self):
        pass    
$ pytest foo.py -s -q
Created
.Created
.
2 passed in 0.03 seconds

So we should probably do something similar for normal test instances.

@d-b-w would be willing to work on that as well? Otherwise I would rather we merge this as it is because it is already an improvement and create a separate issue.

@nicoddemus
Copy link
Member

nicoddemus commented Nov 8, 2016

I didn't think people would do add attributes to the test instances for pytest style test cases because they could just use fixtures. I'll put together a second PR for non-unittest cases.

Oh sorry I didn't see your reply before I posted mine. I will merge this as it is then if you prefer a separate PR.

Thanks again!

@nicoddemus nicoddemus merged commit 0b94c43 into pytest-dev:master Nov 8, 2016
@d-b-w
Copy link
Author

d-b-w commented Nov 9, 2016

Heh - I'm accustomed to making my commits as small as they can be and still be functional. I suppose that for contributions to pytest, PRs should resolve complete issues/ or be complete lines on the release notes :)

@nicoddemus
Copy link
Member

No problem at all having two separate PRs for this. I went ahead and merged because I don't know when you can find time to also work on the issue of the non-unittest cases so I thought best to get this into master right away. 😁

@d-b-w
Copy link
Author

d-b-w commented Nov 17, 2016

I'm having some trouble with the fix for non-unittest tests - when I add a teardown to the Instance class, the nose generator tests fail (from python.py):

class Instance(PyCollector):
    ...
    def teardown(self):
         self.obj = None

The failure message:

test_nose_test_generator_fixtures.py .....FFFFF
______________________________ TestClass.test[0] _______________________________

self = <test_nose_test_generator_fixtures.TestClass object at 0x103ac8190>
i = 0

 def check(self, i):
     print ("check called in %s" % self)
     expect = ['setup']
     #for x in range(0, i):
     #    expect.append('setup')
     #    expect.append('teardown')
     #expect.append('setup')
  eq_(self.called, expect)

E AttributeError: 'TestClass' object has no attribute 'called'

I think this means that the setup wasn't performed for some of the tests?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

unittest instance objects exist for the lifetime of the py.test run
4 participants