[MERGE] selftest enhancements: --exclude, --randomize, --list-only
John Arbash Meinel
john at arbash-meinel.com
Wed Apr 11 20:52:43 BST 2007
John Arbash Meinel has voted +1 (conditional).
Status is now: Semi-approved
Comment:
I like the general changes, my only feeling is that we don't want
comma-separated for --exclude.
A few comments, otherwise +1.
I don't think we want:
bzr selftest --exclude a,b,c
to mean
bzr selftest --exclude "a|b|c"
It is (after all) a regex, so we should treat it as such. They probably
need to escape "*" and "?" and any other thing they are passing in. And
"." isn't going to act like they expect. (blackbox.test_foo, will
generally work, but not because of why they think it does).
If we decide to leave ',' in, then I would do:
if exclude is None:
exclude_pattern = None
else:
exclude_pattern = exclude.replace(',', '|')
^- space here
(There is no need to check for ',' before you replace it. If there
aren't any commas, then .replace won't do anything)
Also, I feel like we should treat 'exclude' in the same way we treat the
include regex. So if we support commas in 'exclude' we should put them
in the standard parsing, too.
And it might be nicer if we support multiple -x options. Once JML's
patch lands to provide a ListOption, it should be easy enough to add.
Then instead of using ',' we can have people do:
-x one -x two -x three
Oh, and you don't have any tests that '--exclude x,y,z' does what you
think it does. (I didn't see any tests for --exclude that included
commas)
+def filter_suite_by_re(suite, pattern, exclude_pattern=None,
+ random_order=False):
should be:
+def filter_suite_by_re(suite, pattern, exclude_pattern=None,
+ random_order=False):
And it should have a doc string.
Same for:
+def sort_suite_by_re(suite, pattern, append_rest=True,
exclude_pattern=None,
+ random_order=False):
Especially since I don't understand what "append_rest" means. So we
should define it in the docstring.
+ (header,body,footer) = self._parse_test_list(out.splitlines())
+ self.assertContainsRe(footer[0], 'Listed \d+ tests in')
+ summary_re = re.compile('(\d+)')
^- Should be r'Listed \d+' so python doesn't interpret the \d. (It
succeeds by accident because there happens to be no \d escape)
But since you are checking the number in the next step, why not just do:
num_tests = len(body)
self.assertContainsRe(footer[0], 'Listed %s tests in' % num_tests)
For details, see:
http://bundlebuggy.aaronbentley.com/request/%3C461C4C37.80200%40internode.on.net%3E
More information about the bazaar
mailing list