Call cleanup functions if explicit teardown is needed

[scorphus]

Not calling cleanup functions after a test fails may affect other tests down the row.

This issue was introduced in 04f27d4 (git bisect FTW)

When a test fails, a GetOutOf_testPartExecutor exception is raised to make unittest halt testPartExecutor:

pytest/src/_pytest/unittest.py

Lines 206 to 228 in 413ca8a

	def runtest(self):
	# TODO: move testcase reporter into separate class, this shouldnt be on item
	import unittest
	testMethod = getattr(self._testcase, self._testcase._testMethodName)
	class _GetOutOf_testPartExecutor(KeyboardInterrupt):
	"""Helper exception to get out of unittests's testPartExecutor (see TestCase.run)."""
	@functools.wraps(testMethod)
	def wrapped_testMethod(*args, **kwargs):
	"""Wrap the original method to call into pytest's machinery, so other pytest
	features can have a chance to kick in (notably --pdb)"""
	try:
	self.ihook.pytest_pyfunc_call(pyfuncitem=self)
	except unittest.SkipTest:
	raise
	except Exception as exc:
	expecting_failure = self._expecting_failure(testMethod)
	if expecting_failure:
	raise
	self._needs_explicit_tearDown = True
	raise _GetOutOf_testPartExecutor(exc)

That prevents teardown and cleanup functions from being called. That's why self._needs_explicit_tearDown is set to True to then explicitly call teardown later on:

pytest/src/_pytest/unittest.py

Lines 122 to 126 in 413ca8a

	def teardown(self):
	if self._needs_explicit_tearDown:
	self._testcase.tearDown()
	self._testcase = None
	self._obj = None

But no cleanup functions are called. Hence this PR.

[blueyed]

Thanks for trying to tackle this, but it does not fix #6947 unfortunately (there is an explicit test there: #6947 (comment)).

[scorphus]

I fail to see what that test has to do with cleanups. Maybe it's a test for a different issue and it should be either renamed or reported as a new one. I'm also happy to investigate it.

This PR does not make that test pass, for sure, but it fixes the problem with cleanup functions not being called after a test failure and... a KeyboardInterrupt exception is raised.

Anyways I'll remove the Fix #6947 so it remains open for any sake.

Then, can you please accept this pull request and make a new release? This is a big blocker for me and my team – and I believe for others too.

Thanks a bunch!

[nicoddemus]

I fail to see what that test has to do with cleanups.

I agree, I left a comment there asking for clarification. 👍

This looks great to me, thanks for the PR!

Note to ourselves: we need to backport this to 5.4.x for the next release.

[nicoddemus]

Suggested change

	Call cleanup functions on test failure.
	Call cleanup functions of `unittest.TestCase` subclasses on test failures.

[scorphus]

@nicoddemus I'm up for backporting it. I'll lookup how to do it. Any links or hints?

[nicoddemus]

@nicoddemus I'm up for backporting it. I'll lookup how to do it. Any links or hints?

Thanks for the offer! Weirdly I can't find the instructions right now, but I'm sure @bluetech can point at them when he get's a chance.

(Btw @bluetech just noticed that this document also needs to be updated: https://github.com/pytest-dev/pytest/blob/master/doc/en/development_guide.rst)

[blueyed]

@nicoddemus
I've thought it was clear that "raising out of unittest" is the main problem, and this is just a side-effect of it. I.e. this fixes just one symptom of it.
When fixing it like this you basically need to also do everything else unittest is doing, e.g. feeding errors etc.
Therefore I still think it should be reverted instead (#6014) - given that nobody is working on fixing it "properly" anytime soon.

[nicoddemus]

Therefore I still think it should be reverted instead (#6014) - given that nobody is working on fixing it "properly" anytime soon.

I agree, but this still counts as a bug fix that can be released in the next patch release. Reverting it would require at least a minor version, and as I understand our users would like to get this released ASAP.

Merging this now won't make reverting #6014 any harder, and we will even have a regression test to boot to prevent further mistakes.

[RonnyPfannschmidt]

I agree with @nicoddemus 's assessment of the timeline.

[blueyed]

If you do not want to fix it proper I suggest taking #6972 instead - which basically is a revert of the "raising out of unittest".

[blueyed]

Merging this now won't make reverting #6014 any harder

#6014 was meant to be a revert.
By now #5996 would have to be reverted (and any follow-ups like this one (if merged)).

All I am saying is that it fixes one symptom, but e.g. pytest-django would still be affected (if it would not monkeypatch it out already).

[scorphus]

So, what's the verdict? Is that one small single line so bad compared to pytest not working as it should for a particular case?

Should I close this PR?

What are you going to do about it?

Thanks.

[nicoddemus]

Hi @scorphus,

Thanks again for the PR, and sorry for the delay! We were solving some internal issues.

This has been fixed (along with pdb/trace support) in #7151, which supersedes this PR.

We plan to release 5.4.2 soon, probably today. 👍

Again thanks for the PR and sorry for the long response time here.