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

Refactor package creation support code into a common location #59608

Open
cjerdonek opened this issue Jul 20, 2012 · 22 comments
Open

Refactor package creation support code into a common location #59608

cjerdonek opened this issue Jul 20, 2012 · 22 comments
Labels
tests Tests in the Lib/test dir

Comments

@cjerdonek
Copy link
Member

BPO 15403
Nosy @ncoghlan, @pitrou, @vstinner, @merwok, @cjerdonek, @ericsnowcurrently
Dependencies
  • bpo-15494: Move test/support.py into a test.support subpackage
  • bpo-18576: Document test.support.script_helper
  • bpo-19700: Update runpy for PEP 451
  • Files
  • issue-15403-1.patch
  • issue-15403-2.patch
  • issue-15403-3.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 = None
    created_at = <Date 2012-07-20.11:13:12.663>
    labels = ['tests']
    title = 'Refactor package creation support code into a common location'
    updated_at = <Date 2019-07-29.11:44:38.181>
    user = 'https://github.com/cjerdonek'

    bugs.python.org fields:

    activity = <Date 2019-07-29.11:44:38.181>
    actor = 'vstinner'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['Tests']
    creation = <Date 2012-07-20.11:13:12.663>
    creator = 'chris.jerdonek'
    dependencies = ['15494', '18576', '19700']
    files = ['26447', '26670', '30870']
    hgrepos = []
    issue_num = 15403
    keywords = ['patch']
    message_count = 22.0
    messages = ['165910', '165912', '166499', '166759', '166771', '166773', '166774', '166776', '166778', '166787', '166788', '166790', '166793', '166844', '166847', '166850', '167266', '192708', '193822', '193823', '205639', '348630']
    nosy_count = 7.0
    nosy_names = ['ncoghlan', 'pitrou', 'vstinner', 'eric.araujo', 'chris.jerdonek', 'eric.snow', 'italip']
    pr_nums = []
    priority = 'normal'
    resolution = None
    stage = 'needs patch'
    status = 'open'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue15403'
    versions = ['Python 3.3']

    @cjerdonek
    Copy link
    Member Author

    This issue addresses the "file creation" portion of bpo-15376, which is to refactor the walk_package support code in test_runpy into a common location.

    @cjerdonek cjerdonek added easy tests Tests in the Lib/test dir labels Jul 20, 2012
    @cjerdonek
    Copy link
    Member Author

    Attaching patch.

    @ncoghlan ncoghlan self-assigned this Jul 26, 2012
    @cjerdonek
    Copy link
    Member Author

    It occurs to me that the filecmp/dircmp tests in Lib/test/test_filecmp.py would also benefit from code like this (i.e. being able to create a nested directory of files in one or two lines). And perhaps elsewhere in the tests.

    This is an argument for slightly generalizing the test support API in the uploaded patch from creating modules/packages to creating files/directories. A convenience wrapper for packages could still be included that includes an __init__.py in the calls to the underlying directory code.

    @cjerdonek
    Copy link
    Member Author

    Lib/test/test_import.py also contains test code that would benefit from this (see for example bpo-15425). (Though not all files need to be refactored in a single issue.)

    @pitrou
    Copy link
    Member

    pitrou commented Jul 29, 2012

    Please let's not have something called test.supportlib in addition to test.support. If test.support grows too large we can turn it into a package.

    @cjerdonek
    Copy link
    Member Author

    I feel like it is already too large (it is over 1750 lines), and I did not want to create a third sibling test support module (there is also test/script_helper.py that overlaps with test.support). Do you think that the community would be open to refactoring test.support into a package for Python 3.3? This was meant to assist in increasing test coverage for Python 3.3 bugs.

    @pitrou
    Copy link
    Member

    pitrou commented Jul 29, 2012

    I feel like it is already too large (it is over 1750 lines), and I did
    not want to create a third sibling test support module (there is also
    test/script_helper.py that overlaps with test.support). Do you think
    that the community would be open to refactoring test.support into a
    package for Python 3.3? This was meant to assist in increasing test
    coverage for Python 3.3 bugs.

    That's too late for 3.3, IMO. Whether or not test.support is too large
    is a matter of taste (we have larger files in the stdlib; what makes
    test.support annoying to work with is that it's a hodge-podge of utility
    functions with little care for consistency). Unless we can clearly
    separate it in thematic areas, making it a package would only make
    finding stuff more difficult.

    @cjerdonek
    Copy link
    Member Author

    At this point, would you advise me to add even more to the existing hodge podge, or to create a third sibling test support module? My patch adds closely related test support functionality.

    Incidentally, this discussion relates to the point I was getting at in the python-dev thread in the past couple days about how restrictions on refactoring test support code for maintenance releases can affect the quality of our test code. I am still struggling with how to approach that.

    @pitrou
    Copy link
    Member

    pitrou commented Jul 29, 2012

    I don't mind refactoring test support routines in maintenance release. I
    said it was too late for 3.3 because the beta period is closing; but it
    could be fine for 3.3.1 ;)

    As for the way forward, I see three possibilites:

    1. put everying in test.support, as a single module (that includes
      folding script_helper into it)
    2. make test.support a package of thematic modules
    3. refactor test.support into a variety of independent modules
      (script_helper, import_helper, socket_helper, etc.); a small
      test.support providing fundamental features such as TESTFN could remain

    My personal preference would be for 1) or 3). But whatever we choose, we
    should do it consistently. Having a large test.support + a supportlib
    package + some independent helpers is not a good idea.

    @cjerdonek
    Copy link
    Member Author

    Thanks for your thoughts. For the purposes of this patch, I will change to putting the new support functionality in test.support. Going forward, if we could do some of the refactoring for 3.3.1, that would be great. :)

    I worry that the third option may make things worse because it would become less obvious where all of the different test support functionality is located.

    Are you opposed to (2), or is it simply less favorable? The transition to (2) could be a gradual one beginning with two or even one module inside the package.

    @pitrou
    Copy link
    Member

    pitrou commented Jul 29, 2012

    Are you opposed to (2), or is it simply less favorable?

    Less favorable, because it produces longer import strings
    ("test.support.some_helper" instead of "test.some_helper").

    @cjerdonek
    Copy link
    Member Author

    Less favorable, because it produces longer import strings
    ("test.support.some_helper" instead of "test.some_helper").

    This can be addressed by exposing the API in __init__.py though (as does, say, the unittest package), no?

    @pitrou
    Copy link
    Member

    pitrou commented Jul 29, 2012

    Le dimanche 29 juillet 2012 à 17:00 +0000, Chris Jerdonek a écrit :

    Chris Jerdonek added the comment:

    > Less favorable, because it produces longer import strings
    ("test.support.some_helper" instead of "test.some_helper").

    This can be addressed by exposing the API in __init__.py though (as
    does, say, the unittest package), no?

    Yes, that's right.

    @ncoghlan
    Copy link
    Contributor

    Discoverability is definitely a problem - part of that is a docs issue, since test.support is currently the only one mentioned in the prose docs.

    One advantage to moving to a support subpackage is that we can lose the "helper" suffix from the names, while still allowing thematic divisions.

    So, for example, we could have

    # support.py -> support/__init__.py
    import test.support
    # script_helper.py -> support/scripts.py
    import test.support.scripts
    # Other possible additions
    import test.support.imports
    import test.support.network

    Even without updating the prose docs, this would help discoverability a lot by having a much smaller directory listing to scan for useful support code. At the moment the general purpose helper modules are mixed in with the tests

    I agree with Antoine that we're probably better off handling this as a refactor post-release at this point, though. I'd hoped to have the time to devote to it beforehand, but there's user facing stuff that's higher priority right now.

    @cjerdonek
    Copy link
    Member Author

    Sounds good. Later today I will create an issue to move test/support.py into a test.support subpackage post-release.

    We can continue the discussion of how to organize it there.

    @cjerdonek
    Copy link
    Member Author

    Later today I will create an issue to move test/support.py into a test.support subpackage post-release.

    I created bpo-15494 for this.

    @cjerdonek
    Copy link
    Member Author

    Attaching an updated patch. Changes include:

    1. Cleans up and expands support API to include creating nested directory structures (for use in the filecmp tests, for example).
    2. Adds unit tests for the full API (similar to test_support.py).
    3. Refactors test_filecmp and test_import to use the support code, in addition to test_runpy and test_cmd_line_script. We can remove some of these refactorings if you think it does not add much at this point.
    4. Puts changes in package_helper.py for now (until bpo-15494 is done). I didn't put them in script_helper.py to avoid creating a circular dependency.

    @italip
    Copy link
    Mannequin

    italip mannequin commented Jul 9, 2013

    updated patch to apply on top of patch from bpo-15494

    @ncoghlan
    Copy link
    Contributor

    As noted in bpo-18576 and bpo-18578, I'd like for these relocation patches to include the addition of documentation for the submodule in Doc/library/test.rst

    @ncoghlan
    Copy link
    Contributor

    Also, now that test.support is a subpackage, the helper should be a submodule of that (test.support.package_helper) rather than directly in the test directory.

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Dec 9, 2013

    I took a look at this last night, and found the combination of moving stuff around *and* refactoring it at the same time was too hard to review.

    So, once the PEP-451 changes for runpy are done, I think it would make more sense to tackle this as at least three patches:

    1. Do the refactoring *within* test_runpy to make the helper API a bit cleaner (this should also use types.SimpleNamespace where appropriate rather than a custom alternative)

    2. Move the helper API out to a documented test.support.package_helper module without altering the test.support API

    3+. Refactor other test modules to use the now shared API (there may be some pkg related functionality to move out of script_helper as well).

    @ncoghlan ncoghlan removed their assignment Jun 28, 2015
    @vstinner
    Copy link
    Member

    This issue is 7 years old and has patches: it is no newcomer friendly, I remove the "easy" keyword.

    @vstinner vstinner removed the easy label Jul 29, 2019
    @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
    tests Tests in the Lib/test dir
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants