[MERGE] selftest enhancements: --exclude, --randomize, --list-only

[John Arbash Meinel]

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