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

Allow all resources if not running under regrtest.py #62692

Closed
zware opened this issue Jul 18, 2013 · 11 comments
Closed

Allow all resources if not running under regrtest.py #62692

zware opened this issue Jul 18, 2013 · 11 comments
Assignees
Labels
tests Tests in the Lib/test dir type-feature A feature request or enhancement

Comments

@zware
Copy link
Member

zware commented Jul 18, 2013

BPO 18492
Nosy @terryjreedy, @ezio-melotti, @bitdancer, @zware, @serhiy-storchaka
Files
  • regrtest_run.diff
  • issue18492.v2.diff
  • 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/zware'
    closed_at = <Date 2014-06-02.21:07:35.248>
    created_at = <Date 2013-07-18.15:38:20.381>
    labels = ['type-feature', 'tests']
    title = 'Allow all resources if not running under regrtest.py'
    updated_at = <Date 2014-06-02.23:02:59.321>
    user = 'https://github.com/zware'

    bugs.python.org fields:

    activity = <Date 2014-06-02.23:02:59.321>
    actor = 'terry.reedy'
    assignee = 'zach.ware'
    closed = True
    closed_date = <Date 2014-06-02.21:07:35.248>
    closer = 'zach.ware'
    components = ['Tests']
    creation = <Date 2013-07-18.15:38:20.381>
    creator = 'zach.ware'
    dependencies = []
    files = ['30968', '35171']
    hgrepos = []
    issue_num = 18492
    keywords = ['patch']
    message_count = 11.0
    messages = ['193310', '193314', '193360', '193386', '193387', '193494', '218064', '218509', '219639', '219640', '219649']
    nosy_count = 6.0
    nosy_names = ['terry.reedy', 'ezio.melotti', 'r.david.murray', 'python-dev', 'zach.ware', 'serhiy.storchaka']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue18492'
    versions = ['Python 2.7', 'Python 3.4', 'Python 3.5']

    @zware
    Copy link
    Member Author

    zware commented Jul 18, 2013

    Here's a patch to implement the idea I posted in bpo-18258, msg193242. The patch also removes usage of "support.use_resources = ['<resource>']" from the test package since it is no longer needed.

    (No test_main -> unittest.main conversions are done in this patch; the modules changed that still us. test_main are either covered by another issue or are more complex than a simple remove-and-replace conversion, and so I didn't want to lump them into this patch)

    @zware zware added tests Tests in the Lib/test dir type-feature A feature request or enhancement labels Jul 18, 2013
    @serhiy-storchaka
    Copy link
    Member

    I don't think we should accept anything if not running under regrtest, We can unintentionally run a testfile in which some tests consume a lot of resources. Also we need in skipping this tests for the test testing. I think it will be better just add a support of the -u option to discovery command line interface.

    @bitdancer
    Copy link
    Member

    Since we only use unittest for running subsets of our test suite, I think the better behavior is to run everything that -u all normally runs. That is, when one uses unittest to run a bit of the test suite, one generally wants to run the tests one specifies, not have them skipped.

    Obviously this is open to discussion, however ;)

    @terryjreedy
    Copy link
    Member

    Currently, all requires() tests pass when the file they occur in is run as '__main__'. This is especially needed when the file ends with the now standard boilerplate.
    if __name__ == '__main__':
    ...
    unittest.main(...)
    as there is currently no way to set resources within the unittest.main call.

    The problem is that this permissiveness does not apply to subsidiary files discovered from and run by a main file, even though it should. The current workaround is to explicitly set use_resources for the benefit of subsidiary files, as in test_idle.py.

    As I see it, the main point of this patch, somewhat obscured by the title, is to extend the current resource permissiveness from tests *in* main files run as main to tests in other files discovered and run from a main file. It also extends the permisiveness to any test not run by regrtest (ie, by unittest).

    The key change is

    • if sys._getframe(1).f_globals.get("__name__") == "__main__":
      + if not regrtest_run:

    'regrtest_run == True' is currently spelled 'use_resources is not None', so the latter could be used instead to replace the frame-check without otherwise adding new code.

    Extending the permissiveness from main files to subsidiary files strikes me as a no-brainer. Splitting a (potentially) large file into a master file and a package of subsidiary files should not affect which tests are run.

    More interesting is extending the permisiveness to tests run under unittest with "python -m unittest target". Target can be a master file, a test file, or a test case or test methods. Subfile targets can only be run with unittest, not regrtest, and there is no way to enable resources for such targets.
    python -m unittest idlelib.idle_test.test_xy.TextText # runs
    python -m unittest idlelib.idle_test.test_xy.GuiText # skips
    So the patch enables something that is currently not possible.

    Serhiy is concerned about the possible booby-trap for users that run slow resource intensive tests. Some thoughts:

    • Running tests as main is mainly done interactively, and usually by developers at that. Humans can stop a test and ignore errors better than buildbots.

    • There are multiple ways to run a file from the command line. The test chapter of the manual can document that
      python -m test.test_xyz
      is more or less equivalent to
      python -m test -uall test_zyz
      with -v possibly tossed in. Anyone who does not want that can still run under regrtest by using the currently documented
      python -m test test_xyz

    • Anyone running a test file loaded in an Idle window very likely wants to run all tests possible.

    • Documenting that running under unittest enables all resources is trickier as long as resources are cpython and regrtest specific. I think I would mention this in the test chapter, where resources are discussed, rather than the unittest chapter.

    *If -u is added to unittest (and 'use=' to .main), a default of all would be the right thing for subfile targets, even if not for file and multi-file targets.
    ---

    @terryjreedy
    Copy link
    Member

    bpo-18441 is partly related in that it also suggests (eventually) moving check code (for guis) from multiple test files to support and regrtest. I do not believe there would be any conflict.

    @zware
    Copy link
    Member Author

    zware commented Jul 22, 2013

    Terry J. Reedy added the comment:

    The problem is that this permissiveness does not apply to subsidiary files discovered from and run by a main file, even though it should. The current workaround is to explicitly set use_resources for the benefit of subsidiary files, as in test_idle.py.

    This is exactly what inspired this issue, though with
    test_codecmaps_*.py rather than test_idle.

    As I see it, the main point of this patch, somewhat obscured by the title, is to extend the current resource permissiveness from tests *in* main files run as main to tests in other files discovered and run from a main file. It also extends the permisiveness to any test not run by regrtest (ie, by unittest).

    I did choose the title rather poorly, but yes, this is the real point.

    The key change is

    • if sys._getframe(1).f_globals.get("__name__") == "__main__":
    • if not regrtest_run:

    'regrtest_run == True' is currently spelled 'use_resources is not None', so the latter could be used instead to replace the frame-check without otherwise adding new code.

    I think using the 'regrtest_run' flag is more explicit about why we're
    just letting anything go, and could potentially be useful in other
    situations as well (though I'm coming up blank on what any of those
    might be right now).

    <snip>

    Serhiy is concerned about the possible booby-trap for users that run slow resource intensive tests. Some thoughts:
    <snip thoughts>

    I agree with your thoughts here, which if I'm not mistaken basically
    boil down to "Yes, it's a booby-trap, but it's not a lethal one and we
    can document around most of it."

    *If -u is added to unittest (and 'use=' to .main), a default of all would be the right thing for subfile targets, even if not for file and multi-file targets.

    I'm not sure I understand this line. But I do have an initial patch
    adding -u to unittest which I will be posting shortly which, if
    accepted, should render this issue moot.

    @zware
    Copy link
    Member Author

    zware commented May 7, 2014

    Here's a new and better patch. This patch keeps the regrtest_run global in support and moves the regrtest-or-not check into is_resource_enabled to make is_resource_enabled, requires, and requires_resource consistent with each other and in a way that still allows explicitly setting support.use_resources. The real change is confined to Lib/test/regrtest.py and Lib/test/support/init.py, the rest of the patch is cleanup allowed by the change (except for test_decimal, which has a minor change required to allow one of the command line options to that script to work).

    @zware zware changed the title Add test.support.regrtest_run flag, simplify support.requires Allow all resources if not running under regrtest.py May 7, 2014
    @serhiy-storchaka
    Copy link
    Member

    You have convinced me. In general the approach and the patch LGTM.

    But I agree with Terry that flag support.regrtest_run is redundant, it duplicates a bit of information which provides support.use_resources. Instead we can rewrite support.is_resource_enabled() as

        return use_resources is None or resource in use_resources

    Introducing regrtest_run can cause inconsistency between it and use_resources.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jun 2, 2014

    New changeset eabff2a97b5b by Zachary Ware in branch '2.7':
    Issue bpo-18492: Allow all resources when tests are not run by regrtest.py.
    http://hg.python.org/cpython/rev/eabff2a97b5b

    New changeset 8519576b0dc5 by Zachary Ware in branch '3.4':
    Issue bpo-18492: Allow all resources when tests are not run by regrtest.py.
    http://hg.python.org/cpython/rev/8519576b0dc5

    New changeset 118e427808ce by Zachary Ware in branch 'default':
    Issue bpo-18492: Merge with 3.4
    http://hg.python.org/cpython/rev/118e427808ce

    @zware
    Copy link
    Member Author

    zware commented Jun 2, 2014

    You convinced me too, Serhiy :). Committed, without the regrtest_run flag. Thanks for review.

    @zware zware closed this as completed Jun 2, 2014
    @terryjreedy
    Copy link
    Member

    Just in time, as I will be reviewing and committing new Idle unittest module starting this week (new GSOC student).

    @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 type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants