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.py patch: add skipped test functionality #40949

Closed
rblank mannequin opened this issue Sep 24, 2004 · 27 comments
Closed

unittest.py patch: add skipped test functionality #40949

rblank mannequin opened this issue Sep 24, 2004 · 27 comments
Assignees
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@rblank
Copy link
Mannequin

rblank mannequin commented Sep 24, 2004

BPO 1034053
Nosy @birkenfeld, @rhettinger, @ncoghlan, @pitrou, @giampaolo, @benjaminp
Files
  • unittest_skip.patch: Patch against unittest.py from Python 2.3.3
  • testSkippedTest.py: Test suite for test-skipping functionality
  • SkippedTestDemo.py: Sample code for test-skipping functionality
  • skip.diff
  • unittest_galore.patch
  • 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/benjaminp'
    closed_at = <Date 2009-03-23.21:51:07.758>
    created_at = <Date 2004-09-24.14:08:18.000>
    labels = ['type-feature', 'library']
    title = 'unittest.py patch: add skipped test functionality'
    updated_at = <Date 2009-03-23.21:51:07.757>
    user = 'https://bugs.python.org/rblank'

    bugs.python.org fields:

    activity = <Date 2009-03-23.21:51:07.757>
    actor = 'benjamin.peterson'
    assignee = 'benjamin.peterson'
    closed = True
    closed_date = <Date 2009-03-23.21:51:07.758>
    closer = 'benjamin.peterson'
    components = ['Library (Lib)']
    creation = <Date 2004-09-24.14:08:18.000>
    creator = 'rblank'
    dependencies = []
    files = ['6263', '6264', '6265', '11098', '13399']
    hgrepos = []
    issue_num = 1034053
    keywords = ['patch']
    message_count = 27.0
    messages = ['46926', '46927', '46928', '46929', '46930', '46931', '46932', '46933', '46934', '46935', '46936', '46937', '46938', '46939', '46940', '61284', '61492', '61517', '71014', '71015', '78409', '78424', '78467', '83975', '83977', '83992', '84042']
    nosy_count = 10.0
    nosy_names = ['georg.brandl', 'rhettinger', 'calvin', 'purcell', 'ncoghlan', 'pitrou', 'rblank', 'giampaolo.rodola', 'pupeno', 'benjamin.peterson']
    pr_nums = []
    priority = 'normal'
    resolution = 'accepted'
    stage = 'patch review'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue1034053'
    versions = ['Python 3.1', 'Python 2.7']

    @rblank
    Copy link
    Mannequin Author

    rblank mannequin commented Sep 24, 2004

    I added the possibility for tests using the unittest.py
    framework to be skipped. Basically, I added two methods
    to TestCase:

    TestCase.skip(msg): skips test unconditionally
    TestCase.skipIf(expr, msg): skips test if expr is true

    These can be called either in setUp() or in the test
    methods. I also added reporting of skipped tests to
    TestResult, _TextTestResult and TextTestRunner. If no
    tests are skipped, everything should be the same as
    before.

    I am using Python 2.3.3, so the changes are against the
    file in that version. I can generate a patch for a more
    recent version if desired. I attached the patch against
    the original (unittest_skip.patch). I can provide a
    complete test suite for the new functionality and a usage
    example program.

    Quick usage example:

    class ReadShadowTest(unittest.TestCase):
            """Read access to /etc/shadow"""
            def testReadingAsRoot(self):
                    """Reading /etc/shadow as root"""
                    self.skipIf(os.geteuid() != 0, "Must be root")
                    open("/etc/shadow").close()

    The example program produces the following output:

    $ ./SkippedTestDemo.py -v
    Access to autoexec.bat ... SKIPPED (Only available on 
    Windows)
    Access to config.sys ... SKIPPED (Only available on 
    Windows)
    Reading /etc/shadow as root ... SKIPPED (Must be root)
    Reading /etc/shadow as non-root ... ok
    
    -------------------------------------------------------
    

    Ran 4 tests in 0.004s

    OK (skipped=3)

    @rblank rblank mannequin assigned purcell Sep 24, 2004
    @rblank rblank mannequin added the stdlib Python modules in the Lib dir label Sep 24, 2004
    @rblank rblank mannequin assigned purcell Sep 24, 2004
    @rblank rblank mannequin added the stdlib Python modules in the Lib dir label Sep 24, 2004
    @rhettinger
    Copy link
    Contributor

    Logged In: YES
    user_id=80475

    Will this muck up some the existing test runners that people
    have written?

    @rblank
    Copy link
    Mannequin Author

    rblank mannequin commented Sep 25, 2004

    Logged In: YES
    user_id=568100

    I don't think so. Basically, the patch changes the following:

    • Adds a class SkippedException to the unittest module
    • Adds a skipped attribute to TestResult, containing the list of skipped
      tests, and an addSkipped() method to add to the list.
    • Catches the SkippedException in TestCase.__call__()
    • Adds skip() and skipIf() to TestCase
    • Modifies _TextTestResult and TextTestRunner to report skipped tests
      *only if there are any*

    I see two potential problems:

    • Test runners based on (or using the output of) TextTestRunner. I've
      taken care that the output is unchanged if there are no skipped tests.
    • Code that uses repr() of a TestResult, as I extended it to always report
      skipped tests. I think this would be bad practice anyway.

    However, to use the test-skipping functionality, custom test runners will
    obviously need to be extended to report skipped tests.

    OTOH, I don't have a big test codebase to check. I read that e.g. Zope is
    using unittest. Maybe I can try to run their test suite with the patched
    unittest.py. I'll check.

    @rblank
    Copy link
    Mannequin Author

    rblank mannequin commented Sep 25, 2004

    Logged In: YES
    user_id=568100

    The test suite for the added functionality.

    @rblank
    Copy link
    Mannequin Author

    rblank mannequin commented Sep 25, 2004

    Logged In: YES
    user_id=568100

    Added sample code.

    @calvin
    Copy link
    Mannequin

    calvin mannequin commented Sep 26, 2004

    Logged In: YES
    user_id=9205

    This is a nice patch. I am wondering if it can be extended
    to support the resource idea used in the python regression
    tests. That is the user has a --resource option to give a
    list of available resources and a test only runs if its
    requested resources are available. Otherwise it will be skipped.
    Example:
    TestDemo.py --resource=network --resource=audio
    ... would supply the network and audio resource.
    Or does it make more sense to put this in a separate patch?

    @rhettinger
    Copy link
    Contributor

    Logged In: YES
    user_id=80475

    The skipIf() method is sufficient. From there, it is
    trivial to roll your own resource check.

    @ncoghlan
    Copy link
    Contributor

    Logged In: YES
    user_id=1038590

    I'd certainly find such a feature handy - when testing
    different variations of an embedded firmware image, it would
    make it much easier to enable/disable different tests based
    on the capabilities of the firmware.

    Ditto for the original example of cross-platform testing.

    @rhettinger
    Copy link
    Contributor

    Logged In: YES
    user_id=80475

    After more thought, I think decorators offer a cleaner, more
    complete solution without further complicating the unittest
    module.

    def rootonly(f):
        "Decorator to skip tests that require root access"
        if os.geteuid() == 0:
            return f
        return lambda self: 0
    
    
    @rootonly
    def testReadingAsRoot(self):
       . . .

    Note the rootonly() decorator need only be defined once
    instead of writing a full self.skipIf(condition) inside
    every test. Also, it appears prior to the definition rather
    than inside. The approach is more flexible than the
    original proposal though it does lack a reporting mechanism.

    @rblank
    Copy link
    Mannequin Author

    rblank mannequin commented Sep 30, 2004

    Logged In: YES
    user_id=568100

    I strongly disagree. Skipped tests should not just be
    transformed into passed tests, but must be recorded as
    skipped and reported to the user. Knowing that a test
    skipped is important information.

    The Python regression tests (although I'm not familiar with
    them) provide the same "skip" functionality, and I don't
    think people would be happy to replace it with just "pass".

    The decorator approach is an interesting idea, though, and
    could be combined with skipIf() so as to provide the other
    advantages you mention, namely single definition and
    appearance prior to definition. Something along the following:

    def rootOnly(f):
            """Decorator to skip tests that require root access"""
            def wrapper(self):
                    self.skipIf(os.getuid() != 0, "Must be root")
                    self.f()
            wrapper.__doc__ = f.__doc__
            return wrapper
    
    
    class ReadShadowTest(unittest.TestCase):
            """Read access to /etc/shadow"""
            @rootOnly
            def testReadingAsRoot(self):
                    """Reading /etc/shadow as root"""
                    open("/etc/shadow").close()

    Note that I'm not yet familiar with decorators, so the
    wrapper() function might not be the correct way to do this.

    @purcell
    Copy link
    Mannequin

    purcell mannequin commented Sep 30, 2004

    Logged In: YES
    user_id=21477

    I've been really tied up; sorry for the delayed response, but I've been
    reading all the comments on this patch.

    Overall, I'm leaning in favour of accepting this patch, probably with
    some minor changes to the way skipped tests are reported.

    The concept of skipping is one that has been kept out of JUnit, but is
    found in NUnit and is well regarded there. In my XP coaching work
    ThoughtWorks I see an obscenely large number of JUnit tests, and a
    common mistake is to comment out test method bodies, leading to
    "false passes". Explicit support for skipping in unittest would mitigate
    this.

    I agree with Remy that the decorator example, though ingenious, has
    the wrong result; skipped tests will be reported as successes. In order
    for a test method to decide if it should be skipped, it will often need
    information from 'self' that was gathered during setUp() -- this makes
    decorators cumbersome for this. Also, a decorator solution would not
    allow test methods to skip if the setUp() method itself decides to skip().

    Please give me a few more days on this, and I'll work on integrating
    and tweaking the patch.

    @rblank
    Copy link
    Mannequin Author

    rblank mannequin commented Sep 30, 2004

    Logged In: YES
    user_id=568100

    Speaking of decorators, the NUnit example is quite
    instructive, re. their use of attributes to mark classes as
    test cases, methods as test methods, grouping tests by
    category, and for that matter ignoring tests temporarily. I
    expect all of this can be done with decorators: @Testmethod
    to mark individual tests, @category("LongRunning"), @ignore,
    @explicit, ...

    And if I'm not mistaken all of this can be added without
    breaking backward compatibility.

    Interesting times lay ahead!

    @purcell
    Copy link
    Mannequin

    purcell mannequin commented Sep 30, 2004

    Logged In: YES
    user_id=21477

    Yes, that's right, and I would consider providing a number of such
    decorators at a later date. I've just spent a little time chatting to my
    colleage Joe Walnes (of nMock fame) about all this; he's more of an
    nUnit authority than I am.

    Categories are particularly interesting. In theory, it would be possible
    to get the same effect using TestSuites, but in practice tool support
    (including unittest.main()) discourages the use of TestSuites in favour
    of magic discovery of test cases; categories would be a better way of
    allowing tools to dynamically construct suites. @ignore could be
    considered equivalent to @category("ignored") in a certain sense.

    Skipping is not quite the same as ignoring, since it's determined at
    run-time, and so I think it is appropriate to add methods to explicitly
    support it.

    Interesting times indeed.

    @rblank
    Copy link
    Mannequin Author

    rblank mannequin commented Oct 4, 2004

    Logged In: YES
    user_id=568100

    I have just run the unit tests from Zope 2.7.2 (2358 tests) on
    Python 2.3.3, first with the original unittest.py, then with the
    patch. There was no difference in output, except for the total
    duration.

    That may give a hint about backward compatibility.

    @rblank
    Copy link
    Mannequin Author

    rblank mannequin commented Nov 24, 2004

    Logged In: YES
    user_id=568100

    Anything new about skipping tests? I'm still very interested.

    @pitrou
    Copy link
    Member

    pitrou commented Jan 20, 2008

    So what's the status of this? Skipping test is still an important
    ability to have.

    @purcell
    Copy link
    Mannequin

    purcell mannequin commented Jan 22, 2008

    The status of this ticket is unchanged. I'm somewhat removed from the
    Python scene in recent times, and I'm not in a position to apply this
    patch or a variation of it.

    I still believe this would be a beneficial change to the unittest
    module, though, and perhaps someone else would be willing to apply
    Remy's patch (updated if necessary to apply cleanly). There's a minor
    typo in there ("tuble" instead of "tuple"), but otherwise it looks
    acceptable to me.

    @birkenfeld
    Copy link
    Member

    I'll take it.

    @birkenfeld birkenfeld assigned birkenfeld and unassigned purcell Jan 22, 2008
    @pupeno
    Copy link
    Mannequin

    pupeno mannequin commented Aug 11, 2008

    Hello,

    The attached patch adds the skip functionality and tests to a very
    recent (yesterday) Python 3. It took me a few hours to apply the patch,
    change to Python 3 style, merge the tests into the current set of tests
    not doing a mess.

    I think the whole thing is missing documentation, which I would gladly
    write if the code is good to go. Just let me know.

    Thanks.

    @pupeno
    Copy link
    Mannequin

    pupeno mannequin commented Aug 11, 2008

    Oh, I forgot to upgrade versions to include Python 3.0 and to mention
    that I have to fix some other tests to work with the new TestCase. It's
    nothing serious, just doctests that where too dependent on the output of
    the TextTestRunner. As previously discussed, doing that is not a good
    idea anyhow, but it is needed for these doctests and I wouldn't expect
    anybody else out there in the while to encounter this problem.

    @pitrou
    Copy link
    Member

    pitrou commented Dec 28, 2008

    Pupeno's patch looks good to me. Additional candy would be a decorator
    to flag skipped tests (e.g. @skipped_test or @skipped_test("A
    message")), but we can do that later.

    @pitrou pitrou added the type-feature A feature request or enhancement label Dec 28, 2008
    @pitrou pitrou added the type-feature A feature request or enhancement label Dec 28, 2008
    @rblank
    Copy link
    Mannequin Author

    rblank mannequin commented Dec 28, 2008

    There's still a typo in the docstring of TestResult.addSkipped() (tuble
    -> tuple).

    @benjaminp
    Copy link
    Contributor

    I think this is a good improvement, and I hope it can make it into 2.7/3.1.

    Several comments on patch:

    • I don't like the name "SkipException" SkipTest is better IMO.

    • TestResult.addSkipped should be changed to TestResult.addSkip.

    • I'm not sure why TestResult.addSkipped gets sys.exc_info() pass in. I
      think you should just catch the exception and pass the reason ("str(e)")
      to addSkipped.

    • The patch needs docs before it can be applied.

    • As Antoine said, it would be nice to have decorators for skipping.
      When I implemented this, I added the skip() (unconditional skip)
      skip_if(condition, reason) and skip_unless(condition, reason)
      decorators. It should also be easy to extend the mechanism, so that
      custom decorators can be written.

    • It would nice to be able to skip whole classes, too. This could easily
      be done with class decorators.

    (Georg, I hope you don't mind if I "steal" this from you.)

    @benjaminp benjaminp assigned benjaminp and unassigned birkenfeld Dec 29, 2008
    @benjaminp
    Copy link
    Contributor

    Ok, here's my unittest skipping patch. It supports skipping classes and
    expected failures and includes skipping decorators.

    I had to employ a little evil to make test skipping work for classes.
    The made a new TestSuite class called ClassTestSuite, to contain the
    tests for one class. It pretends to be a TestCase enough that in can be
    completely skipped.

    @benjaminp
    Copy link
    Contributor

    Here's the patch on Rietveld: http://codereview.appspot.com/27095

    @benjaminp
    Copy link
    Contributor

    I've attached a new patch which takes into account Antoine's review.

    @benjaminp
    Copy link
    Contributor

    Committed my patch in r70555.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 9, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants