-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
gh-96139: [unittest] call tearDown
even if asyncSetUp
fails
#99596
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
base: main
Are you sure you want to change the base?
Conversation
@@ -211,6 +262,9 @@ async def asyncTearDown(self): | |||
events.append('asyncTearDown') | |||
raise MyException() | |||
|
|||
def tearDown(self): # must not be called |
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 one is questionable. Right now if asyncTearDown
fails, tearDown
is not called.
But, following the same logic: setUp
was successful. Maybe it should be called?
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 think that it should be called.
Also, add tearDown
in test_exception_in_setup
. It should be called.
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 worked on different solution (it is a solution of different problem, but would also solve this issue), but is backward incompatible, and needs other changes to avoid some regressions. In any case, even if it will be accepted, it cannot be backported. So it is better to fix this issue separately.
But your solution seems has a problem with classes with tearDown.
events.append('setUp') | ||
|
||
async def asyncSetUp(self): | ||
self.assertEqual(events, ['setUp']) |
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.
test_exception_in_setup
does not contain such checks. If you want to add them, look at the updated test_full_cycle
. It is more convenient to define the expected
at the beginning and then use its slices. If you will add more events, you will only need to update one list and several indices.
@@ -211,6 +262,9 @@ async def asyncTearDown(self): | |||
events.append('asyncTearDown') | |||
raise MyException() | |||
|
|||
def tearDown(self): # must not be called |
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 think that it should be called.
Also, add tearDown
in test_exception_in_setup
. It should be called.
Docs:
Some parts that are important:
In this case test method is not called. But, we have a guard in the docs. Callers must be careful with its internal state.
This gives me confidence in this patch. It does what the docs say. If
setUp()
is successful, we need to runtearDown()
.Things I like about the implementation:
TestCase
, onlyIsolatedAsyncioTestCase
TestCase
subclasses:_callCleanupAfterFailedSetup
can be adjusted in user-defined classes if neededDo we need to better document it? I would prefer to hide it for now for end users.