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

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".

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.

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

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.

> > 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?

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.

If it helps, think of TestLoader as collecting tests, rather than simply loading Python modules.

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).

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

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

The enhancement is now pushed to git master.  Thank you Jonas for contributing this!

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?

Sure!

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'

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?

> @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.

https://github.com/python/cpython/pull/4589

- Add 3.7 What's New entry
- Fix regression (thanks Tim for the report)

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