bpo-30540: regrtest: add --matchfile option by vstinner · Pull Request #1909 · python/cpython (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation25 Commits1 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
- Add a new option taking a filename to get a list of test names to
filter tests. - support.match_tests becomes a list.
- Modify run_unittest() to accept to match the whole test identifier,
not just a part of a test identifier.
For example, the following command only runs test_default_timeout()
of the BarrierTests class of test_threading:
$ ./python -m test -v test_threading -m test.test_threading.BarrierTests.test_default_timeout
| @@ -189,8 +189,11 @@ def _create_parser(): |
|---|
| help='single step through a set of tests.' + |
| more_details) |
| group.add_argument('-m', '--match', metavar='PAT', |
| dest='match_tests', |
| dest='match_tests', action='append', |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Support of multiple -m options -- I like this!
| help='match test cases and methods with glob pattern PAT') |
|---|
| group.add_argument('--matchfile', metavar='FILENAME', |
| dest='match_filename', |
| help='similar to --match but get pattersn from a text file, one pattern per line') |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"patterns"
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
| @@ -159,7 +159,7 @@ def test_match(self): |
|---|
| for opt in '-m', '--match': |
| with self.subTest(opt=opt): |
| ns = libregrtest._parse_args([opt, 'pattern']) |
| self.assertEqual(ns.match_tests, 'pattern') |
| self.assertEqual(ns.match_tests, ['pattern']) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add tests for multiple -m and for --matchfile.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| @@ -350,5 +353,10 @@ def _parse_args(args, **kwargs): |
|---|
| print("WARNING: Disable --verbose3 because it's incompatible with " |
| "--huntrleaks: see http://bugs.python.org/issue27103", |
| file=sys.stderr) |
| if ns.match_filename: |
| ns.match_tests = [] |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if specify both --match and --matchfile?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--matchfile overrides --match.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be defined as extending match_tests.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, fixed
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| regex = re.compile("^(test[^ ]+).*ok$", flags=re.MULTILINE) |
|---|
| return [match.group(1) for match in regex.finditer(output)] |
| def test_matchfile(self): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, it would be enough to add a test only in ParseArgsTestCase! But more comprehensive test is good.
| with open(filename, "w") as fp: |
|---|
| for name in subset: |
| print(name, file=fp) |
| fp.flush() |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is redundant.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the flush.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
| pass |
|---|
| """) |
| all_methods = ['test_method1', 'test_method2', |
| 'test_method3', 'test_method4'] |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The closing bracket doesn't conform PEP 8.
The closing brace/bracket/parenthesis on multi-line constructs may either line up under the first non-whitespace character of the last line of list, ... or it may be lined up under the first character of the line that starts the multi-line construct...
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this rule. The flake8 tool doesn't complain.
| # only match the method name |
|---|
| 'test_method1', |
| # match the full identifier |
| '%s.Tests.test_method3' % testname] |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The closing bracket doesn't conform PEP 8.
See also "When to use trailing commas".
| methods = self.parse_methods(output) |
|---|
| subset = ['test_method1', 'test_method3'] |
| self.assertEqual(methods, subset) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the match file is empty?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, nothing interesting. This doesn't deserve special testing.
| @@ -350,5 +353,10 @@ def _parse_args(args, **kwargs): |
|---|
| print("WARNING: Disable --verbose3 because it's incompatible with " |
| "--huntrleaks: see http://bugs.python.org/issue27103", |
| file=sys.stderr) |
| if ns.match_filename: |
| ns.match_tests = [] |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be defined as extending match_tests.
| @@ -117,6 +117,10 @@ |
|---|
| To enable all resources except one, use '-uall,-'. For |
| example, to run all the tests except for the gui tests, give the |
| option '-uall,-gui'. |
| --matchfile filters the test methods using a text file. Each line must be a |
| test method (ex: test_method) or a test identifier (ex: |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or class, or file.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
- Add a new option taking a filename to get a list of test names to filter tests.
- support.match_tests becomes a list.
- Modify run_unittest() to accept to match the whole test identifier, not just a part of a test identifier.
For example, the following command only runs test_default_timeout() of the BarrierTests class of test_threading:
$ ./python -m test -v test_threading -m test.test_threading.BarrierTests.test_default_timeout
Remove also some empty lines from test_regrtest.py to make flake8 tool happy.
I fixed a bug for relative filenames in --matchfile (use support.SAVEDCWD). All comments should be addressed, except of the comment about PEP 8 but flake8 doesn't complain.
vstinner added a commit that referenced this pull request
