Issue32071
This issue tracker has been migrated to GitHub,
and is currently read-only.
For more information,
see the GitHub FAQs in the Python's Developer Guide.
Created on 2017-11-18 18:12 by jonash, last changed 2022-04-11 14:58 by admin. This issue is now closed.
Pull Requests | |||
---|---|---|---|
URL | Status | Linked | Edit |
PR 4496 | merged | jonash, 2017-11-21 20:55 | |
PR 4589 | merged | jonash, 2017-11-27 17:04 |
Messages (19) | |||
---|---|---|---|
msg306490 - (view) | Author: Jonas H. (jonash) * | Date: 2017-11-18 18:12 | |
I'd like to add test selection based on parts of the test class/method name to unittest. Similar to py.test's "-k" option: https://docs.pytest.org/en/latest/example/markers.html#using-k-expr-to-select-tests-based-on-their-name Here's a proof of concept implementation: https://github.com/jonashaag/cpython/compare/master...unittest-select Is this something others find useful as well? If so, I'd like to work on getting this into Python stdlib proper. This is my first time contributing to the unittest framework; is the general approach taken in my PoC implementation correct in terms of abstractions? How can I improve the implementation? Jonas |
|||
msg306492 - (view) | Author: Jonas H. (jonash) * | Date: 2017-11-18 18:18 | |
Just to be clear, the current implementation is limited to substring matches. It doesn't support py.test like "and/or" combinators. (Actually, py.test uses 'eval' to support arbitrary patterns.) So say we have test case SomeClass test_foo test_bar Then - python -m unittest -k fo matches "test_foo" - python -m unittest -k Some matches "test_foo" and "test_bar" - python -m unittest -k some matches nothing The -k option may be used multiple times, combining the patterns with "or": - python -m unittest -k fo -k b matches "test_foo" and "test_bar" It's also possible to use glob-style patterns, like -k "spam_*_eggs". |
|||
msg306495 - (view) | Author: Antoine Pitrou (pitrou) * | Date: 2017-11-18 20:24 | |
I think this would be a useful enhancement. Feel free to open a PR. I haven't looked at your implementation, but you'll also need to add tests for it. |
|||
msg306584 - (view) | Author: Jonas H. (jonash) * | Date: 2017-11-20 20:58 | |
Thanks Antoine. I will need some guidance as to what are the correct places to make these changes. I'm not quite sure about the abstractions here (runner, loader, suite, case, etc.) My PoC (see GitHub link in first post) uses a TestSuite subclass. (The subclass is only so that it's easier to assess the general implementation approach; I guess it should be put into the main class instead.) Things I'm unsure of: 1) Is suite the correct place for this kind of feature? 2) Is the hardcoded fnmatch-based pattern matcher ok, or do we need a new abstraction "NameMatcher"? 3) Is the approach of dynamically wrapping 'skip()' around to-be-skipped test cases OK? 4) The try...catch statement around 'test.id()' is needed because there are some unit tests (unit tests for the unittest module itself) that check for some error cases/error handling in the unittest framework, and crash if we try to call '.id()' on them. Please remove the try...catch to see these errors if you're interested in the details. Is the check OK like that, or is this a code smell? Thanks Jonas |
|||
msg306593 - (view) | Author: Antoine Pitrou (pitrou) * | Date: 2017-11-20 22:30 | |
First, I should clarify that I'm not a unittest maintainer. However, as far as I can tell, the maintainers have not been very active lately. Also, this is a reasonably simple enhancement (at least conceptually), so I think can do without a maintainer's formal approval. To your questions: > 1) Is suite the correct place for this kind of feature? At its core, TestSuite is really a glorified collection of tests. The selection logic is inside the TestLoader, and indeed the TestLoader docstring says: """This class is responsible for loading tests according to various criteria and returning them wrapped in a TestSuite""" So I would recommend putting this in TestLoader. > 2) Is the hardcoded fnmatch-based pattern matcher ok, or do we need a new abstraction "NameMatcher"? I think the hardcoded approach is ok. Though to benefit readability you may want to use a predicate function instead. > 3) Is the approach of dynamically wrapping 'skip()' around to-be-skipped test cases OK? I think this is the wrong approach. A test that isn't selected shouldn't be skipped, it should not appear in the output at all. Another reason for putting this in TestLoader :-) > 4) The try...catch statement around 'test.id()' is needed because there are some unit tests (unit tests for the unittest module itself) that check for some error cases/error handling in the unittest framework, and crash if we try to call '.id()' on them I'd ask the question differently: do you need to call .id() to do the matching at all? Intuitively, the TestLoader probably knows about the test names already, even before it instantiates TestCases for them. TestCases shouldn't be instantiated for tests that are not selected. |
|||
msg306596 - (view) | Author: Jonas H. (jonash) * | Date: 2017-11-20 23:23 | |
> > 3) Is the approach of dynamically wrapping 'skip()' around to-be-skipped test cases OK? > I think this is the wrong approach. A test that isn't selected shouldn't be skipped, it should not appear in the output at all. Another reason for putting this in TestLoader :-) My first implementation actually was mostly the test loader. Two things made me change my mind and try to make the changes in the suite code: - The loader code really only deals with loading (i.e., finding + importing) tests. Yes it expects a file pattern like "test*.py" for identifying test case files. But apart from that it didn't "feel" right to put name based selection there. - In py.test you'll get a console output like "5 tests passed, 1 test failed, 12 tests deselected". We can't get anything similar without making bigger changes to the test loader, runner, etc. code. Using skip() we at least have some info on "skipped" tests, although they're technically not skipped. Are you still saying this should go to the test loader? |
|||
msg306598 - (view) | Author: Antoine Pitrou (pitrou) * | Date: 2017-11-20 23:32 | |
Le 21/11/2017 à 00:23, Jonas H. a écrit : > > - The loader code really only deals with loading (i.e., finding + importing) tests. Yes it expects a file pattern like "test*.py" for identifying test case files. But apart from that it didn't "feel" right to put name based selection there. Take a look at TestLoader.loadTestsFromName(). It does much more than look up test files, it can also look up individual classes or methods. > - In py.test you'll get a console output like "5 tests passed, 1 test failed, 12 tests deselected". We can't get anything similar without making bigger changes to the test loader, runner, etc. code. Using skip() we at least have some info on "skipped" tests, although they're technically not skipped. I'm not sure what the point of displaying the number of "deselected" tests is: that information doesn't sound very useful in itself. But in any case, marking them skipped feels wrong to me. Most often, skipping a test is an indication that the current system is not fit to run it (such as not enough RAM, or lacking a third-party library, or being the wrong OS entirely). > Are you still saying this should go to the test loader? IMHO, yes. Looking at your posted implementation, at least I don't think TestSuite is the place for it. |
|||
msg306599 - (view) | Author: Antoine Pitrou (pitrou) * | Date: 2017-11-20 23:35 | |
If it helps, think of TestLoader as collecting tests, rather than simply loading Python modules. |
|||
msg306601 - (view) | Author: STINNER Victor (vstinner) * | Date: 2017-11-20 23:43 | |
The internal Python test runner "regrtest" already implements the feature as the -m MATCH option and --matchfile FILENAME option. It's implemented in test.support._match_file(). See bpo-31324 for a recent issue on this feature: performance issue (it's going to be fixed). |
|||
msg306687 - (view) | Author: Jonas H. (jonash) * | Date: 2017-11-21 20:55 | |
Interesting, Victor. I've had a look at the code you mentioned, but I'm afraid it doesn't really make sense to re-use any of the code. Here's a new patch, implemented in the loader as suggested by Antoine, and with tests. I'm happy to write documentation etc. once we're through with code review. https://github.com/python/cpython/pull/4496 |
|||
msg306959 - (view) | Author: Antoine Pitrou (pitrou) * | Date: 2017-11-25 15:23 | |
New changeset 5b48dc638b7405fd9bde4d854bf477dfeaaddf44 by Antoine Pitrou (Jonas Haag) in branch 'master': bpo-32071: Add unittest -k option (#4496) https://github.com/python/cpython/commit/5b48dc638b7405fd9bde4d854bf477dfeaaddf44 |
|||
msg306960 - (view) | Author: Antoine Pitrou (pitrou) * | Date: 2017-11-25 15:24 | |
The enhancement is now pushed to git master. Thank you Jonas for contributing this! |
|||
msg307041 - (view) | Author: STINNER Victor (vstinner) * | Date: 2017-11-27 10:25 | |
IMHO it's a big enhancement for the base unittest test runner and deserves to be documend in What's New in Python 3.7! Jonas: do you want to write a PR to patch Doc/whatsnew/3.7.rst? |
|||
msg307046 - (view) | Author: Jonas H. (jonash) * | Date: 2017-11-27 10:56 | |
Sure! |
|||
msg307056 - (view) | Author: Tim Graham (Tim.Graham) * | Date: 2017-11-27 15:55 | |
This is appearing as a backwards incompatible change for Django because test case class attributes are now evaluated at load time before setUpClass runs (which initializes some things that those class attributes use). It's possible to adapt Django for this change, but it might affect other projects as well. Traceback from Django's tests: $ python -Wall tests/runtests.py Testing against Django installed in '/home/tim/code/django/django' with up to 3 processes Traceback (most recent call last): File "tests/runtests.py", line 477, in <module> options.exclude_tags, File "tests/runtests.py", line 282, in django_tests extra_tests=extra_tests, File "/home/tim/code/django/django/test/runner.py", line 598, in run_tests suite = self.build_suite(test_labels, extra_tests) File "/home/tim/code/django/django/test/runner.py", line 513, in build_suite tests = self.test_loader.discover(start_dir=label, **kwargs) File "/home/tim/code/cpython/Lib/unittest/loader.py", line 346, in discover tests = list(self._find_tests(start_dir, pattern)) File "/home/tim/code/cpython/Lib/unittest/loader.py", line 403, in _find_tests full_path, pattern, namespace) File "/home/tim/code/cpython/Lib/unittest/loader.py", line 457, in _find_test_path return self.loadTestsFromModule(module, pattern=pattern), False File "/home/tim/code/cpython/Lib/unittest/loader.py", line 124, in loadTestsFromModule tests.append(self.loadTestsFromTestCase(obj)) File "/home/tim/code/cpython/Lib/unittest/loader.py", line 90, in loadTestsFromTestCase testCaseNames = self.getTestCaseNames(testCaseClass) File "/home/tim/code/cpython/Lib/unittest/loader.py", line 234, in getTestCaseNames testFnNames = list(filter(shouldIncludeMethod, dir(testCaseClass))) File "/home/tim/code/cpython/Lib/unittest/loader.py", line 227, in shouldIncludeMethod testFunc = getattr(testCaseClass, attrname) File "/home/tim/code/django/django/utils/decorators.py", line 172, in __get__ return self.fget(cls) File "/home/tim/code/django/django/test/testcases.py", line 1269, in live_server_url return 'http://%s:%s' % (cls.host, cls.server_thread.port) AttributeError: type object 'AdminSeleniumTestCase' has no attribute 'server_thread' |
|||
msg307062 - (view) | Author: Jonas H. (jonash) * | Date: 2017-11-27 16:12 | |
Ah, the problem isn't that it's running getattr() on test methods, but that it runs getattr() on all methods. Former code: attrname.startswith(prefix) and \ callable(getattr(testCaseClass, attrname)) New code: testFunc = getattr(testCaseClass, attrname) isTestMethod = attrname.startswith(self.testMethodPrefix) and callable(testFunc) This is trivial to fix. @Core devs: Should I revert to original behaviour with the order of the prefix check and the getattr() call, and add a regression test that guarantees this behaviour? |
|||
msg307063 - (view) | Author: Antoine Pitrou (pitrou) * | Date: 2017-11-27 16:13 | |
> @Core devs: Should I revert to original behaviour with the order of the prefix check and the getattr() call, and add a regression test that guarantees this behaviour? Yes, that sounds reasonable to me. |
|||
msg307070 - (view) | Author: Jonas H. (jonash) * | Date: 2017-11-27 17:06 | |
https://github.com/python/cpython/pull/4589 - Add 3.7 What's New entry - Fix regression (thanks Tim for the report) |
|||
msg307156 - (view) | Author: Antoine Pitrou (pitrou) * | Date: 2017-11-28 19:40 | |
New changeset 4d193bcc2560b824389e4d98d9d8b9b34e33dbaf by Antoine Pitrou (Jonas Haag) in branch 'master': bpo-32071: Fix regression and add What's New entry (#4589) https://github.com/python/cpython/commit/4d193bcc2560b824389e4d98d9d8b9b34e33dbaf |
History | |||
---|---|---|---|
Date | User | Action | Args |
2022-04-11 14:58:54 | admin | set | github: 76252 |
2017-11-28 19:40:51 | pitrou | set | messages: + msg307156 |
2017-11-27 17:06:58 | jonash | set | messages: + msg307070 |
2017-11-27 17:04:50 | jonash | set | pull_requests: + pull_request4513 |
2017-11-27 16:13:55 | pitrou | set | messages: + msg307063 |
2017-11-27 16:12:53 | jonash | set | messages: + msg307062 |
2017-11-27 15:55:05 | Tim.Graham | set | nosy:
+ Tim.Graham messages: + msg307056 |
2017-11-27 10:56:40 | jonash | set | messages: + msg307046 |
2017-11-27 10:25:52 | vstinner | set | messages: + msg307041 |
2017-11-25 15:24:31 | pitrou | set | status: open -> closed resolution: fixed messages: + msg306960 stage: patch review -> resolved |
2017-11-25 15:23:54 | pitrou | set | messages: + msg306959 |
2017-11-21 20:55:04 | jonash | set | keywords:
+ patch stage: needs patch -> patch review messages: + msg306687 pull_requests: + pull_request4433 |
2017-11-20 23:43:33 | vstinner | set | nosy:
+ vstinner messages: + msg306601 |
2017-11-20 23:35:13 | pitrou | set | messages: + msg306599 |
2017-11-20 23:32:44 | pitrou | set | messages: + msg306598 |
2017-11-20 23:23:53 | jonash | set | messages: + msg306596 |
2017-11-20 22:30:07 | pitrou | set | messages: + msg306593 |
2017-11-20 20:58:22 | jonash | set | messages: + msg306584 |
2017-11-18 20:24:50 | pitrou | set | versions:
+ Python 3.7 nosy: + pitrou, ezio.melotti, michael.foord, rbcollins messages: + msg306495 stage: needs patch |
2017-11-18 18:18:05 | jonash | set | messages: + msg306492 |
2017-11-18 18:12:40 | jonash | create |