This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: Fix test discovery for test_tarfile.py
Type: behavior Stage: resolved
Components: Tests Versions: Python 3.3, Python 3.4
process
Status: closed Resolution: duplicate
Dependencies: Superseder:
Assigned To: Nosy List: brett.cannon, ezio.melotti, pitrou, zach.ware
Priority: normal Keywords: patch

Created on 2013-04-10 18:40 by zach.ware, last changed 2022-04-11 14:57 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
test_tarfile_discovery.diff zach.ware, 2013-04-10 18:40 test_tarfile.py fix, version 1 review
test_tarfile_discovery.v2.diff zach.ware, 2013-04-11 18:54 Version 2 review
test_tarfile_discovery.v3.diff zach.ware, 2013-04-16 16:19 Version 3 review
Messages (6)
msg186525 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2013-04-10 18:40
Here's a patch for test_tarfile.py discovery.

There are a couple of inheritance issues resolved; CommonReadTest (and its children, MiscReadTest and StreamReadTest) are changed simply to avoid running the tests in CommonReadTest on their own--they pass, but are unnecessary.  LongnameTest and WriteTestBase cause failures when run on their own, so they no longer inherit from unittest.TestCase.

test_main() has been replaced by setUpModule, some skip decorators, and tearDownModule.  HardlinkTest and LinkEmulationTest are given skipUnless and skipIf decorators, respectively, instead of simply being appended to 'tests' or not.  This does change the test count; on my machine it adds the 4 tests skipped in LinkEmulationTest, raising the total from 307 to 311.  gzip, bz2, and lzma setup has been moved into setUpModule, and each group of those tests now has a new dummy base class with a skip decorator.  This will also change test counts on machines that don't have gzip, bz2, or lzma available.  tearDownModule replaces the finally clause at the end of test_main().

A simpler patch would just convert test_main into load_tests and add a tearDownModule function to replace the finally clause of test_main(), but I believe this slightly more invasive approach is more correct and should simplify future maintenance of the module.

Thanks,

Zach
msg186798 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-04-13 18:55
Wouldn't it be simpler to write (untested):

def skipUnlessCompressionModule(name):
    """
    Skip if the specified compression module is not available.  Must e a
    string, currently any of 'gzip', 'bz2', or 'lzma'.
    """
    return unittest.skipUnless(globals()[name],
                               '{} not available'.format(name))

?
msg187008 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2013-04-15 17:03
That is indeed simpler than what I wrote, and it does work as expected.  But, is it preferable to do it this way, or with Ezio's suggested method (``skip_unless_gzip = unittest.skipUnless(gzip, "gzip not available")``, and for bz2 and lzma)?  I can see merits to both; mine only requires a docstring change if there's a new compression module sometime in the future, but Ezio's is simpler and shorter.

Here's another thought; would it be more useful to have a general version of this skip decorator in test.support, something along the lines of:

"""
def skipWithoutModule(name):
    """
    Skip if the named module is not available.
    """
    try:
        module = importlib.import_module(name)
    except ImportError:
        module = None

    return unittest.skipUnless(module, 
                               "{} module not available".format(name))
"""

And, actually, looking through test.support to see if there was anything like this already, I found requires_bz2 and requires_lzma that already exist; should I just add a requires_gzip to test.support and use those three in test_tarfile?
msg187082 - (view) Author: Ezio Melotti (ezio.melotti) * (Python committer) Date: 2013-04-16 14:15
> I found requires_bz2 and requires_lzma that already exist;

There's also requires_zlib.  import_module is also somewhat similar, but it's used to skip the whole test file when the module is missing.

> Here's another thought; would it be more useful to have a general
> version of this skip decorator in test.support

We were discussing this a couple of days ago on #python-dev.  Adding a skip_unless_module('modulename') (or requires_module('modulename')) and get rid of the several requires_* is certainly an option.

skip_unless_* (or requires_*) is shorter and more readable than skip_unless_module('modulename'), so this would be my preferred choice if it's defined and used within a single test module.  However if we add something to test.support, I'd rather have a more generic skip_unless_module('modulename') and get rid of the requires_*.
It's should also be possible to define specific ``skip_unless_x = skip_unless_module('x')`` in the test modules if necessary.

> should I just add a requires_gzip to test.support and use those three
> in test_tarfile?

I think that's reasonable for now -- we can always refactor it later and replace them with a skip_unless_module()
msg187101 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2013-04-16 16:19
>> should I just add a requires_gzip to test.support and use those three
>> in test_tarfile?

> I think that's reasonable for now -- we can always refactor it later 
> and replace them with a skip_unless_module()

Here's a patch :)
msg191232 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2013-06-15 19:47
Superseded by issue18223.
History
Date User Action Args
2022-04-11 14:57:44adminsetgithub: 61889
2013-06-15 19:51:21ezio.melottisetstage: resolved
2013-06-15 19:47:34zach.waresetstatus: open -> closed
resolution: duplicate
messages: + msg191232
2013-04-16 16:19:04zach.waresetfiles: + test_tarfile_discovery.v3.diff

messages: + msg187101
2013-04-16 14:15:36ezio.melottisetmessages: + msg187082
2013-04-15 17:03:13zach.waresetmessages: + msg187008
2013-04-13 18:55:10pitrousetnosy: + pitrou
messages: + msg186798
2013-04-11 18:54:41zach.waresetfiles: + test_tarfile_discovery.v2.diff
2013-04-10 18:40:49zach.warecreate