-
-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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
bpo-24412: Adds cleanUps for setUpClass and setUpModule. #9190
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this PR! I've wanted this for a long time, and really appreciate you working on it. I have a few questions and comments inlined.
Lib/unittest/case.py
Outdated
tearDownModule.""" | ||
while _module_cleanups: | ||
function, args, kwargs = _module_cleanups.pop() | ||
function(*args, **kwargs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if an exception occurs in function
? Currently that will prevent other module cleanups from being called. Without looking at what's done for other cleanup functions, I think it might be better to catch the exception and continue processing, but OTOH consistency in behavior with other cleanups is best.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes you're right, this doesn't work quite right here. The normal tearDown() neatly handles the exceptions so that all the cleanups will be called. I'll look into doing that here an in the class tearDown as well. Thanks for catching this!
@@ -390,6 +404,8 @@ class TestCase(object): | |||
|
|||
_classSetupFailed = False | |||
|
|||
_class_cleanups = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nit (maybe!): In this case, the blank line between these two class attributes may not be needed.
Lib/unittest/case.py
Outdated
@@ -445,6 +461,12 @@ def addCleanup(self, function, *args, **kwargs): | |||
Cleanup items are called even if setUp fails (unlike tearDown).""" | |||
self._cleanups.append((function, args, kwargs)) | |||
|
|||
@classmethod | |||
def addClassCleanup(cls, function, *args, **kwargs): | |||
"""Same as addCleanup, excet the cleanup items are called even if |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/excet/except/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Lib/unittest/case.py
Outdated
tearDownClass.""" | ||
while cls._class_cleanups: | ||
function, args, kwargs = cls._class_cleanups.pop() | ||
function(*args, **kwargs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question as above with exceptions in function
.
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
I have made the requested changes; please review again. Thanks for the feedback Barry! Hopefully I haven't missed any edge cases, I think I have covered the possible exception scenarios now. |
Thanks for making the requested changes! @warsaw: please review the changes made to this pull request. |
except Exception as exc: | ||
exceptions.append(exc) | ||
if exceptions: | ||
raise exceptions[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I totally get why you have to do it this way, although it would be nice if we had trio's MultiError in the stdlib. :) It's unfortunate that we have to throw away all the other exceptions, but I don't know where you can stash them. E.g. IIUC, instance cleanups stash the exceptions on self.errors
, but there's no such place on modules (unless this code adds it, which, yuck :)). The other alternative is to create something like a simple MultiError
, add the exceptions to that, and then raise the MultiError
(maybe only if len(exceptions) > 1
?).
It seems like a lot of extra complication for hopefully an, um, exceptional case, so maybe punt on that for now. IOW, I'm cool with this, unless you have any other ideas.
Lib/unittest/case.py
Outdated
except Exception as exc: | ||
exceptions.append(exc) | ||
if exceptions: | ||
raise exceptions[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case exceptions
could be stashed on cls
so they don't get lost.
Lib/unittest/suite.py
Outdated
finally: | ||
_call_if_exists(result, '_restoreStdout') | ||
|
||
def _createClassOrModuleLevelException(self, result, exc, parent, name): | ||
errorName = '{} ({})'.format(parent, name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is Python 3.8, so what do you think about using f{parent} ({name})
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely! I wasn't sure how important maintaining the current style in the code is, I didn't want to jump the gun and make it 3.8 only if normally we try to keep the code more backwards compatible. I will move it to the f-string style :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have just a few more comments, but this is looking great, so I'll approve it. If you have any thoughts on how to save the list of module exceptions, then I think that would be good to implement, but if not, then I think this branch looks great. Thanks!
But,oops, Travis is failing ;) |
That appears to have been a transient error; I restarted the build and it succeeded. |
Thanks Tal! I plan to finish this up tonight, looking into if I can improve the exception handling. |
I'm looking forward to it! IMO we should definitely have this available in 3.8. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a What's New entry.
Doc/library/unittest.rst
Outdated
@@ -1448,6 +1448,66 @@ Test cases | |||
|
|||
.. versionadded:: 3.1 | |||
|
|||
.. method:: addClassCleanup(function, *args, **kwargs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
classmethod
Doc/library/unittest.rst
Outdated
.. versionadded:: 3.8 | ||
|
||
|
||
.. method:: doClassCleanups() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
classmethod
Doc/library/unittest.rst
Outdated
|
||
.. versionadded:: 3.8 | ||
|
||
.. method:: addModuleCleanup(function, *args, **kwargs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not a method of TestCase.
Doc/library/unittest.rst
Outdated
.. versionadded:: 3.8 | ||
|
||
|
||
.. method:: doModuleCleanups() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not a method of TestCase.
Lib/unittest/suite.py
Outdated
finally: | ||
_call_if_exists(result, '_restoreStdout') | ||
|
||
def _createClassOrModuleLevelException(self, result, exc, parent, name): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't names parent
and name
be swapped or changed to more meaning names? The third argument is the method name and the forth argument is the module or class name.
@@ -0,0 +1,2 @@ | |||
Add ``addModuleCleanup()`` and ``addClassCleanup()`` to unittest to support |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be worth to add references: :meth:`~unittest.TestCase.addModuleCleanup()`
, :func:`~unittest.addModuleCleanup()`
, :meth:`~unittest.TestCase.setUpClass()`
, :func:`~unittest.setUpModule()`
.
|
https://bugs.python.org/issue24412
This issue is rather old so it could benefit from a refreshed discussion.
I think having the cleanUps be available for all of the setUp types makes sense, it keeps code cleaner by reducing try-catches and keeps the API consistent across all the setUps.
https://bugs.python.org/issue24412