unittest.skip and other decorators should complain if the argument isn't callable

[geofft]

Bug report

Bug description:

The practical motivation here is that the third-party decorator parameterized.parameterized.expand from the parameterized project works by generating more test methods in the containing class, one for each expansion, and then returning None as the result of the decorator. So if you do something like

import unittest
import parameterized

class TestStuff(unittest.TestCase):
    @unittest.skip("these tests modify prod")
    @parameterized.parameterized.expand([("a", 1), ("b", 2)])
    def test_stuff(self, x, y):
        ...

then the tests actually get run. The correct way to implement this is the other way around - use @expand(...) @skip(...), and then parameterized will copy the skip wrapper, giving the behavior the programmer expects.

The underlying reason is that unittest.skip's decorator never actually calls the function its decorating and therefore doesn't care what it is:

>>> import unittest
>>> unittest.skip("these tests modify prod")(None)
<function skip.<locals>.decorator.<locals>.skip_wrapper at 0x10082c1f0>
>>> _()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Applications/Xcode.app/Contents/Developer/Library/Frameworks/Python3.framework/Versions/3.9/lib/python3.9/unittest/case.py", line 119, in skip_wrapper
    raise SkipTest(reason)
unittest.case.SkipTest: these tests modify prod

If unittest.skip(reason)(function) were to instead raise an error if callable(function) is false, instead of returning a wrapper, then the test case above would raise at import time, before any of the tests were run, indicating to the test author that they have implemented something incorrectly.

This should also apply to skipIf and skipUnless, and I think also to expectedFailure and removeHandler, the other decorators in unittest.

CPython versions tested on:

3.9, 3.10, 3.11, 3.12

Operating systems tested on:

Linux, macOS

[terryjreedy]

I think you have posted to the wrong issue tracker, as the bug is in the third-party callable, parameterized.parameterized.expand. It is not a decorator. A decorator is "A function returning another function ...", which allows chaining, whereas 'expand' returns the non-callable None. This, as you note, breaks chaining, an essential feature of decorators, and for any other decorator, not just the unittest skip functions. Using it as if it were a decorator abuses the syntax. It should better be used as what it is, a post-processor, with the proper non-deceptive syntax.

    def test_stuff(self, x, y):
        ...
    parameterized.parameterized.expand([("a", 1), ("b", 2)])(test_stuff)

(I am presuming that expand deletes 'test_stuff', so that its return should not be bound to 'test_stuff'.) The doc for expand should give the proper usage as the above, and should have a red warning that it is not a decorator and that if it used as if it were, it must be the topmost, outer 'decorator' and that any real decorators, such as but not limited to skips, must be applied before expand.

While we could change our non-buggy skip functions to cater to expand, I don't think we should. First, we should not encourage abuse of decorator syntax. Second, function application is normally not commutative, so that order of application usually matters, making having to think about proper order completely normal.

[sobolevn]

I agree with @terryjreedy, but I would also like to add that skip and friends can be applied to classes, not only methods. So, the rule would be much more complex than just checking if something is a callable.

[sobolevn]

@geofft you can also provide your own @skip decorator in parameterized which would know how to handle your special case.

[geofft]

I think you have posted to the wrong issue tracker, as the bug is in the third-party callable, parameterized.parameterized.expand.

Yes, I debated which project to post this bug in. But, ultimately, unittest is the only code that is in a position to do anything about this. Even if parameterized.parameterized.expand actually returned a function, it would not be able to detect that the function it returned was about to be decorated by another decorator: precisely because the purpose of skip is that it doesn't call the decorated function, it is impossible for anything to detect that it's being decorated by skip; the value is never used.

As a result, it is therefore unable to apply the second decoration to its copies. In other words, it is not the failure of expand to return a function that breaks chaining - it is the fact that (by its nature) it has a side effect of creating additional functions, and those copies diverge from the chain. It would be equally broken if it returned a function.

So, yes, I am asking for unittest to go out of its way to be helpful, even though strictly speaking it's not unittest's problem.

Second, function application is normally not commutative, so that order of application usually matters, making having to think about proper order completely normal.

Yes, you should have to think about the proper order. But Python generally tries to be helpful about mistakes made by the programmer. In recent versions it will suggest fixes to typos etc. And it's pretty common in the stdlib for functions to do type checks in advance of when a value is used, and perhaps even if the value is never actually used. From a quick grep for if not callable, I see examples like shutil.register_archive_format, which checks that its argument is a callable even if you never ask for an archive of that format, or various functions in argparse, which check that an action is a callable even if you never parse the argument that has that action. The current behavior certainly feels fine from, say, the UNIX-ish worldview that you're expected to know what you're doing and it should treat mistakes as intentional, but it doesn't seem like it matches Python's worldview.

I am not proposing a change to skip that makes it commute with expand. I'm just proposing that skip raise an error if it's called in an erroneous way. I'm not even proposing that the error message detect this particular problem and advise you of how to get it right (though it certainly could, there's enough information to do that). The error message I'm asking for is simply something like TypeError: NoneType is not callable. The programmer would still have to figure out why this error is happening - but the error would be enough to make them think about the proper order.

skip and friends can be applied to classes, not only methods. So, the rule would be much more complex than just checking if something is a callable.

Classes are callable.
`

you can also provide your own @skip decorator in parameterized which would know how to handle your special case.

Just to be clear, I'm not involved with the parameterized project. I'm reporting this because this is an actual real-world issue from a user of both unittest and parameterized. I don't think having a new decorator in parameterized would have helped - there would be a serious discoverability problem. It's pretty common for someone who isn't the original author of a unit test to have to add @skip/@skipIf; they might not even be familiar with the parameterized project and just sort of guess what it does from context clues. (And as outlined above I don't think you could even implement such a decorator, anyway.)

[geofft]

First, we should not encourage abuse of decorator syntax.

PEP 614 is explicitly motivated by wanting to use decorator syntax for something that doesn't return a callable - connect returns a Connection object, which is not callable. That return value is essentially unused and not interesting to the program; it happens to be assigned to the function's name but nothing usually refers to it again. The whole purpose of the PEP is to make it easier to use decorator syntax for this use case, and it was accepted and implemented. So I don't think I will really get anywhere by trying to argue to the parameterized project that core Python believes this is not how the syntax should be used. :)

I'm not asking for anything nearly as significant as that PEP, which allowed using syntax that was previously disallowed. I'm just asking for a stdlib function to raise TypeError when it has an argument of the wrong type, as countless other functions do, which only has the effect of breaking code that is already buggy. You can use expand with decorator syntax today, and if you get the order right it works fine, so I don't think refusing this change will do anything to discourage its use - it's already perfectly usable.

[terryjreedy]

Thank you for the good comments. While your request may look plausible, I see 3 problems. First, the fix is fragile. The exception signal could be avoided/evaded just by changing the expand return from None to lambda: None. Second, reducing the domain of skip from any object to callables is not needed by skip itself. Functions check parameters and stop only when they cannot properly finish. Third, reducing the domain is an API change that will break working code for anyone else using a test decorator that returns None or any other none-callable. We cannot assume that parameterized people are the only ones to do so. Such a change would require a deprecation period and internal necessity that is no present here.

Your point that @Skip may be added by maintainers convinces me even more strongly that using expand as currently written as a decorator is a mistake, a bug trap. (But see below.) Such people should be able to use skip in the documented manner.

If expand is used as a decorator, it needs to detect if it is decorated, not immediately, but at least before a test function would be called. If a test is to be skipped, it is wrapped with a skipper with custom skip attributes:

from unittest import skip, skipIf, skipUnless, TestCase
class T(TestCase):
    def f(self): pass

    @skip('skip')
    def fskip(self): pass

    @skipIf(True, 'skip_if')
    def fif(self): pass

    @skipUnless(False, 'skip_unless')
    def funless(self): pass

for f in T.fskip, T.fif, T.funless:
    print(f"{f.__unittest_skip__}, '{f.__unittest_skip_why__}', {f.__wrapped__}")

produces

True, 'reason skip', <function T.fskip at 0x000001EABF6854E0>
True, 'reason skip_if', <function T.fif at 0x000001EABF685620>
True, 'reason skip_unless', <function T.funless at 0x000001EABF685760>

As near as I can tell, __unittest_skip__, etc, are only present in a skip wrapper. Expand in your example should be able to wrap its generated test functions with something like:

@skipIf(getattr(TestStuff.test_stuff, '__unittest_skip__', False), getattr(TestStuff.test_stuff, '__unittest_skip_why__', '')

and have an outer skip/If/Until/ work.

[terryjreedy]

I had nothing to do with PEP 614. It did not enable the use of connect, it made existing uses less ugly than the 3 workarounds presented. I don't think it was meant to encourage non-callable returns and side-effects. Still, it suggests that the glossary entry might need revision, with a note about the chain and side-effect issues.