Skip to content
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

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

Conversation

@mbyt
Copy link
Contributor

@mbyt mbyt commented Aug 30, 2016

Assuming pytest is used to run a GUI test unittest suite. Typically GUI tests close the GUI in the unittest tearDowns or cleanup methods.

These tests cannot be easily post mortem debugged (pytest --pdb), as unittests tearDown and cleanup (which close the GUI) are called on an exception before the post mortem debugger jumps into place. Therefore, the GUI is closed and no direct interaction is possible any more.

A work around when running the unittest via the unittest runner is to add the following line to ~/.pdbrc: !import unittest; unittest.TestCase.run = lambda self,*args,**kw: unittest.TestCase.debug(self), start the tests with python -m pdb test.py and hit continue.

This pull request accomplishes the same within the pytest unittest compatibility layer. It is solely activated if pytest ist called with --pdb.

An example program which shows the problem would be the following (run with and without this change):

from __future__ import print_function
import unittest

class Blub(unittest.TestCase):
    def setUp(self):
        self.addCleanup(lambda: print("2 Cleanup"))

    def tearDown(self):
        print("1 tearDown")

    @classmethod
    def tearDownClass(self):
        print("3 tearDownClass")

    def test_false(self):
        self.assertTrue(False)

def suite():
    return unittest.makeSuite(Blub, 'test_')

if __name__ == '__main__':
    unittest.main(defaultTest='suite')
@mbyt
Copy link
Contributor Author

@mbyt mbyt commented Aug 30, 2016

Please let me know what you think about the problem. Is there another / more simple solution to this problem?

In case the proposed solution is sensible, I'll add a corresponding test.

@coveralls
Copy link

@coveralls coveralls commented Aug 30, 2016

Coverage Status

Coverage decreased (-0.01%) to 93.017% when pulling 4eeb475 on mbyt:disable_tearDown_and_cleanups_for_post_mortem_debugging into 82218e4 on pytest-dev:master.

@nicoddemus
Copy link
Member

@nicoddemus nicoddemus commented Aug 30, 2016

Hey @mbyt, thanks for the PR.

TBH I'm reluctant of adding such an esoteric use-case to pytest core, specially on the unittest layer. It seems problematic to simply disable teardowns completely when pytest is invoked with --pdb. On the other hand, one usually doesn't use the --pdb option unless it is running a specific test which is failing and he/she wants to debug.

You can accomplish something similar by adding this to your conftest.py:

def pytest_configure(config):
    if config.getvalue("usepdb"):
         import _pytest.unittest
         _pytest.unittest.TestCaseFunction.runtest = lambda self,*args,**kw: self._testcase.debug()
@nicoddemus
Copy link
Member

@nicoddemus nicoddemus commented Aug 30, 2016

Hmm now that I thought a little more and played around a bit, I think actually we somehow should make it possible to use --pdb with unittest subclasses.

Consider:

import unittest

class Test(unittest.TestCase):

    def tearDown(self):
        self.filename = None

    def test_1(self):
        assert 1

    def test_2(self):
        self.filename = 'debug me'
        assert 0

If --pdb is used, without @mbyt's PR there's no way to evaluate in the debugger what self.filename was on the failing test_2, so it is very limited at that point for actually debugging.

One might even argue that --pdb is currently broken when used in unittest sub-classes.

What do others think?

@RonnyPfannschmidt
Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt commented Aug 31, 2016

i believe this is something we want to add, but it should be accompanied with a test so we dont break it

@coveralls
Copy link

@coveralls coveralls commented Aug 31, 2016

Coverage Status

Coverage decreased (-0.01%) to 93.078% when pulling be08223 on mbyt:disable_tearDown_and_cleanups_for_post_mortem_debugging into 67ba8aa on pytest-dev:master.

@mbyt
Copy link
Contributor Author

@mbyt mbyt commented Aug 31, 2016

Thank you very much for your insights and your fast reply.

I added a corresponding test.

Another possible solution would be to read the .pdbrc during the initialization, which would allow to enable monkey patches as the one described.

@@ -91,6 +91,7 @@ Martin Prusse
Matt Bachmann
Matt Williams
Matthias Hafner
mbyt

This comment has been minimized.

@nicoddemus

nicoddemus Aug 31, 2016
Member

Don't you want to use your real name here?

This comment has been minimized.

@mbyt

mbyt Aug 31, 2016
Author Contributor

Thanks for letting me know. If you do not mind, let's leave it like this.

@@ -18,15 +18,22 @@
if a test suite uses ``pytest_plugins`` to load internal plugins (`#1888`_).
Thanks `@jaraco`_ for the report and `@nicoddemus`_ for the PR (`#1891`_).

* Do not call tearDown (and cleanups) when running unittest with ``--pdb``

This comment has been minimized.

@nicoddemus

nicoddemus Aug 31, 2016
Member

I think we should make crystal clear that this change only affects unittest-tests, so I suggest replacing:

Do not call tearDown (and cleanups) when running unittest with --pdb

With:

Do not call tearDown and cleanups when running tests from unittest.TestCase subclasses with --pdb

if self.config.pluginmanager.get_plugin("pdbinvoke") is None:
self._testcase(result=self)
else:
# disables tearDown and cleanups for post mortem debugging

This comment has been minimized.

@nicoddemus

nicoddemus Aug 31, 2016
Member

Please add "(see #1890)" to this comment for future reference

@nicoddemus
Copy link
Member

@nicoddemus nicoddemus commented Aug 31, 2016

Thanks! 😁

Other than my comments above, would you mind adding a small blurb to the docs mentioning this change? I think adding a .. note:: to unittest.rst right before the Mixing pytest fixtures into unittest.TestCase style tests section would be great, because the previous paragraph mentions using --pdb for debugging.

plus small scale refactoring
@mbyt
Copy link
Contributor Author

@mbyt mbyt commented Aug 31, 2016

@nicoddemus thanks again for looking at the changesets and your constructive comments. I modified the code accordingly.

@coveralls
Copy link

@coveralls coveralls commented Aug 31, 2016

Coverage Status

Coverage decreased (-0.01%) to 93.078% when pulling 696a911 on mbyt:disable_tearDown_and_cleanups_for_post_mortem_debugging into 67ba8aa on pytest-dev:master.

@coveralls
Copy link

@coveralls coveralls commented Aug 31, 2016

Coverage Status

Coverage decreased (-0.01%) to 93.078% when pulling e43d117 on mbyt:disable_tearDown_and_cleanups_for_post_mortem_debugging into 67ba8aa on pytest-dev:master.

@nicoddemus nicoddemus merged commit a094fb3 into pytest-dev:master Sep 1, 2016
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nicoddemus
Copy link
Member

@nicoddemus nicoddemus commented Sep 1, 2016

Thanks again!

@blueyed
Copy link
Contributor

@blueyed blueyed commented Apr 13, 2019

Just stumbled across this via #1932.
See #1932 (comment) for my comment on it.

These tests cannot be easily post mortem debugged (pytest --pdb), as unittests tearDown and cleanup (which close the GUI) are called on an exception before the post mortem debugger jumps into place. Therefore, the GUI is closed and no direct interaction is possible any more.

Why is pdb not invoked before the teardown, if the error is before it?
I think that's the real issue, and just skipping teardown for all tests in general when --pdb is used (which some might even use as a default..!) is rather bad.

@blueyed
Copy link
Contributor

@blueyed blueyed commented Apr 13, 2019

Ok, current code looks a bit differrent:
https://github.com/blueyed/pytest/blob/1beba4e69517e69c872fc4a04bed81f9bae1610a/src/_pytest/unittest.py#L219-L226

It might be fine then - let's followup on #1932 / close it if fixed.

blueyed added a commit to blueyed/pytest that referenced this pull request Oct 20, 2019
Reverts pytest-dev#1890, which needs to
be fixed/addressed in another way
(pytest-dev#5996).

Fixes pytest-dev#5991.
@blueyed
Copy link
Contributor

@blueyed blueyed commented Oct 20, 2019

#5996 fixes this.

blueyed added a commit to blueyed/pytest that referenced this pull request Oct 20, 2019
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
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 Nov 9, 2019
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 added a commit to blueyed/pytest that referenced this pull request Nov 9, 2019
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 added a commit to blueyed/pytest that referenced this pull request Nov 9, 2019
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 added a commit to blueyed/pytest that referenced this pull request Nov 9, 2019
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 added a commit to blueyed/pytest that referenced this pull request Nov 9, 2019
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 added a commit to blueyed/pytest that referenced this pull request Nov 9, 2019
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 added a commit to blueyed/pytest that referenced this pull request Nov 9, 2019
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 added a commit to blueyed/pytest that referenced this pull request Nov 9, 2019
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 added a commit to blueyed/pytest that referenced this pull request Nov 9, 2019
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
vinaycalastry added a commit to vinaycalastry/pytest that referenced this pull request Dec 11, 2019
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants