bpo-32071: Add unittest -k option #4496
Conversation
|
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA). Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. Thanks again to your contribution and we look forward to looking at it! |
There was a problem hiding this comment.
Thank you for posting this PR. In addition to the review comments I posted, there are two points to address:
- sign the contributor's agreement, if you haven't already done so
- add documentation for the new command-line option and the
TestLoaderaddition; the docs areDoc/library/unittest.rst
| @@ -225,8 +241,7 @@ def _do_discovery(self, argv, Loader=None): | |||
| self._initArgParsers() | |||
| self._discovery_parser.parse_args(argv, self) | |||
|
|
|||
| loader = self.testLoader if Loader is None else Loader() | |||
| self.test = loader.discover(self.start, self.pattern, self.top) | |||
| self.createTests(from_discovery=True, Loader=Loader) | |||
There was a problem hiding this comment.
I'm curious, why was this change necessary?
There was a problem hiding this comment.
This refactoring was necessary because I didn't want to repeat the code that sets loader.testNamePatterns from the CLI options. That code has to be executed BEFORE loader.discover. Besides, I believe it makes sense to have a single place where self.test is set.
| @@ -409,6 +409,15 @@ def testParseArgsAbsolutePathsThatCannotBeConverted(self): | |||
| # for invalid filenames should we raise a useful error rather than | |||
| # leaving the current error message (import of filename fails) in place? | |||
|
|
|||
| def testParseArgsSelectedTestNames(self): | |||
There was a problem hiding this comment.
Thank you for adding these tests. Do you think you could also create a small functional test? There is an example of such a test (using subprocess) in Test_TextTestRunner.test_warnings.
There was a problem hiding this comment.
I added a functional test but I'm not sure if that's the right place to put it.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
|
There really should be both an issue opened and a NEWS item for this. These should not be skipped. Otherwise, I have to read the diff to infer what |
|
I fixed the title to refer to the issue on the bug tracker. |
jonashaag
force-pushed
the
unittest-select2
branch
from
98617e4 to
0dbf322
Compare
|
I have made the requested changes; please review again. |
|
Thanks for making the requested changes! @pitrou: please review the changes made to this pull request. |
|
I'm not sure what the Sphinx error means. |
Doc/library/unittest.rst
Outdated
| List of Unix shell-style wildcard test name patterns that test methods | ||
| have to match to be included in test suites (see ``-v`` option). | ||
|
|
||
| If this attribute is not `None` (the default), all test methods to be |
There was a problem hiding this comment.
The sphinx build says:
[2] library/unittest.rst:1772: default role used
It points to line 1772 of unittest.rst.
I think it's because single backticks was used around None.
Try using double backticks instead.
Doc/library/unittest.rst
Outdated
| @@ -229,6 +245,9 @@ Command-line options | |||
| .. versionadded:: 3.5 | |||
| The command-line option ``--locals``. | |||
|
|
|||
| .. versionadded:: 3.7 | |||
| The command-line option ``-k``. | |||
There was a problem hiding this comment.
Add one more space before "The command-line .. ".
Doc/library/unittest.rst
Outdated
| imported by the test loader. | ||
|
|
||
| For example, ``-k foo`` matches ``foo_tests.SomeTest.test_something``, | ||
| ``bar_tests.SomeTest.test_foo``, but not `bar_tests.FooTest.test_something``. |
There was a problem hiding this comment.
Mismatched number of backticks in bar_tests.FooTest.test_something.
|
Not sure why CLA not signed label is still there; I signed it on 2017-11-21. |
Hmm... I've tried to reach to some people who might have a clue about that :-) |
and
@jonashaag: If the bot adds again "not signed", it means that you forgot to fill your GitHub account login in your bugs.python.org profile. |
|
Read also the @the-knights-who-say-ni message which explains that ;-) #4496 (comment) |
|
@vstinner It's there, I added it on 2017-11-21.
|
|
Screenshots of signed CLA as received from Adobe sign (?) Is there a manual check by a human involved in the process? If so, maybe the person simply hasn't found the time to process it.
|
|
On bugs.python.org, I still see "Contributor Form Received: No". Yeah, I think that a human process is required somewhere. |
|
@vstinner bugs account has been updated. For whatever reason, the email notification never came through. |
|
Ok, the CLA signed label is now here :-) Thank you Ewa. |
|
The PR now looks good to me, I'm going to merge. |
Due to change [1] in python 3.7 one of ryu's unit tests was failing with this version of interpreter. It was like that because of missing __qualname__ attribute in functools.partial object. This patch fixes it by adding such attribute if it's not set already. [1] python/cpython#4496
https://bugs.python.org/issue32071