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 discovery doesn't detect namespace packages when given no parameters #68070

Closed
FlorianApolloner mannequin opened this issue Apr 7, 2015 · 29 comments
Closed
Labels
3.11 only security fixes docs Documentation in the Doc dir type-bug An unexpected behavior, bug, or error

Comments

@FlorianApolloner
Copy link
Mannequin

FlorianApolloner mannequin commented Apr 7, 2015

BPO 23882
Nosy @warsaw, @terryjreedy, @ericvsmith, @rbtcollins, @ezio-melotti, @voidspace, @methane, @PCManticore, @ericsnowcurrently, @phmc, @ashkop, @maggyero, @miss-islington, @miss-islington, @miss-islington, @miss-islington, @miss-islington, @adamchainz
PRs
  • bpo-23882: Doc: Clarify unittest discovery document #21560
  • [3.9] bpo-23882: Doc: Clarify unittest discovery document (GH-21560) #24619
  • [3.9] bpo-23882: Doc: Clarify unittest discovery document (GH-21560) #24619
  • [3.9] bpo-23882: Doc: Clarify unittest discovery document (GH-21560) #24619
  • [3.9] bpo-23882: Doc: Clarify unittest discovery document (GH-21560) #24619
  • [3.8] bpo-23882: Doc: Clarify unittest discovery document (GH-21560) #24620
  • [3.7] bpo-23882: Doc: Clarify unittest discovery document (GH-21560) #24621
  • bpo-23882: unittest: Drop PEP 420 support from discovery. #29745
  • Files
  • console_log.txt
  • issue_23882_test_case.sh
  • issue23882_find_all.patch
  • issue23882_find_one_level.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 = None
    closed_at = <Date 2022-03-07.03:57:48.580>
    created_at = <Date 2015-04-07.11:36:35.978>
    labels = ['3.11', 'type-bug', 'invalid', 'docs']
    title = "unittest discovery doesn't detect namespace packages when given no parameters"
    updated_at = <Date 2022-03-07.03:57:48.580>
    user = 'https://bugs.python.org/FlorianApolloner'

    bugs.python.org fields:

    activity = <Date 2022-03-07.03:57:48.580>
    actor = 'methane'
    assignee = 'docs@python'
    closed = True
    closed_date = <Date 2022-03-07.03:57:48.580>
    closer = 'methane'
    components = ['Documentation']
    creation = <Date 2015-04-07.11:36:35.978>
    creator = 'Florian.Apolloner'
    dependencies = []
    files = ['38857', '39142', '39222', '39223']
    hgrepos = []
    issue_num = 23882
    keywords = ['patch']
    message_count = 29.0
    messages = ['240205', '240254', '240264', '241331', '241334', '241351', '241619', '241621', '241627', '242170', '242203', '242213', '242967', '248935', '248936', '373925', '373926', '373927', '373952', '373961', '387500', '387501', '387502', '387532', '387542', '387549', '397150', '406761', '410180']
    nosy_count = 20.0
    nosy_names = ['barry', 'terry.reedy', 'eric.smith', 'rbcollins', 'ezio.melotti', 'michael.foord', 'methane', 'rgammans', 'docs@python', 'Claudiu.Popa', 'Zbynek.Winkler', 'eric.snow', 'pconnell', 'Florian.Apolloner', 'ashkop', 'maggyero', 'miss-islington', 'miss-islington', 'miss-islington', 'miss-islington', 'miss-islington', 'adamchainz', 'rgammans@gammascience.co.uk', 'mdjonyhossainhabib']
    pr_nums = ['21560', '24619', '24619', '24619', '24619', '24620', '24621', '29745']
    priority = 'normal'
    resolution = 'not a bug'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue23882'
    versions = ['Python 3.11']

    @FlorianApolloner
    Copy link
    Mannequin Author

    FlorianApolloner mannequin commented Apr 7, 2015

    Unittest discovery does not seem to work if the tests package is a namespace package. The attached file should have all details to reproduce, as soon as I readd an __init__.py everything works fine.

    Test was done using python3.4.2 and 3.4.3

    @ezio-melotti ezio-melotti added the type-bug An unexpected behavior, bug, or error label Apr 7, 2015
    @ashkop
    Copy link
    Mannequin

    ashkop mannequin commented Apr 8, 2015

    Spent some time looking into this one. Looks like the problem is in TestLoader.discover() method. There are couple of issues I found in it, all caused by same assumption.

    Documentation [1] states that all test modules found by discover() method should be importable from top_level_dir. Whenever this method finds a subdirectory of start_dir it checks for __init__.py file. If there's no __init__.py then finder assumes that files within this package is not importable and stops recursion. This kind of 'importablity' check is not valid since we have namespace packages.

    I'm not sure what should be done to fix this issue. We can change documentation to state that only regular packages with tests will be discovered. Or we can fix 'importability' checks, which will mean that all tests in all subdirectories will be discovered.

    [1] https://docs.python.org/3.4/library/unittest.html#unittest.TestLoader.discover

    @ericsnowcurrently
    Copy link
    Member

    Is there any reason for unittest to not use pkgutil.iter_modules or pkgutil.walk_packages? Either should work.

    @PCManticore
    Copy link
    Mannequin

    PCManticore mannequin commented Apr 17, 2015

    Isn't this already supported by the patch added in bpo-17457?

    @ashkop
    Copy link
    Mannequin

    ashkop mannequin commented Apr 17, 2015

    Not fully. Patch for bpo-17457 fixed discovery when you explicitly specified namespace package as a target for discovery. I.e. when you run

    python -m unittest namespace_pkg

    But it didn't recurse into any namespace packages inside namespace_pkg. So when you run

    python -m unittest

    it doesn't go under all namespace packages (basically folders) in cwd.

    @rbtcollins
    Copy link
    Member

    Hi. I'd be happy enough to use pkgutil helpers if they (like walkdirs) allowed trimming the output: part of the definition of discovery is that one can control it, to stop it importing or processing part of the tree that one doesn't want processed.

    Alex: can you create a test case please - just a small script, python or shell or whatever, that will setup a demonstration of the bug? Thanks.

    @ashkop
    Copy link
    Mannequin

    ashkop mannequin commented Apr 20, 2015

    This script creates following directory structure

    issue_23882\
    tests\
    test_fail.py

    If you run from issue_23882 directory

    python -m unittest

    it doesn't find any tests. If there is __init__.py inside tests folder, same command finds test_fail.py and runs it.

    I'm not sure yet about using pkgutil cause looks like iter_modules and walk_packages do not find namespace packages as well.

    @rbtcollins
    Copy link
    Member

    Ok, so here's whats happening:
    the default behaviour is to do discovery of '.', which bypasses the namespace support code.

    Running with tests as the first parameter works because it doesn't require the directory being directly scanned to be a package.

    Equally, if the namespace isn't mapped to a local directory, 'python -m unittest tests' will load tests from the tests namespace.

    So - this seems to be acting as designed. Namespace packages working fine.

    However, I think what you're asking for is that namespace packages in your cwd are picked up and tested, without you having to supply the namespace name?

    I think what we'd need for that to work sanely is a patch to teach loader.discover to determine the local namespaces which are present under start_dir. Right now this code:
    elif os.path.isdir(full_path):
    if (not namespace and
    not os.path.isfile(os.path.join(full_path, '__init__.py'))):
    return None, False

    gets in your way: we assume that either we're processing a namespace, or there are no namespaces at all.

    I think some more detangling of the generator could address this fairly cleanly.

    @rbtcollins rbtcollins changed the title unittest discovery and namespaced packages unittest discovery doesn't detect namespace packages when given no parameters Apr 20, 2015
    @ashkop
    Copy link
    Mannequin

    ashkop mannequin commented Apr 20, 2015

    Thanks. I understand the code pretty well and I saw bpo-17457 that fixes discovery for explicitly specified namespace package.

    What I need is to know how discovery has to work. Do we need to discover namespace packages inside discovery path? And should we do that recursively? I.e. should we pick namespace packages inside namespace packages? This will lead to recursive scan of all directories under discovery path.

    @ashkop
    Copy link
    Mannequin

    ashkop mannequin commented Apr 28, 2015

    I'm still not sure which solution is the best. So I attach two simple patches.

    First one enables full recursive scan of start_dir for namespace packages.

    Second one loads tests only from top level namespace packages.

    @rbtcollins
    Copy link
    Member

    Ah the user model?

    I think the following:

    If I run 'python -m unittest' in a directory, then I expect to run all of the tests contained within that directory tree, and no others.

    Does that definition help?

    @ashkop
    Copy link
    Mannequin

    ashkop mannequin commented Apr 29, 2015

    Yes. That is how issue23882_find_all.patch works. I just removed hte condition

        if (not namespace and
            not os.path.isfile(os.path.join(full_path, '\_\_init__.py'))):
            return None, False
    

    This makes namespace parameter redundant. Can I remove it?

    @ashkop
    Copy link
    Mannequin

    ashkop mannequin commented May 12, 2015

    Please, review the patch.

    @rbtcollins
    Copy link
    Member

    reviewed in rietvald, but here too just in case.

    The hunk that saves/restores _top_level_dir feels wrong to me - and not part of this bug, please remove it.

    The rest of the patch is fine today.

    But it also needs to add two specifically namespace tests: concretely we need the following cases covered:

    a) namespace subdir/namespace subdir/test_foo.py gets loaded
    b) namespace subdir/test_foo.py with another instance of the namespace subdir on sys.path also containing e.g. test_bar.py, test_bar.py is not loaded.

    @rbtcollins
    Copy link
    Member

    (for the trivial case of CLI discover without a parameter - so translate that to the lower level API and then test that)

    @methane
    Copy link
    Member

    methane commented Jul 19, 2020

    I had rejected this idea in bpo-29642. This is a copy of my comments in the issue.

    ---

    I'm afraid this change makes testloader searches unrelated directory contains massive files (like node_modules).

    I don't think loading all tests from whole namespace package is not usual use case.

    When using import, (namespace) package name is explicitly specified.
    Only specified name is searched.

    In test loader's case, there are no such limit.
    Loader may search millions of completely unrelated directories.
    It may include directories in NFS or samba over slow network.

    @methane methane added the 3.10 only security fixes label Jul 19, 2020
    @methane
    Copy link
    Member

    methane commented Jul 19, 2020

    I think people misunderstanding and misusing PEP-420, withouth knowing what is namespace package for.

    I had wrote an article to describe namespace package is not a regular package.
    https://dev.to/methane/don-t-omit-init-py-3hga

    @methane
    Copy link
    Member

    methane commented Jul 19, 2020

    Searching into directory without __init__.py recursively is not only inefficient, but also dangerous.

    project/

    • mylib/
      • __init__.py
      • foo.py
    • tests/
      • __init__.py
      • test_foo.py
    • tools/
      • bin/
        • dangerous_script.py

    What happens if python -m unittest is run in the project root?
    Who excepts tools/bin/dangarous.py is executed?

    My conclution is:

    • People shouldn't abuse PEP-420. Omitting __init__.py is allowed only for namespace package.
    • Namespace package should be searched based on PEP-420 rule. Don't search into regular directory unless it is specified.

    @rgammansgammasciencecouk
    Copy link
    Mannequin

    But namespace packages are still useful for what PEP-420 envisages and they should be able to have runnable tests.

    For instance

    projectX/

    • interfaces/
      - proxyX.py
      - testX.py
      projectY/
    • intefaces/
      - proxyY.py
      - testY.py

    Here interfaces is a namespace package and projectX and projectY are kept separate, perhaps to reduce dependency and/or for separation of concerns.

    I don't think this is insurmountable - even taking into account Inada's very good point about dangerous_scripts. A full tests/ packages in on both projectX and projectY would allow their test to run. However, in some environments, it is normal to put the test alongside the code as shown here.

    But some better documentation on this issue would advisable, and some guidance on the best practice.

    Python has a complex ecosystem, and so an individual developer might hit this problem but be using a third party framework, which hasn't taken all of this on board. Eg. https://code.djangoproject.com/ticket/30403 . (I'm not really singling django out, although it's the one I hit) So it important that there is a clear understanding of the best way to deal with this case.

    @methane
    Copy link
    Member

    methane commented Jul 19, 2020

    In such case, both directories must be specified explicitly.

    Test discovery doesn't know that a directory is namespace package or
    namespace package. So it shouldn't assume it is namespace package unless
    specified.

    Note that PEP-420 searches namespace package only when it is specified.

    @methane methane added the docs Documentation in the Doc dir label Jul 20, 2020
    @methane
    Copy link
    Member

    methane commented Feb 22, 2021

    New changeset 5a4aa4c by Inada Naoki in branch 'master':
    bpo-23882: Doc: Clarify unittest discovery document (GH-21560)
    5a4aa4c

    @miss-islington
    Copy link
    Contributor

    New changeset 9dd018e by Miss Islington (bot) in branch '3.8':
    bpo-23882: Doc: Clarify unittest discovery document (GH-21560)
    9dd018e

    @miss-islington
    Copy link
    Contributor

    New changeset 30fe3ee by Miss Islington (bot) in branch '3.9':
    bpo-23882: Doc: Clarify unittest discovery document (GH-21560)
    30fe3ee

    @methane methane added 3.8 only security fixes 3.9 only security fixes labels Feb 22, 2021
    @methane methane closed this as completed Feb 22, 2021
    @terryjreedy
    Copy link
    Member

    From the merge:

    +++ b/Doc/library/unittest.rst
    @@ -330,7 +330,9 @@ Test modules and packages can customize test loading and discovery by through
     the `load_tests protocol`_.
     
     .. versionchanged:: 3.4
    -   Test discovery supports :term:`namespace packages <namespace package>`.
    +   Test discovery supports :term:`namespace packages <namespace package>`
    +   for start directory. Note that you need to the top level directory too.
    +   (e.g. ``python -m unittest discover -s root/namespace -t root``).
     
    The last sentence is missing a verb after 'you need to' saying what to do about "the top level directory.  "be in"?  "go to"?  "later destroy"? (just kidding ;-)

    @terryjreedy terryjreedy reopened this Feb 22, 2021
    @mdjonyhossainhabib
    Copy link
    Mannequin

    mdjonyhossainhabib mannequin commented Feb 22, 2021

    diff -r 293d9964cf6e Lib/unittest/loader.py
    --- a/Lib/unittest/loader.py	Tue Apr 28 00:04:53 2015 -0400
    +++ b/Lib/unittest/loader.py	Tue Apr 28 10:12:07 2015 +0300
    @@ -338,7 +338,7 @@
                 raise ImportError('Start directory is not importable: %r' % start_dir)
     
             if not is_namespace:
    -            tests = list(self._find_tests(start_dir, pattern))
    +            tests = list(self._find_tests(start_dir, pattern, namespace=True))
             return self.suiteClass(tests)
     
         def _get_directory_containing_module(self, module_name):
    @@ -403,7 +403,7 @@
                     name = self._get_name_from_path(full_path)
                     self._loading_packages.add(name)
                     try:
    -                    yield from self._find_tests(full_path, pattern, namespace)
    +                    yield from self._find_tests(full_path, pattern, False)
                     finally:
                         self._loading_packages.discard(name)
     
    diff -r 293d9964cf6e Lib/unittest/test/test_program.py
    --- a/Lib/unittest/test/test_program.py	Tue Apr 28 00:04:53 2015 -0400
    +++ b/Lib/unittest/test/test_program.py	Tue Apr 28 10:12:07 2015 +0300
    @@ -16,7 +16,7 @@
             expectedPath = os.path.abspath(os.path.dirname(unittest.test.__file__))
     
             self.wasRun = False
    -        def _find_tests(start_dir, pattern):
    +        def _find_tests(start_dir, pattern, namespace):
                 self.wasRun = True
                 self.assertEqual(start_dir, expectedPath)
                 return tests

    @methane
    Copy link
    Member

    methane commented Feb 23, 2021

    I noticed that namespace package support has been broken since this commit.
    bbbcf86

    Now namespace pacakge has __file__ attribute which is None. But...

                try:
                    start_dir = os.path.abspath(
                       os.path.dirname((the_module.__file__)))
                except AttributeError:
                    # look for namespace packages
    

    the_module.__file__ doesn't raise AttributeError for now. But os.path.dirname(None) raise TypeError.

    The commit is backported to 3.7 branch. So namespace package support has been broken since Python 3.7.

    Shouldn't we drop namespace package support?
    It is misleading. And we could not maintain it. We didn't notice that it is broken for 3 years!

    @ericvsmith
    Copy link
    Member

    As the author of PEP-420, I think having test discovery support namespace packages is a mistake, at least in the sense of pretending every directory is a namespace package.

    @adamchainz
    Copy link
    Mannequin

    adamchainz mannequin commented Nov 22, 2021

    I just reported https://bugs.python.org/issue45864 , and closed as duplicate of this.

    @methane methane added 3.11 only security fixes and removed 3.8 only security fixes 3.9 only security fixes 3.10 only security fixes labels Nov 26, 2021
    @methane
    Copy link
    Member

    methane commented Jan 10, 2022

    New changeset 0b2b9d2 by Inada Naoki in branch 'main':
    bpo-23882: unittest: Drop PEP-420 support from discovery. (GH-29745)
    0b2b9d2

    @methane methane closed this as completed Mar 7, 2022
    @methane methane added the invalid label Mar 7, 2022
    @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.11 only security fixes docs Documentation in the Doc dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    7 participants