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

Jump to bottom

bpo-36674: Stops skipped tests from running in debug mode. #13180

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

bpo-36674: Stops skipped tests from running in debug mode. #13180

wants to merge 2 commits into from
+32 −1

Conversation

@lisroach
Copy link
Contributor

@lisroach lisroach commented May 7, 2019 •
edited by bedevere-bot

TestCase.run() will skip tests that have skip/skipIf/skipWhatever applied to them, this PR applies the same skipped functionality when tests are run using TestCase.debug().

https://bugs.python.org/issue36674

https://bugs.python.org/issue36674
mangrisano
mangrisano reviewed Jun 5, 2019
View changes
Lib/unittest/case.py
@@ -745,7 +745,11 @@ def __call__(self, *args, **kwds):
def debug(self):
"""Run the test without collecting errors in a TestResult"""
self.setUp()
getattr(self, self._testMethodName)()
testMethod = getattr(self, self._testMethodName)
if (not getattr(self.__class__, "__unittest_skip__", False) and
Copy link
Contributor

@mangrisano mangrisano Jun 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's not necessary use the round brackets for the condition.

Copy link
Contributor Author

@lisroach lisroach Jun 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean the parens around the if statement? It is not necessary, but encouraged by the style guide to wrap multi-line if statements in parens: https://www.python.org/dev/peps/pep-0008/#multiline-if-statements

Copy link
Contributor

@potomak potomak Jul 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's out of the scope of this PR, but I'd like to suggest a small refactoring to make this condition a little bit clearer.

  1. Define a new _is_skipped(self, testMethod) method:
def _is_skipped(self, testMethod):
    return any(
        getattr(t, "__unittest_skip__", False)
        for t in (self.__class__, testMethod)
    )
  1. Update the new condition to not self._is_skipped(testMethod) and the existing condition around line 665 to self._is_skipped(testMethod)

If it doesn't make sense to include this small update in this PR, but you think it makes sense in general, I'd be happy to work on it.

Lib/unittest/test/test_case.py
def test_something(self):
self.addCleanup(_skip)

for klass in (Test1, Test2, Test3, Test4):
Copy link
Contributor

@mangrisano mangrisano Jun 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What did you mean with "klass"?

Copy link
Contributor Author

@lisroach lisroach Jun 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a common variable name used throughout this file (see line 1774 for example), it's a variable representing a class.

@lisroach
Copy link
Contributor Author

@lisroach lisroach commented Jun 5, 2019

How do I kick the CI to rerun their tests? I think these failures were transient.

@asvetlov asvetlov closed this Jun 5, 2019
@asvetlov asvetlov reopened this Jun 5, 2019
@asvetlov
Copy link
Contributor

@asvetlov asvetlov commented Jun 5, 2019

Close and Reopen -- as I did just now

@lisroach
Copy link
Contributor Author

@lisroach lisroach commented Jun 6, 2019

Thanks @asvetlov!

@asfaltboy asfaltboy mentioned this pull request Oct 19, 2019
Closed
@asfaltboy
Copy link

@asfaltboy asfaltboy commented Oct 19, 2019 •
edited

It seems that TestCase.run() implementation uses the testPartExecutor context manager. It does so to wrap exceptions (and skips) raised in either setUp, the test method or tearDown calls:

cpython/Lib/unittest/case.py

Lines 654 to 662 in e4c431e

with outcome.testPartExecutor(self):
self._callSetUp()
if outcome.success:
outcome.expecting_failure = expecting_failure
with outcome.testPartExecutor(self, isTest=True):
self._callTestMethod(testMethod)
outcome.expecting_failure = False
with outcome.testPartExecutor(self):
self._callTearDown()

With this in mind, and considering the docs would it make more sense to wrap in try/except like this:

def debug(self):
    """Run the test without collecting errors in a TestResult"""
    try:
        self.setUp()
        getattr(self, self._testMethodName)()
        self.tearDown()
    except SkipError:
        pass
    while self._cleanups:
        function, args, kwargs = self._cleanups.pop(-1)
        function(*args, **kwargs)

Note: users and developers of pytest have been struggling with understanding the TestCase.debug() interface and use case for a while (with regards to launching postmortem, teardown and skipping). Here's one of the relevant ones: pytest-dev/pytest-django#772

serhiy-storchaka
serhiy-storchaka reviewed Aug 29, 2021
View changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

setUp() should not be run for skipped tests. Perhaps a SkipTest should be raised for skipped tests.

If SkipTest is raised it the test method, it should be ignored. If it is raised in setUp() -- I am not sure whether tearDown() and doCleanup() should still be called.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@potomak potomak

@serhiy-storchaka serhiy-storchaka

@mangrisano mangrisano

Assignees
No one assigned
Labels
awaiting core review CLA signed
Projects
None yet
Milestone
No milestone
8 participants
@lisroach @asvetlov @asfaltboy @potomak @serhiy-storchaka @mangrisano @the-knights-who-say-ni @bedevere-bot