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

Move test/support.py into a test.support subpackage #59699

Closed
cjerdonek opened this issue Jul 30, 2012 · 40 comments
Closed

Move test/support.py into a test.support subpackage #59699

cjerdonek opened this issue Jul 30, 2012 · 40 comments
Assignees
Labels
easy tests Tests in the Lib/test dir type-feature A feature request or enhancement

Comments

@cjerdonek
Copy link
Member

BPO 15494
Nosy @loewis, @ncoghlan, @pitrou, @ezio-melotti, @merwok, @bitdancer, @florentx, @cjerdonek, @shihai1991
PRs
  • bpo-40275: Use new test.support helper submodules in tests #20824
  • Files
  • move_test_support.patch: moves test.test_support to the test.support
  • 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/ncoghlan'
    closed_at = <Date 2013-07-28.10:56:43.943>
    created_at = <Date 2012-07-30.01:37:10.932>
    labels = ['easy', 'type-feature', 'tests']
    title = 'Move test/support.py into a test.support subpackage'
    updated_at = <Date 2020-06-15.13:05:48.554>
    user = 'https://github.com/cjerdonek'

    bugs.python.org fields:

    activity = <Date 2020-06-15.13:05:48.554>
    actor = 'shihai1991'
    assignee = 'ncoghlan'
    closed = True
    closed_date = <Date 2013-07-28.10:56:43.943>
    closer = 'python-dev'
    components = ['Tests']
    creation = <Date 2012-07-30.01:37:10.932>
    creator = 'chris.jerdonek'
    dependencies = []
    files = ['30850']
    hgrepos = []
    issue_num = 15494
    keywords = ['patch', 'easy']
    message_count = 40.0
    messages = ['166848', '166849', '166852', '166854', '166858', '166864', '166871', '166872', '166873', '173721', '173722', '173723', '173727', '173732', '173733', '173734', '173736', '173737', '173757', '173762', '173790', '173791', '173792', '173799', '173826', '173849', '173906', '173907', '173908', '185515', '185588', '185590', '185618', '188541', '192609', '192616', '192623', '193816', '193817', '193955']
    nosy_count = 13.0
    nosy_names = ['loewis', 'ncoghlan', 'pitrou', 'ezio.melotti', 'eric.araujo', 'r.david.murray', 'flox', 'chris.jerdonek', 'python-dev', 'mjdorma', 'sptonkin', 'italip', 'shihai1991']
    pr_nums = ['20824']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue15494'
    versions = ['Python 3.3', 'Python 3.4']

    @cjerdonek
    Copy link
    Member Author

    This issue is to move test/support.py into a test.support subpackage as discussed in bpo-15403.

    This can be done post-release.

    @cjerdonek cjerdonek added the tests Tests in the Lib/test dir label Jul 30, 2012
    @cjerdonek
    Copy link
    Member Author

    If people are okay with populating test/support/init.py with code (at least initially), the first patch could be to move support.py into test/support/init.py. One nice thing about this approach is that the calling code won't need to change.

    The second patch could be to move test/script_helper.py into the package.

    @ezio-melotti
    Copy link
    Member

    I'd prefer to keep test.support as a single module, rather than converting it to a package.
    Most of the time I open up test.support when I'm trying to find some function, so having more files will only make this more complicated.

    @ezio-melotti ezio-melotti added the type-feature A feature request or enhancement label Jul 30, 2012
    @ncoghlan
    Copy link
    Contributor

    Note that I don't think test.support itself should be broken up - I don't see any good reason to split the current grab bag of functionality out into submodules, so you'd just have support/init.py open instead.

    This is really about giving us a better way to organise the *other* helper modules like script_helper and make them easier to find.

    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Jul 30, 2012

    -1. test.support is not at all too large for a single module; there is no point in refactoring it.

    Without a specific patch to review which proposes some specific change, I'm rejecting this change request.

    @loewis loewis mannequin closed this as completed Jul 30, 2012
    @ncoghlan
    Copy link
    Contributor

    Martin, this change has been specifically requested by me to better organise all the support code that ISN'T in test.support.

    That file is already huge, and I'm not going to make it even bigger with all the test infrastructure needed for generating packaging heirarchies and zip files and invoking Python subprocesses in various ways.

    However, that support code *does* need to be made more discoverable (so we stop reinventing these wheels badly).

    A package with the current support.py as its __init__.py is the obvious solution.

    @ncoghlan ncoghlan reopened this Jul 30, 2012
    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Jul 30, 2012

    So who is going to provide a patch for it, and when?

    I don't think the tracker is the right place to keep list of things that someone wants to do some day. There isn't an issue "Python should have a JIT", either. Tracker issues should be actionable at the time the issue is submitted, unless there is a clear criterion on which to defer the issue.

    If the objective is to just put the module into an __init__.py, it takes just a few minutes, no?

    @ncoghlan
    Copy link
    Contributor

    As noted in the original post, this is a change which will be made once the 3.3 release is out the door. It's origin lies in the fact that one of the new pkgutil tests currently lives in test_runpy because test_runpy has much better infrastructure for that kind of thing.

    While figuring out where to put some shared infrastructure for the runpy, pkgutil, import and importlib tests, I came to the conclusion that having support.py, script_helper.py and whatever we decide to call the new module all living in the main test directory makes the support modules unnecessarily hard to find, and merging them all into a single massive support.py module doesn't make sense either.

    Chris has been working on the patches. We were going to try to get this refactoring done before the release, but enough other things have ended up coming up that Antoine and I decided it was better to wait.

    @ncoghlan ncoghlan self-assigned this Jul 30, 2012
    @ncoghlan
    Copy link
    Contributor

    I've just gone through and made sure all the related issues are correctly assigned to me.

    @ezio-melotti
    Copy link
    Member

    Tonight I was looking for assert_python_failure() in test.support, and it took me a while to figure out it was in script_helper.py instead.

    As suggested in msg166778 by Antoine, I would rather pick option 1 and merge everything in test.support, rather than further scattering these functions among different files.

    @ncoghlan
    Copy link
    Contributor

    test.support will become a package in 3.3 and trunk after the final 3.2
    release goes out.

    Excessively large source files are a sign of bad software design.

    @ezio-melotti
    Copy link
    Member

    Excessively large source files are a sign of bad software design.

    Are there actual reasons that support this claim?

    @ncoghlan
    Copy link
    Contributor

    Umm, WTF? You're really asking me to justify the basic software
    engineering principle that modularity is good, and wanting to dump
    everything in one file is one of the classic signs that your
    architectural design is non-existent and you have completely failed to
    adequately decompose your problem into smaller ones? Here it's even
    worse, because we *have* already done that decomposition, and people
    are proposing that we *abandon it*.

    The classic work on the physical design of software is probably Large
    Scale C++ Software Design from Lakos, and even though much of that is
    C++ specific (and the technical specifics are now rather dated even in
    that space), the general principles of understanding your physical
    dependencies and allowing suitable levels of granularity in your
    dependency hierarchy holds regardless of your development language.

    Small is beautiful (but not too small, that way lies madness).

    @pitrou
    Copy link
    Member

    pitrou commented Oct 25, 2012

    test.support will become a package in 3.3 and trunk after the final 3.2
    release goes out.

    Excessively large source files are a sign of bad software design.

    Do you weigh in the python-dev discussion about splitting
    unicodeobject.c?

    @pitrou
    Copy link
    Member

    pitrou commented Oct 25, 2012

    s/Do you weigh in/Do you want to weigh in/, sorry.

    @cjerdonek
    Copy link
    Member Author

    > Excessively large source files are a sign of bad software design.

    Do you [want to] weigh in the python-dev discussion about splitting
    unicodeobject.c?

    Nick did weigh in starting here:

    http://mail.python.org/pipermail/python-dev/2012-October/122391.html

    @pitrou
    Copy link
    Member

    pitrou commented Oct 25, 2012

    Ah, ok. Perhaps I should read python-dev before talking about it!

    @ncoghlan
    Copy link
    Contributor

    I already did, that thread 'tis a large part of why I'm somewhat irritable in relation to this topic today. "Huge source files are inherently bad because they provide no hint as to the modular breakdown and encourage excessive coupling between subcomponents" is just such a basic assumption of physical software design that I'm completely dumbfounded that people are questioning it.

    If the objections were "I think this particular proposed breakdown is bad", it would be one thing, but most of them aren't, they're "I don't think it should be broken up at all for stupidly trivial reasons that have nothing to do with anything".

    @ezio-melotti
    Copy link
    Member

    Umm, WTF? You're really asking me to justify the basic software
    engineering principle that modularity is good,

    I'm aware of the principle -- what I'm asking is what practical advantages this will bring (practicality beats purity).

    and wanting to dump everything in one file is one of the classic signs
    that your architectural design is non-existent and you have completely
    failed to adequately decompose your problem into smaller ones?

    As I see it, test.support is a module containing miscellaneous helper functions to be used in the tests. Therefore, if I'm looking for an helper function (like assert_python*()), I could just open the file and find it there.
    This is especially true for *generic* helper functions, like assert_python* (not widely used, see bpo-9517) and temp_dir (duplicated in support.py, see bpo-15376). Both these issues were probably caused by the fact that people didn't know about script_helper.py and ended up implementing their own versions of the functions either in the test files or in support.py.
    Something similar happened with requires_zlib (and possibly a few others) too -- it was defined in several test modules before being moved to support.py.

    Having several modules doesn't solve the problem of discoverability -- it actually make it worse by adding another level of indirection (find what files do you have and which one is the right one -> find what you are looking for in the right file). I often look at the code from hg.python.org, and ctrl+f there clearly covers only one file. Similarly inspecting/grepping a single file is easier than doing it for multiple files, especially if you don't know what the other files are.

    If people are not trying to find a specific function, but rather want an overview of the helper test functions, we could (and should) document them like we did with Doc/library/test.rst and have sections there. For example if I ls Lib/test/support/ and see a script_helper.py file, I could hardly guess that it contains functions to run python in a subprocress. This could be easily explained in the doc.

    That said, I should mention that 1) I'm not sure how many other helper modules there are around, how big they are, and how well things are already divided; 2) I don't know the structure that you want to use for the package. If you are just grouping the existing helper modules in a package and if the namespace is kept flat (so that we don't end up with test.support.whatever.TESTFN), then I agree that it's an improvement. If you are planning to further split support.py, then I'm -1.

    "Huge source files are inherently bad because they
    provide no hint as to the modular breakdown

    I can't think off the top of my head of a sensible breakdown for support.py. I can see that the assert/kill/spawn/python functions are all related, but the current module division seems to me quite arbitrary. Why import(_fresh)_module and related functions are not on their own module even if they have more code than all the assert/kill/spawn_python functions? Should all the skip/require decorators in support.py get their own module? What about the context manages like run_with_locale, temp_dir, temp_umask, EnvironmentVarGuard, etc.? Why script_helper contains helper functions to run python in a subprocess and also helper functions for scripts?

    If the objections were "I think this particular proposed
    breakdown is bad"

    If you suggest a breakdown I can provide better objections, however I think it's already broken down enough (and maybe even too much).

    I already did, that thread 'tis a large part of why
    I'm somewhat irritable in relation to this topic today.

    Same here, just in the opposite directions ;)

    P.S. incidentally the other day someone asked me where unittest.main was defined because he couldn't find it using "Find in files". We tried looking in __main__.py and __init__.py for "def main" and it wasn't there, but we noticed a "from .main import TestProgram, main". So we looked in main.py for a "def main" and still couldn't find it, so we looked for just "main". Eventually we found a "main = TestProgram" at the very bottom of main.py.

    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Oct 25, 2012

    Since "basic software engineering principles" have been invoked, I'd like to point out that many of the standard motivations to modularization don't apply in this case. So it is fair to ask what *specific* benefits such a restructuring may have. Such a description is necessary in advance in order to determine whether a specific restructuring actually achieves the stated objectives.

    In the Wikipedia article on "Modular programming", the main advantage (as in "top-down design") is that you can decompose a yet-to-be-built system upfront, and split the work into several teams, which then can develop the system independently, and only need to integrate at the end. This is less relevant as the system in question is already built.

    Another motivation given in Wikipedia is reuse in different systems. There isn't really any reuse planned for the code in question, but perhaps reuse in other Python implementations would make a point. If that is the motivation, the split should be to separate CPython support code from other support code.

    The wikipedia article also claims that the code is easier to debug, update, and modify. Easier modification may come from the notion of abstract interfaces: the module implementation can be changed without affecting module use (if the interfaces are unchanged. For this case, the code is already at a module boundary (of the support module); I fail to see why further modularization would improve anything.

    Easier debugging may come from less code being involved in a certain code path. Mere refactoring cannot improve that. You have to rewrite the functions, but that would be possible even without splitting the module.

    @cjerdonek
    Copy link
    Member Author

    If you are planning to further split support.py, then I'm -1.

    Ezio, this issue is not to split up support.py. Nick already responded to you to this effect in the fourth comment above. Also, in the second comment to this issue, I said that this issue was to move support.py as is into support/init.py, and then to move script_helper.py as is into the package.

    Having several modules doesn't solve the problem of discoverability

    We already have more than one module. This issue helps discoverability by grouping them in one place so people know what support modules there are. In other words, this issue addresses the following problem you mentioned:

    "Both these issues were probably caused by the fact that people didn't know about script_helper.py and ended up implementing their own versions of the functions either in the test files or in support.py. Something similar happened with requires_zlib (and possibly a few others)..."

    So it is fair to ask what *specific* benefits such a restructuring may have.

    Martin, see the paragraph above. In addition, this change makes it easier to search whatever support modules we have. It also provides a place to put new support modules to avoid further exacerbating the discoverability problem stated above.

    @ezio-melotti
    Copy link
    Member

    Ezio, this issue is not to split up support.py. Nick already responded
    to you to this effect in the fourth comment above.

    Indeed, I should have re-read the messages more carefully.

    Also, in the second comment to this issue, I said that this issue was
    to move support.py as is into support/init.py, and then to move
    script_helper.py as is into the package.

    We already have more than one module.

    Are there other modules except these two? Because if the package will only group these two, I think it's better/easier to merge script_helper.py in support.py.

    As I said in the previous message I don't see why the spawn/kill/assert_python functions should be in a separate file and the import_fresh_module ones shouldn't. Even if having these two files in the same package will make discoverability easier it still seems an arbitrary division.

    @bitdancer
    Copy link
    Member

    Every sizeable (and some not-so-sizeable) Python projects I've worked on have one or more 'utils' modules that collect stuff that doesn't logically fit elsewhere. That's what test.support is for the test infrastructure.

    I agree that we should not talk in generalities. As Martin has indicated, appeal to authority in the form of "basic software engineering principles" is not enough by itself. We should look at specific proposals and decide based on specific arguments whether they will improve matters. And yes, their status as good engineering principles is meaningful, but only if the reasons for those specific principles actually apply in the specific case.

    Certainly making the functions in script_helpers more discoverable is a laudable goal.

    As an aside: I've worked on a project that is well modularized if you judge by the number and size of the source files (if you ignore a few single-class files created by former java programmers)...but it didn't follow any of the logical reasons Martin quoted for modularization: finding things in the source base was a pain, and different responsibility groups were responsible for files spread across the source tree and wound up stepping on each other's toes, and debugging was not aided by the source structure because while the APIs started out reasonably well defined they did not stay that way...because the responsibility areas were not well defined or maintained. *Why* you modularize, and *how* you modularize, are much more important than file size.

    @loewis
    Copy link
    Mannequin

    loewis mannequin commented Oct 25, 2012

    This may increasingly get off-topic, so I'll stop with generalities after this post.

    I find the effect of modularization on discoverability to be two-sided.

    On the one hand, splitting functionality into groups helps discoverability, since you don't have to wade through tons of API, and the risk "to see but not to recognize" is reduced.

    OTOH, the risk of entirely ignoring some functionality because it is placed somehwere where you wouldn't expect it is increased. The classical example is the the gethostname() function, which many people wouldn't expect in the socket module.

    I have personally given up on expecting "logical" grouping when trying to find functionality (not just in Python, but in general). Instead, I rely on full-text search, which works reasonably well these days. I do use modularity when studying "close friends" related to some API I already know.

    @ncoghlan
    Copy link
    Contributor

    This whole refactoring project grew out of the fact that Chris and I were looking for somewhere to put common helpers for package testing. Specfically, I wrote a heap of helpers for test_runpy that I wanted to use for the new pkgutil tests.

    The short term hack (because 3.3 was imminent) is that a couple of the pkgutil tests are *in* test_runpy, not because they belong there, but so they can use those helper utilities. We knew when we did it that wasn't a viable long term solution, but we didn't have a good place to put the helpers so test_pkgutil could get at them - they're packaging specific, so junking up test.support with them would be bad, and we knew from experience that a separate helper module (like script_helper) sucked for discoverability.

    So, the idea of making test.support a subpackage was born. The current contents of support.py go into the new __init__.py, script_helper moves there to make it more discoverable, and the utilities for packaging tests can move out of test_runpy and into test.support.pkg_helper. Both script_helper and pkg_helper can also be easily covered in the docs under the existing "test.support" caveat.

    We didn't start down this road on a whim - we did it because we ran up hard against a clear and obvious problem in the structure of the test suite's support modules.

    @ezio-melotti
    Copy link
    Member

    So IIUC the package will contain __init__.py (what is now support.py), script_helper, and pkg_helper.

    One reason why I proposed to merge script_helper with support.py is that functions like assert_python*() are used in several modules already, and are generic enough to belong in support.py IMHO (that's also why I was looking for them there). I count almost 20 modules that are currently using these functions, and there might be more that could benefit from them.

    In script_helper I also see two functions that seem to deal with packages: make_pkg and make_zip_pkg. If these get moved to pkg_helper, script_helper will be left with just an handful of generic functions.

    In addition, if the functions you want to add in pkg_helper are also generic enough, it might still be better to put everything in support.py.

    To summarize I think that adding a package with 3 modules is still better than having modules around in test/, but merging everything in support.py might be even better (this depends on the functions that will end up in each module, how generic they are, and how the modules are divide).

    @ncoghlan
    Copy link
    Contributor

    Whereas I actively want to avoid *deliberately* making test.support even
    more of an incoherent mess. I'm still utterly astonished you're actively
    promoting the idea. Please stop telling me to write awful code.

    @ezio-melotti
    Copy link
    Member

    Please stop telling me to write awful code.

    I'm not telling you to write awful code, I'm telling you that IME having to deal with a single file is easier, so that's what I would personally prefer. Clearly if other developers disagree and/or if there is too much unrelated code (for some definition of "much") in the same file, then the package can be still done.

    @bitdancer
    Copy link
    Member

    Let's not argue over generalities. Let's wait and argue over a specific patch proposal.

    @ncoghlan
    Copy link
    Contributor

    OK, Georg has locked the 3.2 branch now, so we can proceed with this without making any 3.2 blockers harder to deal with.

    @merwok
    Copy link
    Member

    merwok commented Mar 30, 2013

    FWIW I have no principled opinion about big vs. small test.support module, as this is purely internal code.

    The reasons for this proposal as I understand them are:

    • One module for script helpers, one module for package creation, etc. are not bad.
    • We don’t really want to spend time and thought cleaning up test.support, but it be good to stop adding to the mess.
    • Making it easier to find that some function already exists (e.g. script helpers weren’t known by all core devs) would be good.

    ISTM that moving support.py to support/init.py and moving script_helper.py and friends to support/*.py is a quick and efficient way to address these points and make our lives a little easier:

    • Script helpers, package helpers, etc. continue to be in their own file.
    • Time is not lost trying to do an upfront cleanup of test.support.
    • Developers looking at the test.support package are more likely to have a look at the dosctrings of the other helper modules than they do currently, and thus don’t reinvent the same helpers.

    @ezio-melotti
    Copy link
    Member

    FWIW the reason why I proposed to add assert_python_ok to test.support, is that from my point of view it's just one of the many *test* support utilities defined in test.support. I think that from Nick's point of view, assert_python_ok is just one of the many *script* support utilities defined in script_helper.
    Since I'm not familiar with the other utilities of script_helper, it seemed logical to me to have it where I expect all the other general utilities to be (i.e. test.support), but I figured out that Nick has a point too.

    @ncoghlan
    Copy link
    Contributor

    Yeah, it was only when we went to add "test.pkg_helper" (so that test_runpy and test_pkgutil could share it) that we put the brakes on and decided to stop making a bad situation worse and do something different.

    There are probably some other helpers we could spin out over time (like those for creating test servers). It becomes much easier to do that kind of refactoring once there's an obvious place to put them without bloating the test/support.py to crazy levels.

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented May 6, 2013

    Ah, I *thought* there was another module that started me down this path. bpo-11816 (a dis module upgrade that I finally deemed ready enough to commit) added "test.bytecode_helper", so the new test.support package will consist of at least:

    support/init.py # Current test.support
    support/bytecode_helper.py # Current test.bytecode_helper
    support/pkg_helper.py # New for test_runpy & test_pkgutil
    support/script_helper.py # Current test.script_helper

    @italip
    Copy link
    Mannequin

    italip mannequin commented Jul 8, 2013

    as per Nick's direction the attached patch moves test/support.py to test/support/init.py and includes small fixes for some of the tests that break as a consequence.

    @mjdorma
    Copy link
    Mannequin

    mjdorma mannequin commented Jul 8, 2013

    move_test_support.patch didn't break tests under Windows x64

    @sptonkin
    Copy link
    Mannequin

    sptonkin mannequin commented Jul 8, 2013

    Likewise, move_test_support.patch did not break things under OS X 10.8.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jul 28, 2013

    New changeset 203a77e74aa7 by Nick Coghlan in branch 'default':
    Close bpo-15494: test.support is now a package rather than a module
    http://hg.python.org/cpython/rev/203a77e74aa7

    @python-dev python-dev mannequin closed this as completed Jul 28, 2013
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jul 28, 2013

    New changeset 0b7ed24f7d33 by Nick Coghlan in branch '3.3':
    Issue bpo-15494: test.support is now a package rather than a module
    http://hg.python.org/cpython/rev/0b7ed24f7d33

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jul 30, 2013

    New changeset e6f2f3eda290 by Ned Deily in branch '3.3':
    Issue bpo-15494: Install new test/support directory.
    http://hg.python.org/cpython/rev/e6f2f3eda290

    New changeset 4df2e094f83e by Ned Deily in branch 'default':
    Issue bpo-15494: merge from 3.3
    http://hg.python.org/cpython/rev/4df2e094f83e

    @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
    easy tests Tests in the Lib/test dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    6 participants