| msg193310 - (view) |
Author: Zachary Ware (zach.ware) *  |
Date: 2013-07-18 15:38 |
| Here's a patch to implement the idea I posted in , . The patch also removes usage of "support.use_resources = ['']" 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) |
|
|
| msg193314 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2013-07-18 19:03 |
| 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. |
|
|
| msg193360 - (view) |
Author: R. David Murray (r.david.murray) * |
Date: 2013-07-19 14:44 |
| 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 ;) |
|
|
| msg193386 - (view) |
Author: Terry J. Reedy (terry.reedy) * |
Date: 2013-07-20 00:17 |
| 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. --- |
|
|
| msg193387 - (view) |
Author: Terry J. Reedy (terry.reedy) * |
Date: 2013-07-20 00:25 |
| #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. |
|
|
| msg193494 - (view) |
Author: Zachary Ware (zach.ware) * |
Date: 2013-07-22 04:11 |
| 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). > Serhiy is concerned about the possible booby-trap for users that run slow resource intensive tests. Some 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. |
|
|
| msg218064 - (view) |
Author: Zachary Ware (zach.ware) * |
Date: 2014-05-07 17:29 |
| 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). |
|
|
| msg218509 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2014-05-14 08:33 |
| 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. |
|
|
| msg219639 - (view) |
Author: Roundup Robot (python-dev)  |
Date: 2014-06-02 21:05 |
| New changeset eabff2a97b5b by Zachary Ware in branch '2.7': Issue #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 #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 #18492: Merge with 3.4 http://hg.python.org/cpython/rev/118e427808ce |
|
|
| msg219640 - (view) |
Author: Zachary Ware (zach.ware) * |
Date: 2014-06-02 21:07 |
| You convinced me too, Serhiy :). Committed, without the regrtest_run flag. Thanks for review. |
|
|
| msg219649 - (view) |
Author: Terry J. Reedy (terry.reedy) * |
Date: 2014-06-02 23:02 |
| Just in time, as I will be reviewing and committing new Idle unittest module starting this week (new GSOC student). |
|
|