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

[Ian Clatworthy]

John Arbash Meinel wrote:
> 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.
>
John,

Firstly, thanks for the review - first class as always ...
>
>
> 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).
>
It is a regex as you say so the comma is arguably unnecessary feature 
creep. Having say that, it was added because escaping on Windows is a 
right royal pain in the *rse, even when it is possible. I have a nagging 
feeling that escaping "|" isn't supported on some older DOS shells? Now 
that I'd said that, I hope some Windows users out there can tell me I'm 
mistaken.

With no built-in globbing, * and ? don't require escaping on Windows. On 
other platforms, they don't either unless the current directory happens 
to have files matching the test patterns. FWIW, my test patterns tend to 
be the test case names, not the module names - I don't know how typical 
that is for others though.
> 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)
>
Agreed.
>
> 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.
>
Yes. It wasn't an issue because "space" works fine on the include regexs.
> 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
>
Agreed. Aaron had some feedback re list options (see 
https://bugs.launchpad.net/bzr/+bug/105366) that reviewers of JMLs patch 
need to consider. I do like list options and I'd like us to support them 
one way or another.
>
> 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)
>
Oversight sorry. I'll fix this.
>
>
> +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.
>
Yes.
>
> +        (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)
>
Shall do. Nice.
>
> For details, see: 
> http://bundlebuggy.aaronbentley.com/request/%3C461C4C37.80200%40internode.on.net%3E 
>
>
Ian C.