unittest: do not use TestCase.debug() with `--pdb` #5996

blueyed · 2019-10-18T16:27:34Z

TODO:

changelog
doc update (https://github.com/pytest-dev/pytest/pull/1890/files#diff-c4141365b63ea92331fc88e5f7cc7abf)
rebase on features: while being a bugfix, it certainly changes behavior (e.g. with pytest-django)

nicoddemus

Are you positive this is the right approach? I ask because the original change was meant for people to be able to debug teardown methods in unittest.TestCase subclasses.

Reverts pytest-dev#1890, which needs to be fixed/addressed in another way (pytest-dev#5996). Fixes pytest-dev#5991.

blueyed · 2019-10-26T01:41:54Z

=> #6014 for now.

Fixes #772. Ref: pytest-dev/pytest#5996

blueyed · 2019-11-09T21:14:47Z

I've also tried invoking the pdbinvoke plugin directly from within the wrapped function (simpler in general), but it missing properties that come via from_call etc normally.
This would basically make it more behave like "just put your pdb.set_trace" where you need it.
So given that it might be better to have a more specific method that could be used here then (from within the wrapped call), when --pdb is used - or give using the existing post_mortem a try.

Fixes pytest-dev#5991 Fixes pytest-dev#3823 Ref: pytest-dev/pytest-django#772 Ref: pytest-dev#1890 Ref: pytest-dev/pytest-django#782 - inject wrapped testMethod - adjust test_trial_error - add test for `--trace` with unittests

nicoddemus · 2019-11-11T22:09:37Z

Thanks @blueyed, I'm convinced that we should not use TestCase.debug() at all.

I think we should pdbinvoke directly in the addError and addFailure if --pdb is passed in the command line. I will try to make this in a separate PR, if I'm successful, I believe this will supersed this one and #6014.

nicoddemus · 2019-11-12T15:17:51Z

I think we should pdbinvoke directly in the addError and addFailure if --pdb is passed in the command line.

Just tried that, but unfortunately unittest calls tearDown before reporting errors and failures to the underlying "result" object (comments are mine):

            if outcome.success:
                outcome.expecting_failure = expecting_failure
                with outcome.testPartExecutor(self, isTest=True):
                    self._callTestMethod(testMethod)  # <-- call test method
                outcome.expecting_failure = False
                with outcome.testPartExecutor(self):
                    self._callTearDown()  # <-- call teardown

            self.doCleanups()
            for test, reason in outcome.skipped:
                self._addSkip(result, test, reason)
            self._feedErrorsToResult(result, outcome.errors)  # <-- call addError, addFailure, etc.

So unittest doesn't really gives us a chance to "hook" into the result of the test method call for pytest to jump into the debugger, before tearDown is called.

You approach (wrapping the actual method call) seems like the only one that can actually work, thanks for looking at it into such detail.

- Isolate logic for getting expected exceptions - Use original method name, as users see it when entering the debugger

nicoddemus

Thanks a lot @blueyed, for the research and patience.

While making my comments I was also making the changes myself and testing them, so I decided to just push my changes intead of posting a bunch of diff/suggestions. Feel free to take a look at my comment and accept/reject what you think it is relevant. 👍

Otherwise this LGTM and ready to merge once CI passes.

Thanks again.

nicoddemus · 2019-11-12T15:26:54Z

src/_pytest/unittest.py

+
+                raise _GetOutOf_testPartExecutor(exc)
+
+        self._testcase._wrapped_testMethod = wrapped_testMethod


I think we should override the original method, otherwise the internal name will show up to the user when entering PDB:

self = <foo.T testMethod=_wrapped_testMethod> def test_foo(self): self.filename = 'hello' > assert 0

nicoddemus · 2019-11-12T15:27:25Z

src/_pytest/unittest.py

+        class _GetOutOf_testPartExecutor(KeyboardInterrupt):
+            """Helper exception to get out of unittests's testPartExecutor."""
+
+        unittest = sys.modules.get("unittest")


At this point unittest has definitely been imported, no need to guard against that. 👍

nicoddemus · 2019-11-12T15:44:59Z

src/_pytest/unittest.py

@@ -115,6 +117,8 @@ def setup(self):
            self._request._fillfixtures()

    def teardown(self):
+        if self._need_tearDown:


Are you sure we need this? Even if I comment these two lines, tear down still gets called for this example:

from unittest import TestCase, expectedFailure class T(TestCase): def tearDown(self): print('*********** tearDown') self.filename = None @expectedFailure def test_foo(self): self.filename = 'hello' assert 0

λ pytest .tmp\foo.py -sq *********** tearDown x 1 xfailed in 0.05s

Uncommenting them actually shows tearDown being called twice:

λ pytest .tmp\foo.py -sq *********** tearDown x*********** tearDown

Nevermind, we actually need it, but only if a test is not an expected failure

nicoddemus · 2019-11-12T15:55:49Z

doc/en/unittest.rst

@@ -238,17 +238,6 @@ was executed ahead of the ``test_method``.

 .. _pdb-unittest-note:

-.. note::
-
-    Running tests from ``unittest.TestCase`` subclasses with ``--pdb`` will


Otherwise 'normal' failures won't call teardown explicitly

blueyed · 2019-11-12T17:33:09Z

src/_pytest/unittest.py

@@ -217,6 +220,7 @@ def wrapped_testMethod(*args, **kwargs):
                expecting_failure = self._expecting_failure(testMethod)
                if expecting_failure:
                    raise
+                self._needs_explicit_tearDown = True
                raise _GetOutOf_testPartExecutor(exc)

        setattr(self._testcase, self._testcase._testMethodName, wrapped_testMethod)


Does it pass #5996 (comment) then?

blueyed · 2019-11-12T17:35:48Z

Thanks for looking into this!
I think it is worth revisiting completely though.
After all using post_mortem(sys.exc_info()[2]) in except Exception: comes a long way already, and it should be possible to invoke the normal hooks from there, so that e.g. capturing etc is handled.
It likely means to decouple / change the code around CallInfo.from_call etc though.
I will look into that later.

nicoddemus · 2019-11-12T17:41:58Z

I will look into that later.

Sounds good, but we can consider merging it already (as it solves a bunch of issues and doesn't introduce regressions or external changes), and follow up later with any other changes that improve it. What do you think?

blueyed · 2019-11-12T19:13:20Z

I'd like to avoid unnecessary diff churn, and think it might be a good opportunity to improve internals, since it shows that it is currently not flexible enough.
See blueyed#107 for a dirty POC.

nicoddemus · 2019-12-03T14:15:52Z

@blueyed I figured you won't be working on this anymore, so decided to re-open it, fix conflicts and merge it.

Thanks again!

Fixes pytest-dev/pytest-django#772. Ref: pytest-dev/pytest#5996

blueyed added type: bug problem that needs to be addressed plugin: unittest related to the unittest integration builtin plugin plugin: debugging related to the debugging builtin plugin labels Oct 18, 2019

This was referenced Oct 18, 2019

pytest --pdb does not run tearDown for skipped tests #5991

Closed

TestCase.skipTest() skips _post_teardown pytest-dev/pytest-django#772

Closed

pytest --pdb is no longer working with Flask_testing #1932

Closed

blueyed force-pushed the unittest-debug branch from 834ca0a to ae8c158 Compare October 18, 2019 19:24

blueyed changed the base branch from master to features October 18, 2019 19:24

blueyed force-pushed the unittest-debug branch from ae8c158 to c7eeb05 Compare October 18, 2019 21:01

nicoddemus approved these changes Oct 18, 2019

View reviewed changes

This comment has been minimized.

Sign in to view

This was referenced Oct 18, 2019

teardown_class not being run #5201

Closed

#3823 Fix trace option for unittest test cases #4083

Closed

blueyed changed the title ~~unittest: do not use TestCase.debug() with --pdb~~ [WIP] unittest: do not use TestCase.debug() with --pdb Oct 18, 2019

This comment has been minimized.

Sign in to view

blueyed mentioned this pull request Oct 19, 2019

TestCase: call _post_teardown always when _pre_setup worked django/django#11938

Closed

blueyed force-pushed the unittest-debug branch from c7eeb05 to cd4f7a4 Compare October 20, 2019 16:18

blueyed added a commit to blueyed/pytest that referenced this pull request Oct 20, 2019

unittest: do not use TestCase.debug with --pdb

1f4acae

Reverts pytest-dev#1890, which needs to be fixed/addressed in another way (pytest-dev#5996). Fixes pytest-dev#5991.

This was referenced Oct 20, 2019

unittest: do not use TestCase.debug with --pdb #6014

Closed

unittest runner: avoid tearDown and cleanup to ease post mortem debugging #1890

Merged

blueyed added a commit to blueyed/pytest that referenced this pull request Oct 20, 2019

unittest: do not use TestCase.debug with --pdb

ac53a82

Reverts pytest-dev#1890, which needs to be fixed/addressed in another way (pytest-dev#5996). Fixes pytest-dev#5991.

blueyed added a commit to blueyed/pytest that referenced this pull request Oct 21, 2019

unittest: do not use TestCase.debug with --pdb

fac9331

Reverts pytest-dev#1890, which needs to be fixed/addressed in another way (pytest-dev#5996). Fixes pytest-dev#5991.

blueyed closed this Oct 26, 2019

blueyed deleted the unittest-debug branch October 26, 2019 01:41

blueyed mentioned this pull request Oct 30, 2019

unittest: do not use TestCase.debug with --pdb blueyed/pytest#25

Merged

blueyed restored the unittest-debug branch November 9, 2019 05:00

blueyed reopened this Nov 9, 2019

blueyed force-pushed the unittest-debug branch from cd4f7a4 to 216d874 Compare November 9, 2019 05:03

blueyed added a commit to pytest-dev/pytest-django that referenced this pull request Nov 9, 2019

Monkeypatch pytest instead of Django to workaround pytest bug (#782)

89dd53b

Fixes #772. Ref: pytest-dev/pytest#5996

blueyed force-pushed the unittest-debug branch 5 times, most recently from 013a538 to 4b922e5 Compare November 9, 2019 12:27

unittest: do not use TestCase.debug() with --pdb

04f27d4

Fixes pytest-dev#5991 Fixes pytest-dev#3823 Ref: pytest-dev/pytest-django#772 Ref: pytest-dev#1890 Ref: pytest-dev/pytest-django#782 - inject wrapped testMethod - adjust test_trial_error - add test for `--trace` with unittests

blueyed force-pushed the unittest-debug branch from 4b922e5 to 04f27d4 Compare November 9, 2019 23:21

No need to call tearDown on expected failures

f7b1de7

- Isolate logic for getting expected exceptions - Use original method name, as users see it when entering the debugger

nicoddemus approved these changes Nov 12, 2019

View reviewed changes

nicoddemus reviewed Nov 12, 2019

View reviewed changes

Bring back explicit tear down

5936965

Otherwise 'normal' failures won't call teardown explicitly

blueyed commented Nov 12, 2019

View reviewed changes

blueyed mentioned this pull request Nov 12, 2019

[WIP/blocked] Factor out runner._report_for_call, using item._current_callinfo blueyed/pytest#107

Open

3 tasks

blueyed closed this Nov 27, 2019

nicoddemus reopened this Dec 3, 2019

Merge branch 'features' into unittest-debug

41b7b10

nicoddemus merged commit 1c4a672 into pytest-dev:features Dec 3, 2019

blueyed deleted the unittest-debug branch December 3, 2019 14:35

bluetech mentioned this pull request Mar 13, 2020

pytest 5.4 skips Django's teardown (causing e.g. "connection already closed" errors) pytest-dev/pytest-django#824

Closed

RonnyPfannschmidt mentioned this pull request Mar 14, 2020

pytest 5.4+ doesn't handle IsolatedAsyncioTestCase from CPython core. #6924

Closed

blueyed mentioned this pull request Apr 9, 2020

Call cleanup functions if explicit teardown is needed #7049

Closed

beyondgeeks pushed a commit to beyondgeeks/django-pytest that referenced this pull request Sep 6, 2022

Monkeypatch pytest instead of Django to workaround pytest bug (#782)

6823ae9

Fixes pytest-dev/pytest-django#772. Ref: pytest-dev/pytest#5996

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

unittest: do not use TestCase.debug() with `--pdb` #5996

unittest: do not use TestCase.debug() with `--pdb` #5996

blueyed commented Oct 18, 2019 •

edited

Loading

nicoddemus left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

blueyed commented Oct 26, 2019 •

edited

Loading

blueyed commented Nov 9, 2019

nicoddemus commented Nov 11, 2019

nicoddemus commented Nov 12, 2019

nicoddemus left a comment

nicoddemus Nov 12, 2019

nicoddemus Nov 12, 2019

nicoddemus Nov 12, 2019

nicoddemus Nov 12, 2019

nicoddemus Nov 12, 2019

blueyed Nov 12, 2019

nicoddemus Nov 12, 2019

blueyed commented Nov 12, 2019

nicoddemus commented Nov 12, 2019

blueyed commented Nov 12, 2019

nicoddemus commented Dec 3, 2019


		raise _GetOutOf_testPartExecutor(exc)

		self._testcase._wrapped_testMethod = wrapped_testMethod

unittest: do not use TestCase.debug() with --pdb #5996

unittest: do not use TestCase.debug() with --pdb #5996

Conversation

blueyed commented Oct 18, 2019 • edited Loading

nicoddemus left a comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

blueyed commented Oct 26, 2019 • edited Loading

blueyed commented Nov 9, 2019

nicoddemus commented Nov 11, 2019

nicoddemus commented Nov 12, 2019

nicoddemus left a comment

Choose a reason for hiding this comment

nicoddemus Nov 12, 2019

Choose a reason for hiding this comment

nicoddemus Nov 12, 2019

Choose a reason for hiding this comment

nicoddemus Nov 12, 2019

Choose a reason for hiding this comment

nicoddemus Nov 12, 2019

Choose a reason for hiding this comment

nicoddemus Nov 12, 2019

Choose a reason for hiding this comment

blueyed Nov 12, 2019

Choose a reason for hiding this comment

nicoddemus Nov 12, 2019

Choose a reason for hiding this comment

blueyed commented Nov 12, 2019

nicoddemus commented Nov 12, 2019

blueyed commented Nov 12, 2019

nicoddemus commented Dec 3, 2019

unittest: do not use TestCase.debug() with `--pdb` #5996

unittest: do not use TestCase.debug() with `--pdb` #5996

blueyed commented Oct 18, 2019 •

edited

Loading

blueyed commented Oct 26, 2019 •

edited

Loading