[MERGE] Adding --starting-with <test_id> option to selftest make it run 5x faster
John Arbash Meinel
john at arbash-meinel.com
Wed Apr 30 22:19:08 BST 2008
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Vincent Ladeuil wrote:
|>>>>> "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
|
|
sounds good to me.
John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iEYEARECAAYFAkgY4kwACgkQJdeBCYSNAAPznQCfU43H4BZOqlO5f8TjLjZnCs0Q
Gt0An0rDmm1mTZgpT33B6JpbXSUGoz7P
=oG3I
-----END PGP SIGNATURE-----
More information about the bazaar
mailing list