bpo-34596: Fallback to a default reason when @unittest.skip is uncalled #9082

Naitreey · 2018-09-06T16:41:28Z

Provide a default reason string when @unittest.skip decorator is used in uncalled form.
Add a test case for this change.

https://bugs.python.org/issue34596

the-knights-who-say-ni · 2018-09-06T16:41:31Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

zware

LGTM. I would like to hear from the maintainers of unittest before merging, though.

berkerpeksag

https://docs.python.org/3/library/unittest.html#unittest.skip documentations needs to be updated as well.

berkerpeksag · 2018-09-13T23:24:09Z

Lib/unittest/case.py

@@ -98,6 +99,10 @@ def skip_wrapper(*args, **kwargs):
        test_item.__unittest_skip__ = True
        test_item.__unittest_skip_why__ = reason
        return test_item
+    if isinstance(reason, types.FunctionType):
+        test_item = reason
+        reason = 'unconditionally'


I think it would be better if set reason to None.

I would prefer to have the reason to "", this will be also similar to the Java world.

I'm fine with that. I suggested "unconditionally" mostly just because I had no better suggestion; the empty string probably should have been what I suggested in the first place :)

berkerpeksag · 2018-09-13T23:25:34Z

Misc/NEWS.d/next/Tests/2018-09-07-01-18-27.bpo-34596.r2-EGd.rst

@@ -0,0 +1,2 @@
+Fallback to a default reason when @unittest.skip is uncalled. Patch by


@unittest.skip -> :func:`unittest.skip`

kushaldas · 2018-09-14T18:54:00Z

Lib/unittest/case.py

@@ -98,6 +99,10 @@ def skip_wrapper(*args, **kwargs):
        test_item.__unittest_skip__ = True
        test_item.__unittest_skip_why__ = reason
        return test_item
+    if isinstance(reason, types.FunctionType):
+        test_item = reason
+        reason = 'unconditionally'


I would prefer to have the reason to "", this will be also similar to the Java world.

bedevere-bot · 2018-09-14T18:54:42Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Naitreey · 2018-09-16T13:52:40Z

I have made the requested changes; please review again. Except for the documentation part. From the ongoing discussion on bug tracker, I'm not sure this change is strictly for not breaking existing user code, or intended as a new feature.

bedevere-bot · 2018-09-16T13:52:42Z

Thanks for making the requested changes!

@zware, @kushaldas: please review the changes made to this pull request.

serhiy-storchaka · 2018-12-08T10:17:27Z

Even if this proposition be accepted, it is a new feature and should not be backported.

berkerpeksag

Looking at this again, now I agree with Serhiy's suggestion at https://bugs.python.org/issue34596#msg324836

All skip*() APIs require 'reason' and if we'd accept this PR as-is, I'm sure we are going to get "why don't you make reason optional in skipIf() as well?" reports. I prefer to keep these APIs simpler.

bedevere-bot · 2019-05-31T12:39:46Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

voidspace · 2019-09-09T12:45:26Z

I'm going to land this PR as is on 3.8 and master.

miss-islington · 2019-09-09T14:06:51Z

Thanks @Naitreey for the PR, and @voidspace for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

bedevere-bot · 2019-09-09T14:06:51Z

@voidspace: Please replace # with GH- in the commit message next time. Thanks!

…ed (pythonGH-9082) * bpo-34596: Fallback to a default reason when @unittest.skip is uncalled * Change default reason to empty string * Fix rst formatting of NEWS entry (cherry picked from commit d5fd75c) Co-authored-by: Naitree Zhu <[email protected]>

bedevere-bot · 2019-09-09T14:07:02Z

GH-15781 is a backport of this pull request to the 3.8 branch.

…ed (GH-9082) (#15781) * bpo-34596: Fallback to a default reason when @unittest.skip is uncalled * Change default reason to empty string * Fix rst formatting of NEWS entry (cherry picked from commit d5fd75c) Co-authored-by: Naitree Zhu <[email protected]>

…ed (python#9082) * bpo-34596: Fallback to a default reason when @unittest.skip is uncalled * Change default reason to empty string * Fix rst formatting of NEWS entry

the-knights-who-say-ni added the CLA not signed label Sep 6, 2018

bedevere-bot added the awaiting review label Sep 6, 2018

bpo-34596: Fallback to a default reason when @unittest.skip is uncalled

64b27d7

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Sep 7, 2018

zware added needs backport to 3.6 labels Sep 7, 2018

zware approved these changes Sep 7, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Sep 7, 2018

berkerpeksag reviewed Sep 13, 2018

View reviewed changes

kushaldas requested changes Sep 14, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting merge labels Sep 14, 2018

Naitreey added 2 commits September 16, 2018 20:31

Change default reason to empty string

3ceddb4

Fix rst formatting of NEWS entry

fbf2cdc

bedevere-bot added awaiting change review and removed awaiting changes labels Sep 16, 2018

serhiy-storchaka added type-feature A feature request or enhancement and removed needs backport to 2.7 labels Dec 8, 2018

csabella requested review from kushaldas and zware May 31, 2019 11:10

csabella requested review from berkerpeksag and removed request for zware May 31, 2019 11:10

berkerpeksag requested changes May 31, 2019

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting change review labels May 31, 2019

voidspace self-assigned this Sep 9, 2019

voidspace added the needs backport to 3.8 label Sep 9, 2019

voidspace merged commit d5fd75c into python:master Sep 9, 2019

bedevere-bot removed the awaiting changes label Sep 9, 2019

bedevere-bot removed the needs backport to 3.8 label Sep 9, 2019

		@@ -0,0 +1,2 @@
		Fallback to a default reason when @unittest.skip is uncalled. Patch by

Uh oh!

bpo-34596: Fallback to a default reason when @unittest.skip is uncalled #9082

bpo-34596: Fallback to a default reason when @unittest.skip is uncalled #9082

Uh oh!

Conversation

Naitreey commented Sep 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

the-knights-who-say-ni commented Sep 6, 2018

Uh oh!

zware left a comment

Choose a reason for hiding this comment

Uh oh!

berkerpeksag left a comment

Choose a reason for hiding this comment

Uh oh!

berkerpeksag Sep 13, 2018

Choose a reason for hiding this comment

Uh oh!

kushaldas Sep 14, 2018

Choose a reason for hiding this comment

Uh oh!

zware Sep 14, 2018

Choose a reason for hiding this comment

Uh oh!

berkerpeksag Sep 13, 2018

Choose a reason for hiding this comment

Uh oh!

kushaldas Sep 14, 2018

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Sep 14, 2018

Uh oh!

Naitreey commented Sep 16, 2018

Uh oh!

bedevere-bot commented Sep 16, 2018

Uh oh!

serhiy-storchaka commented Dec 8, 2018

Uh oh!

berkerpeksag left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented May 31, 2019

Uh oh!

voidspace commented Sep 9, 2019

Uh oh!

miss-islington commented Sep 9, 2019

Uh oh!

bedevere-bot commented Sep 9, 2019

Uh oh!

bedevere-bot commented Sep 9, 2019

Uh oh!

Uh oh!

Naitreey commented Sep 6, 2018 •

edited

Loading