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

Make CPython test package discoverable #60952

Closed
zware opened this issue Dec 22, 2012 · 51 comments
Closed

Make CPython test package discoverable #60952

zware opened this issue Dec 22, 2012 · 51 comments
Assignees
Labels
tests Tests in the Lib/test dir type-bug An unexpected behavior, bug, or error

Comments

@zware
Copy link
Member

zware commented Dec 22, 2012

BPO 16748
Nosy @terryjreedy, @ezio-melotti, @merwok, @bitdancer, @cjerdonek, @ericsnowcurrently, @zware, @serhiy-storchaka
Dependencies
  • bpo-14408: Support ./python -m unittest in test_socket
  • bpo-16852: Fix test discovery for test_genericpath.py
  • bpo-16888: Fix test discovery for test_array.py
  • bpo-16968: Fix test discovery for test_concurrent_futures.py
  • bpo-18258: Fix test discovery for test_codecmaps*.py
  • bpo-20008: Clean up/refactor/make discoverable test_decimal
  • bpo-22002: Make full use of test discovery in test subpackages
  • Files
  • issue16748.diff
  • issue16748_genericpath.diff: For test_*path.py
  • issue16748_genericpath.v2.diff: test_*path.py version 2
  • checkTestOverriding.diff: Hacking patch which prints test methods overriding
  • test_overriding.txt: List of overridden test methods
  • 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/ezio-melotti'
    closed_at = None
    created_at = <Date 2012-12-22.01:02:27.055>
    labels = ['type-bug', 'tests']
    title = 'Make CPython test package discoverable'
    updated_at = <Date 2020-01-29.00:34:50.166>
    user = 'https://github.com/zware'

    bugs.python.org fields:

    activity = <Date 2020-01-29.00:34:50.166>
    actor = 'brett.cannon'
    assignee = 'ezio.melotti'
    closed = False
    closed_date = None
    closer = None
    components = ['Tests']
    creation = <Date 2012-12-22.01:02:27.055>
    creator = 'zach.ware'
    dependencies = ['14408', '16852', '16888', '16968', '18258', '20008', '22002']
    files = ['28523', '28535', '28542', '28689', '28690']
    hgrepos = []
    issue_num = 16748
    keywords = ['patch']
    message_count = 49.0
    messages = ['177913', '177925', '177927', '178091', '178140', '178156', '178420', '178425', '178428', '178737', '178749', '178795', '178812', '178831', '178835', '178839', '178840', '178841', '178861', '178932', '178947', '178956', '178958', '178965', '179055', '179286', '179288', '179292', '179296', '179297', '179298', '179305', '179308', '179316', '179319', '179347', '179349', '179361', '179370', '179634', '179635', '179663', '179665', '179956', '179971', '179980', '179996', '189292', '191487']
    nosy_count = 9.0
    nosy_names = ['terry.reedy', 'ezio.melotti', 'eric.araujo', 'r.david.murray', 'chris.jerdonek', 'python-dev', 'eric.snow', 'zach.ware', 'serhiy.storchaka']
    pr_nums = []
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue16748'
    versions = ['Python 3.4', 'Python 3.5']

    @zware
    Copy link
    Member Author

    zware commented Dec 22, 2012

    As pointed out by Éric Araujo in msg177908 of bpo-16694, tests using the idiom presented in PEP-399 are subject to breakage of test discovery. This issue's goal is to root out and fix all usages of this idiom.

    So far, only test_heapq is known to be affected.

    As for fixing the issue, each module get its own mixin class, or should a simple class such as:

    class TestPyAndCMixin:
        module = None

    be added to test.support?

    @zware zware added tests Tests in the Lib/test dir type-bug An unexpected behavior, bug, or error labels Dec 22, 2012
    @serhiy-storchaka
    Copy link
    Member

    No need to add this trivial class to test.support. Mixin can have different parameters in additional to 'module' (see 16659).

    I found a lot of usages of this idiom in tests and some of them can't be changed so simple (for example test_genericpath and test_functools).

    @serhiy-storchaka
    Copy link
    Member

    Éric, can you submit a test which exposes the problem?

    @merwok
    Copy link
    Member

    merwok commented Dec 24, 2012

    I won’t be online for the next two weeks.

    I think there is a bug with Michael Foord, RDM and I where we talk about this very issue.

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Dec 25, 2012

    Perhaps I misunderstood something, but test_decimal.py *is* using the
    exact idiom from PEP-399 and it works. Why do you want to "fix" the
    usage of this idiom?

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Dec 25, 2012

    I finally understood the issue. So this does not work:

    ./python -m unittest discover [Lib/test/](https://github.com/python/cpython/blob/main/Lib/test) 'test_dec*.py'
    

    Neither does this:

    ./python -m unittest discover [Lib/test/](https://github.com/python/cpython/blob/main/Lib/test) 'test_multipro*.py'
    

    And this fails, too (still hanging):

    ./python -m unittest discover [Lib/test/](https://github.com/python/cpython/blob/main/Lib/test) 'test_thread*.py'
    

    I'm not sure why tests in the Python test suite should be discoverable.
    If I read this ...

    http://www.voidspace.org.uk/python/weblog/arch_d7_2009_05_30.shtml
    

    ..., the feature is for projects that don't have a test collection machinery.

    @brettcannon
    Copy link
    Member

    Making tests discoverable allows for the future possibility of dropping our custom test runner code and using more of unittest code by having regrtest use unittest's discovery code to find all tests.

    @terryjreedy
    Copy link
    Member

    I should think that the first fix should be to the PEP. If I understand msg177908, that would mean removing unittest.TestCase as a base for ExampleTest and adding it as bases for AcceleratedExampleTest and PyExampleTest. Or have I missed something?

    @bitdancer
    Copy link
    Member

    That sounds right to me.

    Note that PEP-399 is following older conventions that were laid down in a time when unittest did not *have* test discovery, so this is a new paradigm we'd like to move to (for the reasons Brett mentioned), and it may take a while to get there. It applies to more than just the python/C accelerator distinction; it applies any time a base class plus specialized classes are used to construct test cases. (I do this a bunch in the email tests, for example, and that has no accelerator.)

    @ezio-melotti
    Copy link
    Member

    I just came across the problem described here while reviewing bpo-16694.

    The idiom I used for the JSON tests[0] (and possibly a couple of other tests) when I rewrote them was to have something like:

    class FooTest:
        # all the test methods here
    
    class CFooTest(FooTest, unittest.TestCase):
        module = c_foo
    
    class PyFooTest(FooTest, unittest.TestCase):
        module = py_foo

    The reason to only have the subclasses inheriting from unittest.TestCase was exactly to avoid having FooTest as a discoverable TestCase.

    I think PEP-399 should be updated and the tests should be fixed.
    I don't think it's necessary to provide a base class as suggested in the first message (it's actually not even necessary to set the module to None, because eventually it will be set to either some cmodule or pymodule. In case something goes wrong it's even better if the AttributeError says "self has no attribute 'module'" rather than "NoneType has no attribute 'somemeth'".)

    @brett, should I open a separate issue for the PEP-399 changes?

    [0] see e.g.: Lib/test/json_tests/test_float.py and Lib/test/json_tests/init.py (note that here the idiom is a bit more complicated, because in addition to set the self.module, I also had to set additional attributes and work with different test files in the same package. I also added additional tests in __init__ to make sure that import_fresh_module worked after adapting it to work with packages.)

    @ezio-melotti ezio-melotti self-assigned this Jan 1, 2013
    @brettcannon
    Copy link
    Member

    I created http://bugs.python.org/issue16835 to remind me to update PEP-399.

    @ezio-melotti
    Copy link
    Member

    Attached patch fixes Lib/test/test_heapq.py.
    I also replaced the test_main() with unittest.main() and got rid of the code previously used to "test" reference leaks. As it is the patch can be applied on 3.3/default. If you think it should be backported to 2.7/3.2 too, the changes on test_main() should be reverted.

    @brettcannon
    Copy link
    Member

    Why remove the refleak tests? I'm sure Raymond put those in for a reason as I know he tries to reuse various objects a lot and this helped make sure he didn't mess anything up.

    I don't think the tests need to be backported.

    @ezio-melotti
    Copy link
    Member

    Why remove the refleak tests?

    Because regrtest already provides the feature, so I don't see a reason to duplicate it here. FWIW the same code is copy/pasted elsewhere too, e.g. in Lib/test/test_operator.py:438.

    @brettcannon
    Copy link
    Member

    The patch LGTM

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 2, 2013

    New changeset 008bac4e181c by Ezio Melotti in branch '3.3':
    bpo-16748: test_heapq now works with unittest test discovery.
    http://hg.python.org/cpython/rev/008bac4e181c

    New changeset de6bac0a40cc by Ezio Melotti in branch 'default':
    bpo-16748: merge with 3.3.
    http://hg.python.org/cpython/rev/de6bac0a40cc

    @ezio-melotti
    Copy link
    Member

    Fixed, thanks for the review!

    @serhiy-storchaka
    Copy link
    Member

    As I mentioned above, not only test_heapq uses this idiom, but a lot of other tests. Try to fix hard cases first: test_genericpath and test_functools.

    @zware
    Copy link
    Member Author

    zware commented Jan 2, 2013

    Here's a patch that should clear up test_genericpath.py (and other path tests).

    @zware zware changed the title Ensure test discovery doesn't break for modules testing C and Python implementations Make CPython test package discoverable Jan 2, 2013
    @serhiy-storchaka
    Copy link
    Member

    See also test_functools, test_xml_etree, test_bisect, test_bz2, test_warnings, test_decimal, test_datetime, json_tests, test_io, test_concurrent_futures, and many, many other undiscoverable tests.

    @zware
    Copy link
    Member Author

    zware commented Jan 3, 2013

    Here's version 2 of the genericpath patch.

    Should I try to fix everything in one patch, or one patch per test module (or group of test modules like test_(generic|mac|nt|posix)path.py)? And if separate, should each one get its own issue, or just keep them all here?

    @bitdancer
    Copy link
    Member

    I would suggest one patch and issue per test module. If multiple test modules are related enough, they could go in one patch/issue; that's a judgement call. We can make this issue dependent on those individual issues.

    @zware
    Copy link
    Member Author

    zware commented Jan 3, 2013

    Sounds good to me. Shall I move the genericpath fix to a new issue, or leave that one here and begin starting new issues with the next one tackled?

    Any volunteers for being nosied on new issues to make the dependency link back to this one for me? :)

    @brettcannon
    Copy link
    Member

    You can go ahead and start a new issue so it isn't forgotten about as this becomes a meta issue.

    And you can always add me to the nosy, just can't guarantee how fast I will do the reviews. =)

    @ezio-melotti
    Copy link
    Member

    Feel free to add me to the nosy.

    @zware
    Copy link
    Member Author

    zware commented Jan 7, 2013

    I've come up with a semi-comprehensive list of the tests that cause ugly failures with test discovery. Tests were run on 3.4 debug, on Windows 7 32bit without several of the 'external' projects built, using the command PCbuild\python_d.exe -m unittest discover [Lib/test/](https://github.com/python/cpython/blob/main/Lib/test) "test_x*.py" where 'x' was whatever it took to get a specific test or group of tests to run.

    There are two basic causes of failure, and a couple of oddballs.

    First up, tests that fail with discovery, but run fine with PCbuild\python_d.exe -m test test_name:

    test_array test_asyncore test_bisect test_bufio test_bytes test_codecs test_concurrent_futures test_configparser test_ctypes test_dbm test_decimal test_file test_format test_ftplib test_future3 test_hash test_imaplib test_import test_index test_io test_iterlen test_locale test_multiprocessing test_module test_osx_env test_pickle test_random test_robotparser test_runpy test_set test_shelve test_socket test_sqlite test_sys test_tarfile test_time test_univnewlines test_warnings test_xml_etree

    These 39 should be relatively straightforward fixes like heapq and genericpath have been.

    The next group are tests that are properly skipped by the test package, but fail noisily with unittest discovery:

    test_codecmaps_cn test_codecmaps_hk test_codecmaps_jp test_codecmaps_kr test_codecmaps_tw test_crypt test_curses test_epoll test_fcntl test_gdb test_grp test_hashlib test_ioctl test_kqueue test_largefile test_nis test_openpty test_ossaudiodev test_pipes test_poll test_posix test_pty test_pwd test_readline test_resource test_smtpnet test_socketserver test_ssl test_syslog test_tcl test_threadsignals test_tk test_ttk_guionly test_ttk_textonly test_urllib2net test_wait3 test_wait4 test_winsound test_zipfile64

    Some of these are skipped due to certain resources not being allowed, some don't have the proper module available. In any case, discovery does not respond well to the skip methods used for each of them.

    The 'oddballs' I mentioned before are as follows:

    json_tests.test_fail leakers.test_gestalt test_glob test_json test_shutil test_urllib2_localnet

    Each of these fail on my machine whichever way I run them. I'm not sure if any of them actually have any issues with discovery, I'm merely listing them here for completeness' sake. I will try to test them again myself on Linux later, and report back what I find.

    If anyone else feels led to tackle any of these, please add me to the nosy list of any new issues filed. I plan to eventually work through all of these myself, but don't want to duplicate effort :)

    Thanks,

    Zach Ware

    @bitdancer
    Copy link
    Member

    Great list, thanks.

    The ones that fail to be run/skipped when run under discovery can probably be fixed by moving them to the more modern unittest 'skip' functions instead of depending on being run under regrtest and using the test.support resource functions. When run directly they should not skip due to a resource not being set, but when run under regrtest (I'm not sure how you detect that but if there isn't currently a way we should make one via test.support) they should respect the resources.

    Unittest doesn't yet have a concept of resources, but I believe there is an open issue for it, so at some point we will hopefully be able to move all resource management to unittest and out of regrtest. But not yet.

    @ezio-melotti
    Copy link
    Member

    While we are at it, should we also move these tests to use unittest.main() instead of test_main() and similar?

    @terryjreedy
    Copy link
    Member

    test_main makes it trivial to import a test file and run the test.
    >>> import test/test_xxx as t; t.test_main()
    I do not know the implications of unittest.main(), but I would dislike losing the above ability.

    @bitdancer
    Copy link
    Member

    As we discussed in another issue, you just need to change your pattern to:

    >> import unittest.main as runtest
    >> import test.test_xxx as t
    >> runtest(t)

    Which granted is more typing if you are running just one test, but not much more if you are running more than one. You could also put the import for unittest into your rc file.

    @ezio-melotti
    Copy link
    Member

    Without test_main you would have to do unittest.main(t, exit=False). I'm not sure there's an easier way.

    @cjerdonek
    Copy link
    Member

    >>> import test.test_xxx as t; t.test_main()

    As long as the module has "import unittest", you could also do the following, which has just 5 more characters :)

    >> import test.test_xxx as t; t.unittest.main(t)

    @cjerdonek
    Copy link
    Member

    One more. Since unittest imports strings, you can also do:

    >> import unittest as u; u.main("test.test_xxx")

    @ezio-melotti
    Copy link
    Member

    I think that's the last nail in the coffin of test_main.
    Terry, do you agree?

    @serhiy-storchaka
    Copy link
    Member

    There are tests outside of Lib/test/ hierarchy.

    @bitdancer
    Copy link
    Member

    Yes, but not many, and not as many as there used to be. I'd like to see them all moved, but met resistance on that front. It may be that those just can't be run using unittest discovery, and perhaps that will eventually convince the maintainers to move them. But probably not until a number of years from now :)

    @bitdancer
    Copy link
    Member

    Also, it may be possible to add unittest discovery hooks to the stubs that *are* in Lib/test.

    @brettcannon
    Copy link
    Member

    It's totally doable to set up stubs in Lib/test to use test discovery on those tests which exist outside of that directory. test_importlib actually has some code for that left over from when it lived in Lib/importlib/test: http://hg.python.org/cpython/file/4617b7ac302a/Lib/test/test_importlib/__init__.py

    @cjerdonek
    Copy link
    Member

    Also, it may be possible to add unittest discovery hooks to the stubs that *are* in Lib/test.

    The load_tests protocol (2.7, 3.2+) seems like the right approach for this:

    http://docs.python.org/dev/library/unittest.html#load-tests-protocol

    @ezio-melotti
    Copy link
    Member

    There are tests outside of Lib/test/ hierarchy.

    See bpo-10572.

    @cjerdonek
    Copy link
    Member

    There are lots of modules to change here. I wonder if some or most of this couldn't be automated.

    For example, is there any reason we couldn't write a script to check for the type of test duplication fixed documentation-wise in bpo-16835? We could use such a script to find the classes that need to be updated when removing test_main (or to double-check existing test modules).

    I don't know if there are ever times we want such duplication.

    @cjerdonek
    Copy link
    Member

    As suggested in the previous comment, here is simple code to find candidates for test duplication (TestCase subclasses subclassing other TestCase classes):

    def find_dupes(mod):
        objects = [getattr(mod, name) for name in sorted(dir(mod))]
        classes = [obj for obj in objects if isinstance(obj, type) and
                   issubclass(obj, unittest.TestCase)]
        for c in classes:
            for d in classes:
                if c is not d and issubclass(d, c):
                    print("%s: %s < %s" % (mod.__name__, c.__name__, d.__name__))

    Out of curiosity, I ran a modified form of this against all test modules to find which ones fit this pattern and *already* rely on unittest discovery (i.e. don't have test_main()). We might want to adjust these as well. There were four: test_asyncore, test_configparser, test_heapq, test_ipaddress

    @serhiy-storchaka
    Copy link
    Member

    Here is a dirty patch which hacks unittest to search possible test overriding. Just apply the patch and run regression tests.

    @zware
    Copy link
    Member Author

    zware commented Jan 14, 2013

    Chris:

    There are lots of modules to change here. I wonder if some or most of
    this couldn't be automated.

    Possibly, but I don't mind going through individually if Ezio (or others) don't mind committing individually. From what I've seen, the test suite is varied enough within itself that it would be pretty hard to properly automate the changes I've been making. Also, I'm of the opinion that we'll end up with a higher quality result doing things by hand anyway.

    @cjerdonek
    Copy link
    Member

    Also, I'm of the opinion that we'll end up with a higher quality result doing things by hand anyway.

    Automation doesn't (and shouldn't) preclude careful review and modifications by hand. My point was that it seems like it might be able to get you, say, 80% of the way there (e.g. by deleting test_main(), determining which classes are called by test_main(), adding ", unittest.TestCase", creating stub classes that could be retained or deleted). Or at least provide some additional checks or info. There are nearly 400 test files using test_main(), no?

    @zware
    Copy link
    Member Author

    zware commented Jan 14, 2013

    For the conversion from test_main() to unittest.main(), I could certainly see automation helping a lot; most cases are very simple. But really, that issue is somewhat tangential to this one and in my last message I was thinking from the perspective of just the 80 or so modules that completely fail test discovery otherwise. I believe that trying to automate those would be more trouble than it's worth. Especially if bpo-16935 is fixed, as that will knock out most of the tests I listed above that failed to skip properly.

    @cjerdonek
    Copy link
    Member

    Okay, well the title of this issue is "Make CPython test package discoverable," so as part of that we should be confirming that test discovery finds the same tests that test_main() runs (and if not, to understand why and/or make them the same).

    @terryjreedy
    Copy link
    Member

    Chris: >>> import unittest as u; u.main("test.test_xxx")
    Ezio: I think that's the last nail in the coffin of test_main.
    Terry, do you agree?

    Belated answer: now that I use and understand the idiom, yes.

    Chris: The load_tests protocol (2.7, 3.2+) seems like the right approach for [tests in directories outside of /test].

    I am using that in the new idlelib/idle_test/init.py. But perhaps the load_tests() example in the doc, which I copied, should be changed from 'do nothing' to 'discover all tests in the directory' as a more useful default.

    @zware
    Copy link
    Member Author

    zware commented Jun 19, 2013

    So many things have been fixed since I last made a list of failing modules that it seemed like time to make a new one. First, failing modules that already have open issues with patches:

    Other tests that fail, that may be the fault of my test machine rather than the tests themselves (or, ones that I'd like others to try before I call them broken):

    • test_urllib2net (fails on my machine on normal runs due to network setup)
    • test_shutil (fails on my machine on normal runs for unknown reasons relating to symlinks)

    The below tests fail for reasons that I expect to probably be related to test run order, as all of them pass when narrowing the pattern to match only them (e.g., "test_dbm*.py" rather than "test_*.py"):

    • test_dbm (had a permission error on a test file)
    • test_venv (tried to delete a temp dir that was unexpectedly non-empty)
    • test_wsgiref (failure in testEnviron)
    • test_logging (failure in test_warnings)
    • test_inspect (had some issue with locating its test files)

    This leaves only 6 other tests that fail due to improper inheritance, setup done in test_main rather than setUp/setUpClass/setUpModule, or other reasons directly related to discovery:

    • test_decimal
    • test_largefile
    • test_pickle (pickletester)
    • test_runpy
    • test_shelve
    • test_xml_etree

    Other tests that need some manner of attention related to discovery:

    • test_json: it half-uses discovery, it could use it completely
    • test_importlib: about the same as test_json
    • leakers package: either rename to non-discoverable names or otherwise make leakers.test_gestalt (in particular) not report an error when discovered.

    This may not be a comprehensive list, but it does cover all of the most obvious failures that remain.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @serhiy-storchaka
    Copy link
    Member

    Can this issue be closed? I finished conversion from test_main() to unittest.main() in #89392, but I did test all tests with unittest discover.

    @zware
    Copy link
    Member Author

    zware commented Jan 16, 2024

    Sounds good to me. Thanks for finishing it off, Serhiy!

    @zware zware closed this as completed Jan 16, 2024
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    tests Tests in the Lib/test dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    8 participants