Skip to content

bpo-43913: Fix unittest module cleanup #25700

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.

Sign up for GitHub

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

Closed
miguendes wants to merge 4 commits into from
Closed

bpo-43913: Fix unittest module cleanup #25700

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cd82712
Fix unittest module cleanup
miguendes Apr 28, 2021
75d48ee
📜🤖 Added by blurb_it.
blurb-it[bot] Apr 29, 2021
36f2dc7
Rename method on exception
miguendes May 4, 2021
9ad430c
Ensure module is cleaned up when exception
miguendes May 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions Lib/unittest/suite.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,12 +264,12 @@ def _handleModuleTearDown(self, result):
previousModule)
finally:
_call_if_exists(result, '_restoreStdout')
try:

@JelleZijlstra JelleZijlstra May 4, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it also be called in this finally block? That's the previous behavior, and I imagine it could be important to ensure it's called even when tearDownModule() raises a BaseException.

@miguendes miguendes May 4, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion. I think this is not need TBH, my understanding it that the finally block will be executed anyways in case of a Exception and, since _call_if_exists doesn't return, the clean up is done in the following steps.

Regarding the BaseException, yes, if tearDownModule raises it no doModuleCleanups is performed.

I think it's a good idea to catch it but for consistency we should change setUpClass as well. What do you think?

    setUpClass = getattr(currentClass, 'setUpClass', None)
    if setUpClass is not None:
        _call_if_exists(result, '_setupStdout')
        try:
            setUpClass()
        except Exception as e:
            if isinstance(result, _DebugResult):
                raise`

case.doModuleCleanups()
except Exception as e:
self._createClassOrModuleLevelException(result, e,
'tearDownModule',
previousModule)
try:
case.doModuleCleanups()
except Exception as e:
self._createClassOrModuleLevelException(result, e,
'doModuleCleanups',
previousModule)

def _tearDownPreviousClass(self, test, result):
previousClass = getattr(result, '_previousTestClass', None)
Expand Down
48 changes: 48 additions & 0 deletions Lib/unittest/test/test_runner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -418,6 +418,54 @@ class Module(object):
self.assertEqual(cleanups,
[((1, 2), {'function': 'hello'})])

def test_run_module_cleanUp_without_teardown(self):
ordering = []

class Module(object):
unittest.addModuleCleanup(print, 'module cleanup')

class TestableTest(unittest.TestCase):
@classmethod
def setUpClass(cls):
ordering.append('setUpClass')
def testNothing(self):
ordering.append('test')
@classmethod
def tearDownClass(cls):
ordering.append('tearDownClass')

TestableTest.__module__ = 'Module'
sys.modules['Module'] = Module
runTests(TestableTest)
self.assertEqual(ordering, ['setUpClass', 'test', 'tearDownClass'])
self.assertEqual(unittest.case._module_cleanups, [])

def test_run_module_cleanUp_when_teardown_exception(self):
ordering = []

class Module(object):
unittest.addModuleCleanup(print, 'module cleanup')

@staticmethod
def tearDownModule():
raise Exception('CleanUpExc')

class TestableTest(unittest.TestCase):
@classmethod
def setUpClass(cls):
ordering.append('setUpClass')
def testNothing(self):
ordering.append('test')
@classmethod
def tearDownClass(cls):
ordering.append('tearDownClass')

TestableTest.__module__ = 'Module'
sys.modules['Module'] = Module
runTests(TestableTest)
self.assertEqual(ordering, ['setUpClass', 'test', 'tearDownClass'])
self.assertEqual(unittest.case._module_cleanups, [])

def test_run_module_cleanUp(self):
blowUp = True
ordering = []
Expand Down
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Library/2021-04-29-07-08-47.bpo-43913.wcTUA5.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix the :func:`unittest.addModuleCleanup` behavior. It is now called even if :meth:`tearDownModule` is not defined.