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

Allow to skip unittests if --pdb active #2225

Merged

Conversation

mbyt
Copy link
Contributor

@mbyt mbyt commented Jan 31, 2017

One solution for #2137

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.08%) to 92.749% when pulling d1c7250 on mbyt:allow_skipping_unittests_with_pdb_active into 0931fe2 on pytest-dev:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.08%) to 92.749% when pulling 36b6f17 on mbyt:allow_skipping_unittests_with_pdb_active into 0931fe2 on pytest-dev:master.

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

Overall this looks pretty good, thanks @mbyt for contributing again! 👍

@@ -157,9 +156,20 @@ def runtest(self):
self._testcase(result=self)
else:
# disables tearDown and cleanups for post mortem debugging (see #1890)
# but still implements the skipping machinery (see #2137)
testMethod = getattr(self._testcase, self._testcase._testMethodName)
Copy link
Member

Choose a reason for hiding this comment

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

This looks correct according to the implementation in unittest, thanks!

Could you please move this entire logic to a separate method though? Something like:

...
else:
    if self._handle_skip():
        return
    self._testcase.debug()

This makes the code easier to follow IMHO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea, that makes it much easier to read. I did that in ad56cd8.

# If the class or method was skipped.
skip_why = (getattr(self._testcase.__class__, '__unittest_skip_why__', '') or
getattr(testMethod, '__unittest_skip_why__', ''))
try:
Copy link
Member

@nicoddemus nicoddemus Feb 1, 2017

Choose a reason for hiding this comment

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

I recommend making an explicit version check here; try/except (specially on a generic exception like TypeError) can catch the exception on some unintended part of the code, making it hard to understand/debug the problem.

In short:

if sys.version_info[0] == 3:
    self._testcase._addSkip(self, self._testcase, skip_why)
else:
    self._testcase._addSkip(self, skip_why)

Copy link
Contributor Author

@mbyt mbyt Feb 2, 2017

Choose a reason for hiding this comment

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

@nicoddemus that was intentionally, as the python3 _addSkip has the same signature as the unittest2 _addSkip on python2, see here.

However, it was implicit. I explicitly added a comment and secured the PY2 path in 176c680.

@nicoddemus
Copy link
Member

(Resolved the changelog conflict directly on the GH interface, that's neat!)

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.08%) to 92.749% when pulling 176c680 on mbyt:allow_skipping_unittests_with_pdb_active into da5a3db on pytest-dev:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 92.698% when pulling ad56cd8 on mbyt:allow_skipping_unittests_with_pdb_active into da5a3db on pytest-dev:master.

skip_why = (getattr(self._testcase.__class__, '__unittest_skip_why__', '') or
getattr(testMethod, '__unittest_skip_why__', ''))
try: # PY3, unittest2 on PY2
self._testcase._addSkip(self, self._testcase, skip_why)
Copy link
Member

Choose a reason for hiding this comment

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

Thanks @mbyt, now I see that it is support both unittest and unittest2 on Python 2.

A try/except for TypeError still makes me uneasy, but I have no better suggestion so let's go with it. 👍

@nicoddemus
Copy link
Member

If somebody would like more time to review this please let me know, otherwise I plan to merge this tomorrow evening.

@coveralls
Copy link

coveralls commented Feb 8, 2017

Coverage Status

Coverage decreased (-0.1%) to 92.698% when pulling 6a097aa on mbyt:allow_skipping_unittests_with_pdb_active into a4fb971 on pytest-dev:master.

@nicoddemus nicoddemus merged commit 3d9c5cf into pytest-dev:master Feb 8, 2017
@nicoddemus
Copy link
Member

Thanks again @mbyt!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants