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

[unittest] raise error if @skip is used with an argument that looks like a test method #78777

Closed
Naitreey mannequin opened this issue Sep 6, 2018 · 18 comments
Closed
Assignees
Labels
3.8 only security fixes extension-modules C modules in the Modules dir type-feature A feature request or enhancement

Comments

@Naitreey
Copy link
Mannequin

Naitreey mannequin commented Sep 6, 2018

BPO 34596
Nosy @rbtcollins, @ezio-melotti, @stevendaprano, @voidspace, @berkerpeksag, @zware, @serhiy-storchaka, @Naitreey
PRs
  • bpo-34596: Fallback to a default reason when @unittest.skip is uncalled #9082
  • [3.8] bpo-34596: Fallback to a default reason when @unittest.skip is uncalled (GH-9082) #15781
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/voidspace'
    closed_at = <Date 2019-09-09.14:44:10.065>
    created_at = <Date 2018-09-06.13:05:21.123>
    labels = ['extension-modules', 'type-feature', '3.8']
    title = '[unittest] raise error if @skip is used with an argument that looks like a test method'
    updated_at = <Date 2019-09-09.15:01:17.873>
    user = 'https://github.com/Naitreey'

    bugs.python.org fields:

    activity = <Date 2019-09-09.15:01:17.873>
    actor = 'python-dev'
    assignee = 'michael.foord'
    closed = True
    closed_date = <Date 2019-09-09.14:44:10.065>
    closer = 'michael.foord'
    components = ['Extension Modules']
    creation = <Date 2018-09-06.13:05:21.123>
    creator = 'Naitree Zhu'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 34596
    keywords = ['patch']
    message_count = 18.0
    messages = ['324688', '324689', '324691', '324692', '324697', '324699', '324700', '324701', '324703', '324836', '324853', '324903', '325308', '325417', '351431', '351444', '351471', '351493']
    nosy_count = 9.0
    nosy_names = ['rbcollins', 'ezio.melotti', 'steven.daprano', 'michael.foord', 'python-dev', 'berker.peksag', 'zach.ware', 'serhiy.storchaka', 'Naitree Zhu']
    pr_nums = ['9082', '15781']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue34596'
    versions = ['Python 3.8']

    @Naitreey
    Copy link
    Mannequin Author

    Naitreey mannequin commented Sep 6, 2018

    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.

    @Naitreey Naitreey mannequin added type-bug An unexpected behavior, bug, or error 3.7 only security fixes stdlib Python modules in the Lib dir tests Tests in the Lib/test dir 3.8 only security fixes labels Sep 6, 2018
    @stevendaprano
    Copy link
    Member

    Is there a use-case for reason to be anything but a string?

    @Naitreey
    Copy link
    Mannequin Author

    Naitreey mannequin commented Sep 6, 2018

    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.

    @zware
    Copy link
    Member

    zware commented Sep 6, 2018

    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)

    @Naitreey
    Copy link
    Mannequin Author

    Naitreey mannequin commented Sep 6, 2018

    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...)

    @zware
    Copy link
    Member

    zware commented Sep 6, 2018

    "Unconditionally" :)

    @Naitreey
    Copy link
    Mannequin Author

    Naitreey mannequin commented Sep 6, 2018

    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?

    @zware
    Copy link
    Member

    zware commented Sep 6, 2018

    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.

    @Naitreey
    Copy link
    Mannequin Author

    Naitreey mannequin commented Sep 6, 2018

    Ok, I see. Thank you.

    @serhiy-storchaka
    Copy link
    Member

    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.

    @serhiy-storchaka serhiy-storchaka added type-feature A feature request or enhancement and removed 3.7 only security fixes type-bug An unexpected behavior, bug, or error labels Sep 8, 2018
    @zware
    Copy link
    Member

    zware commented Sep 8, 2018

    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.

    @stevendaprano
    Copy link
    Member

    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.

    @berkerpeksag
    Copy link
    Member

    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.

    @Naitreey
    Copy link
    Mannequin Author

    Naitreey mannequin commented Sep 15, 2018

    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?

    @zware
    Copy link
    Member

    zware commented Sep 9, 2019

    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.

    @voidspace
    Copy link
    Contributor

    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.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 9, 2019

    New changeset d5fd75c by Michael Foord (Naitree Zhu) in branch 'master':
    bpo-34596: Fallback to a default reason when @unittest.skip is uncalled (bpo-9082)
    d5fd75c

    @voidspace voidspace added extension-modules C modules in the Modules dir and removed stdlib Python modules in the Lib dir tests Tests in the Lib/test dir labels Sep 9, 2019
    @voidspace voidspace self-assigned this Sep 9, 2019
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 9, 2019

    New changeset 3bd4bed by Michael Foord (Miss Islington (bot)) in branch '3.8':
    bpo-34596: Fallback to a default reason when @unittest.skip is uncalled (GH-9082) (bpo-15781)
    3bd4bed

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.8 only security fixes extension-modules C modules in the Modules dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants