classification
Title: [unittest] raise error if @skip is used with an argument that looks like a test method
Type: enhancement Stage: resolved
Components: Extension Modules Versions: Python 3.8
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: michael.foord Nosy List: Naitree Zhu, berker.peksag, ezio.melotti, michael.foord, python-dev, rbcollins, serhiy.storchaka, steven.daprano, zach.ware
Priority: normal Keywords: patch

Created on 2018-09-06 13:05 by Naitree Zhu, last changed 2019-09-09 15:01 by python-dev. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 9082 merged python-dev, 2018-09-06 16:41
PR 15781 merged miss-islington, 2019-09-09 14:07
Messages (18)
msg324688 - (view) Author: Naitree Zhu (Naitree Zhu) * Date: 2018-09-06 13:05
When using @skip decorator, `reason` argument is required. But one could easily overlook that and use it like so:

    class SomeTest(TestCase):

        @skip
        def test_method(self):
            # ...

The test actually passes when running with unittest, because it's equivalent to

    class SomeTest(TestCase):

        def test_method(self):
            # ...
        test_method = skip(test_method)

This gives the illusion that @skip decorator worked as expected.

I propose we should check the passed in `reason`, and raise error if it looks like a function/method, therefore prevent this erroneous usage.

In practice, this behavior has misled people using the decorator in the unintended way. For example: In this chapter of Test-Driven Development with Python (http://www.obeythetestinggoat.com/book/chapter_organising_test_files.html), the author used @skip decorator without `reason` argument.
msg324689 - (view) Author: Steven D'Aprano (steven.daprano) * (Python committer) Date: 2018-09-06 13:12
Is there a use-case for reason to be anything but a string?
msg324691 - (view) Author: Naitree Zhu (Naitree Zhu) * Date: 2018-09-06 13:56
Well, I personally can not think of any. I think `reason` should normally just be string.

If that is ok, I'll be happy to submit a PR that restricts reason to be a string.
msg324692 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2018-09-06 14:25
It could be interesting to enable uncalled `skip` by setting a default reason of "Unconditional skip" when the argument is a function.

Do note that decorating with an uncalled `skip` does actually work to skip the test currently, but the test is marked as success rather than skipped (see example pasted below).  For this reason, I don't think we should turn it into an error in maintenance releases, as anyone using this accidentally will suddenly have many failing tests.

$ cat test.py
from unittest import TestCase, skip


class Test(TestCase):

    def test_good(self):
        self.assertTrue(1.0)

    def test_bad(self):
        self.assertFalse(1.0)

    @skip
    def test_bad_skip(self):
        self.assertFalse(1.0)

    @skip('always skipped')
    def test_good_skip(self):
        self.assertFalse(1.0)

$ ./python.exe -m unittest test.py -v
test_bad (test.Test) ... FAIL
test_bad_skip (test.Test) ... ok
test_good (test.Test) ... ok
test_good_skip (test.Test) ... skipped 'always skipped'

======================================================================
FAIL: test_bad (test.Test)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/.../test.py", line 10, in test_bad
    self.assertFalse(1.0)
AssertionError: 1.0 is not false

----------------------------------------------------------------------
Ran 4 tests in 0.002s

FAILED (failures=1, skipped=1)
msg324697 - (view) Author: Naitree Zhu (Naitree Zhu) * Date: 2018-09-06 14:58
What would be a good default reason? How about the function name?

    if isinstance(reason, types.FunctionType):
        reason = reason.__name__

For example,

    from unittest import TestCase, skip

    class Test(TestCase):

        @skip
        def test_bad_skip(self):
            self.assertFalse(1.0)

        @skip('always skipped')
        def test_good_skip(self):
            self.assertFalse(1.0)

$ python -m unittest -v test.py 
test_bad_skip (test.Test) ... skipped 'test_bad_skip'
test_good_skip (test.Test) ... skipped 'always skipped'

----------------------------------------------------------------------
Ran 2 tests in 0.000s

OK (skipped=2)


But this wouldn't be very helpful if test method is a lambda (just saying...)
msg324699 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2018-09-06 15:16
"Unconditionally" :)
msg324700 - (view) Author: Naitree Zhu (Naitree Zhu) * Date: 2018-09-06 15:48
Hi @zach.ware, Just to make sure I'm getting this right (first time contributing to cpython...)

Now I need to open 4 PRs at GitHub,

- 1 PR to master branch, with following changes: raise TypeError when `reason` is not a string. (Include unit test.)
- 3 PRs to 3.7, 3.6, and 2.7, with following changes: Setting `reason` to "Unconditionally" when `reason` is a function. (Include unit test.)

Looking good?
msg324701 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2018-09-06 15:53
I think we should make the same change on all branches (why would we fix it in maintenance branches just to break it in the next major release?), in which case it's just one PR to master and our backport bot (and/or the merging core dev) will take care of it from there.
msg324703 - (view) Author: Naitree Zhu (Naitree Zhu) * Date: 2018-09-06 16:13
Ok, I see. Thank you.
msg324836 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-09-08 12:31
It doesn't look like a good design to me if allow a function be a decorator and a decorator fabric at the same time. It always lead to cumbersome and errorprone implementation. Currently there is only one example of such design in the stdlib, other propositions were rejected.

Checking that the argument is a string and raising exception otherwise looks good to me for 3.8. There is no a bug that need to be fixed in maintained versions. If you use unittest.skip() improperly, it is your failure. Helping to catch such mistakes is a new feature.
msg324853 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2018-09-08 17:32
I don't agree that this change makes the implementation significantly more cumbersome.  I also think there's a backward compatibility argument to be made for allowing the uncalled usage, particularly considering the OP's published example and other similar ones, such as https://stackoverflow.com/questions/2066508/disable-individual-python-unit-tests-temporarily which was for me the first Google result for "python unittest skip" that was not our own docs.
msg324903 - (view) Author: Steven D'Aprano (steven.daprano) * (Python committer) Date: 2018-09-10 00:36
For what its worth, I'm +1 with Serhiy that we raise an exception 
on a non-string argument, including the case where the caller forgets to 
provide a reason at all.

I don't think that @skip with no reason was ever intended to work (it 
certainly wasn't documented as working) and as this only effects tests, 
not production code, I don't think we ought to be constrained by "bug 
compatibility" concerns. Fixing broken tests is easier than fixing 
broken production code.
msg325308 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2018-09-13 23:44
It would be nice to make *reason* optional. Every time I needed to use @unittest.skip() (even if I wanted to use it temporarily), I ended up passing some random string as reason or use 'raise SkipTest' directly.

If we decide to keep *reason* required, I agree with Serhiy's proposal.
msg325417 - (view) Author: Naitree Zhu (Naitree Zhu) * Date: 2018-09-15 01:42
Hi guys, what's our consensus on this?

- raise an exception as a fix? or
- fallback to default `reason` as a new feature?

If we choose to explicitly make `reason` optional (I mean by documenting it as such), shouldn't we also change `@skipIf` and `@skipUnless` to keep consistency?
msg351431 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2019-09-09 11:30
I think we need Michael's ruling on which way to go here.  I'm fine with either an exception or a default reason, but still lean towards the default.  I don't think `skipIf` or `skipUnless` really need a change either way; they both require two arguments and thus can't be misused in the same way as `skip`.
msg351444 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2019-09-09 12:38
I'm in favour of a default and "Unconditionally skipped" is fine with me. Although "Skipped" would also be fine.

Making @skip work with no arguments is fine. Having to pass in arguments message arguments you don't want is a pain and there's no need to force the user.

Let's add a default message and make the skipped tests report as skipped properly on 3.8 and close it.
msg351471 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2019-09-09 14:06
New changeset d5fd75c53fad7049fc640c9a6162d35f0c5bea03 by Michael Foord (Naitree Zhu) in branch 'master':
bpo-34596: Fallback to a default reason when @unittest.skip is uncalled (#9082)
https://github.com/python/cpython/commit/d5fd75c53fad7049fc640c9a6162d35f0c5bea03
msg351493 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2019-09-09 15:01
New changeset 3bd4bed78a0b068e28bcf2242d33aed227c2532c by Michael Foord (Miss Islington (bot)) in branch '3.8':
bpo-34596: Fallback to a default reason when @unittest.skip is uncalled (GH-9082) (#15781)
https://github.com/python/cpython/commit/3bd4bed78a0b068e28bcf2242d33aed227c2532c
History
Date User Action Args
2019-09-09 15:01:17python-devsetmessages: + msg351493
2019-09-09 14:44:10michael.foordsetstatus: open -> closed
assignee: michael.foord
resolution: fixed
components: + Extension Modules, - Library (Lib), Tests
stage: patch review -> resolved
2019-09-09 14:07:00miss-islingtonsetpull_requests: + pull_request15434
2019-09-09 14:06:56python-devsetnosy: + python-dev
messages: + msg351471
2019-09-09 12:38:02michael.foordsetmessages: + msg351444
2019-09-09 11:30:41zach.waresetmessages: + msg351431
2018-09-15 01:42:24Naitree Zhusetmessages: + msg325417
2018-09-13 23:44:47berker.peksagsetnosy: + berker.peksag
messages: + msg325308
2018-09-10 00:36:51steven.dapranosetmessages: + msg324903
2018-09-08 17:32:23zach.waresetmessages: + msg324853
2018-09-08 12:31:23serhiy.storchakasetversions: - Python 2.7, Python 3.6, Python 3.7
nosy: + serhiy.storchaka

messages: + msg324836

type: behavior -> enhancement
2018-09-06 16:41:30python-devsetkeywords: + patch
stage: patch review
pull_requests: + pull_request8540
2018-09-06 16:13:20Naitree Zhusetmessages: + msg324703
2018-09-06 15:54:00zach.waresetnosy: + rbcollins, ezio.melotti, michael.foord
2018-09-06 15:53:43zach.waresetmessages: + msg324701
2018-09-06 15:48:27Naitree Zhusetmessages: + msg324700
2018-09-06 15:16:06zach.waresetmessages: + msg324699
2018-09-06 14:58:52Naitree Zhusetmessages: + msg324697
2018-09-06 14:25:26zach.waresetnosy: + zach.ware

messages: + msg324692
versions: - Python 3.4, Python 3.5
2018-09-06 13:56:56Naitree Zhusetmessages: + msg324691
2018-09-06 13:12:08steven.dapranosetnosy: + steven.daprano
messages: + msg324689
2018-09-06 13:05:21Naitree Zhucreate