TestCase: call _post_teardown always when _pre_setup worked

[blueyed]

This is in line with unittest itself calling TearDown when SetUp
succeeded.

Ref/via: pytest-dev/pytest-django#772

[blueyed]

@felixxm
Please let me know if I should create a ticket for this.
Maybe a ref to (and comment at) https://code.djangoproject.com/ticket/27391 might be enough.

[asfaltboy]

@blueyed thanks for looking into this. Can you please add more details on what this aims to address, and point us to the place in unittest where tearDown is called after debugged error?

It seems to me that tearDown is only called when no exception is triggered, whether with debug or not. Or, am I missing something?

While I do understand the pain of having a non-rolled-back Django DB after a failing/skipped test, I'm not sure I agree with this change.

Calling super().__call__(result) in a TestCase will enter the testPartExecutor contextmanager which catches SkipTest explicitly and handles other errors, thus muting exceptions. Only when we call super().debug() that we allow the exceptions to propagate; and, unittest itself catches nothing in TestCase.debug() as it aims to allow test runners to be the first to handle an exception (for easier postmortem?).

Shouldn't this be handled in the runner instead? If I understand the design in unittest, it's up to whoever is the user of the debug() function to call any teardown necessary for completing the run.

By the way, I'm not sure that unittest.TestCase implementation is necessarily the best, I guess a good solution would be to catch SkipTest in debug explicitly. CPython ticket #36674 that discusses debug not handling SkipTest.

References:

• https://github.com/python/cpython/blob/master/Lib/unittest/case.py#L721-L728
• https://github.com/python/cpython/blob/master/Lib/unittest/case.py#L662
• https://github.com/python/cpython/blob/master/Lib/unittest/case.py#L56-L82
• https://bugs.python.org/issue36674
• bpo-36674: Stops skipped tests from running in debug mode. python/cpython#13180

[blueyed]

If I understand the design in unittest, it's up to whoever is the user of the debug() function to call any teardown necessary for completing the run.

Yes.
And Django appears to not call it itself, but only helps users calling it if I see this correctly(?).

Therefore it is not really needed/useful indeed, closing for now.

This resulted from when I've looked into issues with pytest-django, which only happen in the first place because pytest itself calls .debug() (if --pdb is used). I think this should be fixed in pytest instead (pytest-dev/pytest#5996).

[blueyed]

Well, OTOH I've tried to mimic the behavior that TearDown gets called after SetUp always (in non-debug mode), which is not the case with _pre_setup and _post_teardown.
It explicitly raises (i.e. does not call it) for debug=True like unittest does.

Therefore re-opening - it was just driven by making Django behave like unittest in this regard.

I.e. the added test would fail, which means that _post_teardown is not called when you skip a test via a SkipTest exception - that certainly sounds wrong, doesn't it?

[asfaltboy]

It seems to me that SkipTest is the only exception that should be handled when skipping with debug=True.

Also, do we need to actually wrap the call super().__call__(result), given that _Outcome.testPartExecutor already catches all exceptions, nothing can be raised here (unless it's an exception happening before or after the setup/test/teardown flow).

It's important to note that it's possible to call SkipError from setUp and tearDown, so if we want to support the same in Django's _pre_setup and _post_teardown we should catch SkipTest exception here as well...

[felixxm]

It seems to me that SkipTest is the only exception that should be handled when skipping with debug=True.

Also, do we need to actually wrap the call super().__call__(result), given that _Outcome.testPartExecutor already catches all exceptions, nothing can be raised here (unless it's an exception happening before or after the setup/test/teardown flow).

I agree with these comments ☝️ .

It's important to note that it's possible to call SkipError from setUp and tearDown, so if we want to support the same in Django's _pre_setup and _post_teardown we should catch SkipTest exception here as well...

I'm not sure about this. What steps should we take if someone calls skipTest() in _pre_setup() or _post_teardown()?

[blueyed]

Sorry for not following up earlier.
JFI: pytest actually tries hard getting out of testPartExecutor, which causes _post_teardown not being called now with pytest 5.4 (via pytest-django, pytest-dev/pytest-django#824).
IIRC I've done this PR initially when I've looked at Django's code, without having in mind that pytest might end up doing it this way.

[felixxm]

I.e. the added test would fail, which means that _post_teardown is not called when you skip a test via a SkipTest exception - that certainly sounds wrong, doesn't it?

The new test doesn't use debug mode and works without any changes, we probably want to call

result = _DebugResult()
test_suite.run(result, debug=True)

instead of test_suite.run(self.get_runner()._makeResult()) 🤔.

It's strictly related with the previous fix, so we can use Refs #27391 -- ....

[blueyed]

The new test doesn't use debug mode and works without any changes

Oh, I must have missed this then - I've pretty sure to have checked that it failed without the patch - or isn't that what you've meant?

[felixxm]

Yes, exactly test_suite.run(self.get_runner()._makeResult()) doesn't use debug mode, so it calls _post_teardown() without this patch. Extra tests are fine, but should be added in a separate commit.

[blueyed]

Oh.. 😕 - pushed a fixup.

[felixxm]

@blueyed Are you going to address these suggestions and questions?

Please add self.isolate_debug_test(test_suite, result) cleanup to your test to avoid isolation issues.

[felixxm]

Closing due to inactivity.