Skip to content

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

sobolevn
Copy link
Member

@sobolevn sobolevn commented Nov 19, 2022

Docs:

tearDown()

    Method called immediately after the test method has been called and the result recorded. This is called even if the test method raised an exception, so the implementation in subclasses may need to be particularly careful about checking internal state. Any exception, other than [AssertionError](https://docs.python.org/3/library/exceptions.html#AssertionError) or [SkipTest](https://docs.python.org/3/library/unittest.html#unittest.SkipTest), raised by this method will be considered an additional error rather than a test failure (thus increasing the total number of reported errors). This method will only be called if the [setUp()](https://docs.python.org/3/library/unittest.html#unittest.TestCase.setUp) succeeds, regardless of the outcome of the test method. The default implementation does nothing.

Some parts that are important:

This is called even if the test method raised an exception, so the implementation in subclasses may need to be particularly careful about checking internal state.

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 method will only be called if the setUp() succeeds, regardless of the outcome of the test method.

This gives me confidence in this patch. It does what the docs say. If setUp() is successful, we need to run tearDown().

Things I like about the implementation:

  • It is simple
  • It does not polute TestCase, only IsolatedAsyncioTestCase
  • But, it still gives some flexibility to TestCase subclasses: _callCleanupAfterFailedSetup can be adjusted in user-defined classes if needed

Do we need to better document it? I would prefer to hide it for now for end users.

@@ -211,6 +262,9 @@ async def asyncTearDown(self):
events.append('asyncTearDown')
raise MyException()

def tearDown(self): # must not be called
Copy link
Member Author

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?

Copy link
Member

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.

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a 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'])
Copy link
Member

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
Copy link
Member

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.

@cjw296 cjw296 removed their request for review June 9, 2023 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants