[MERGE] Adding --starting-with <test_id> option to selftest make it run 5x faster

[Vincent Ladeuil]

>>>>> "john" == John Arbash Meinel <john at arbash-meinel.com> writes:

<snip/>

    john> +    def interesting_module(name):
    john> +        if starting_with is not None:
    john> +            return name.startswith(starting_with)
    john> +        elif keep_only is not None:
    john> +            return id_filter.refers_to(name)
    john> +        else:
    john> +            return True
    john> +

    john> This seems like it would be better said by creating a custom
    john> function in the first if/else block. So it would be:

    john> if starting_with:
    john>   def interesting(name):
    john>     return starting_with(name)

    john> That also means the 'if' test isn't run for every test
    john> being checked.

True.

Done.

    john> It also seems like Robert's recent condition test patch is
    john> somewhat related/conflicting. 

Not really.

    john> His is a bit different because it is a splitting
    john> filter,

Exactly.

    john> while filter-at-loading time intentionally doesn't want
    john> to determine the full names unless they are likely to
    john> be in the included set.

Yes, and I'm not interested in the excluded tests, so I don't need to
split.

    john> ...

    john> -        for id in not_found:
    john> -            bzrlib.trace.warning('"%s" not found in the test
    john> suite', id)
    john> +        if starting_with is not None:
    john> +            # No need to annoy the tester with tests not found
    john> since when both
    john> +            # options are used *there will be* tests excluded
    john> from the list.
    john> +            pass
    john> +        else:
    john> +            for id in not_found:
    john> +                bzrlib.trace.warning('"%s" not found in the test
    john> suite', id)

    john> ^- This seems like more of an odd interaction between "keep-only"
    john> and "startswith". Is there really a reason to support both?

Yes. Mentioned in the mail:

    vila> It addresses the main limitations of the --load-list option (you
    vila> *need* a list before using it and it tends to bitrot quickly)
    vila> while still being compatible with it (one use case being a list
    vila> with all failing tests).

One can build a list of failing tests for, say, windows. And
there is a lot of tests failing, so while working on fixing some,
he want to only run a subset.

    john> IMO they could be exclusive options.

When I start coding that option I thought that --starting-with
and --load-list will be exclusive, but then, I realized that that
was not necessary and that one day, someone will ask for
it. Since that didn't make the code more complex, I keep them
compatible.

I've updated the comment to make that clearer:

+        if starting_with is not None:
+            # The tester has used both keep_only and starting_with, so he is
+            # already aware that some tests are excluded from the list, there
+            # is no need to tell him which.
+            pass
+        else:
+            # Some tests mentioned in the list are not in the test suite. The
+            # list may be out of date, report to the tester.
+            for id in not_found:
+                bzrlib.trace.warning('"%s" not found in the test suite', id)


    john> === modified file 'bzrlib/tests/blackbox/test_selftest.py'
    john> --- bzrlib/tests/blackbox/test_selftest.py      2008-01-21
    john> 10:51:02 +0000
    john> +++ bzrlib/tests/blackbox/test_selftest.py      2008-04-23
    john> 19:43:13 +0000
    john> @@ -575,3 +575,12 @@
    john>      def test_load_unknown(self):
    john>          out, err = self.run_bzr('selftest --load-list I_do_not_exist ',
    john>                                  retcode=3)
    john> +
    john> +
    john> +class TestSelftestStartingWith(TestCase):
    john> +
    john> +    def test_starting_with(self):
    john> +        out, err = self.run_bzr(
    john> +            ['selftest', '--starting-with', self.id(), '--list'])
    john> +        self.assertContainsRe(out, "Listed 1 test in")
    john> +

    john> ^- This seems a bit heavy for testing --starting-with. I don't
    john> know how slow it is, though, if it isn't loading a lot of the
    john> suite.

./bzr selftest --starting-with bzrlib.tests.blackbox.test_selftest.TestSelftestStartingWith -v

...est.TestSelftestStartingWith.test_starting_with   OK                 232ms

Hehe, it is fast :)

Oh wait ! The test is with --list.

./bzr selftest --starting-with bzrlib.tests.blackbox.test_selftest.TestSelftestStartingWith -v --list

running 1 tests...
Listing tests only ...

bzrlib.tests.blackbox.test_selftest.TestSelftestStartingWith.test_starting_with

----------------------------------------------------------------------
Listed 1 test in 0.000s

OK
tests passed

Damn, too fast :)

Ok, worst of ten runs:

Listed 1 test in 0.068s


    john> It might be more interesting to have 2 functions

    john> def test_starting_with(self):
    john>   ...

    john> def test_starting_with_a_longer_name(self):

    john> At the least, I think we should test the name of the test that
    john> shows up, rather than just 1 showing up. I can respect that there
    john> is some difficulty because of screen width, etc, though.

Done. Since the test use --list, the whole name is shown, so I added:

        self.assertContainsRe(out, self.id())


    john> +    def test_condition_id_startswith(self):
    john> +        klass = 'bzrlib.tests.test_selftest.TestSelftestFiltering.'
    john> +        start = klass + 'test_condition_id_starts'
    john> +        test_names = [klass + 'test_condition_id_startswith']
    john> +        filtered_suite = filter_suite_by_condition(
    john> +            self.suite, tests.condition_id_startswith(start))
    john> +        my_pattern =
    john> TestSelftestFiltering.*test_condition_id_startswith'
    john> +        re_filtered = filter_suite_by_re(self.suite, my_pattern)
    john> +        self.assertEqual(_test_ids(re_filtered),
    john> _test_ids(filtered_suite))
    john> +

    john> ^- this certainly seems like something that would be changed with
    john> "filter_suite_by_condition" from Robert.

It *uses* filter_suite_by_condition and tests its result against
filter_suite_by_re so that two different code paths are used,
like the other similar tests in that class.

Thanks for the review,

     Vincent