launchpad-reviewers team mailing list archive

[Francesco Banconi <francesco.banconi@xxxxxxxxxxxxx>]

Francesco Banconi has proposed merging lp:~frankban/launchpad/tests-randomization into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~frankban/launchpad/tests-randomization/+merge/109103

= Summary =

bin/test --shuffle --shuffle-seed [seed] can be used to reproduce the same random ordering of test cases in multiple calls. However, this ordering changes if you use --load-list rather than -t.

== Proposed fix ==

--load-list, if shuffle is requested, should generate the initial list of test cases to be shuffled ordering them by id.

== Implementation details ==

The added optional argument `reorder_tests` of lp.services.testing.customresult.filter_tests` can be used to avoid preserving the ordering of the original test cases (as they appear in the given file).
bin/test now calls `filter_tests` passing reorder_tests=True if --shuffle is used.

== Tests ==

bin/test -cvvt lp.services.testing.tests

== Demo and Q/A ==

1. Save a file mytests with this content:

lp.soyuz.tests.test_packagecopyjob.PlainPackageCopyJobTests.test_copying_closes_bugs
lp.bugs.scripts.tests.test_bugimport.ImportBugTestCase.test_import_bug
lp.scripts.tests.test_garbo.TestGarbo.test_OAuthNoncePruner
lp.bugs.scripts.tests.test_bugnotification.TestEmailNotificationsAttachments.test_added_seen

2. check --load-list still preserves test cases ordering:

$ bin/test -cvv --load-list mytests 
...
  Running:
 lp.soyuz.tests.test_packagecopyjob.PlainPackageCopyJobTests.test_copying_closes_bugs
 lp.bugs.scripts.tests.test_bugimport.ImportBugTestCase.test_import_bug
 lp.scripts.tests.test_garbo.TestGarbo.test_OAuthNoncePruner
 lp.bugs.scripts.tests.test_bugnotification.TestEmailNotificationsAttachments.test_added_seen
  Ran 4 tests with 0 failures and 0 errors in 4.595 seconds.
...

3. shuffle tests, and copy the used seed:

$ bin/test -cvv --load-list mytests --shuffle
...
  Running:
 lp.scripts.tests.test_garbo.TestGarbo.test_OAuthNoncePruner
 lp.bugs.scripts.tests.test_bugnotification.TestEmailNotificationsAttachments.test_added_seen
 lp.bugs.scripts.tests.test_bugimport.ImportBugTestCase.test_import_bug
 lp.soyuz.tests.test_packagecopyjob.PlainPackageCopyJobTests.test_copying_closes_bugs
  Ran 4 tests with 0 failures and 0 errors in 4.520 seconds.
...
Tests were shuffled using seed number 342800994878.

4. Run the same tests using -t and the same shuffle seed:

$ bin/test -cvv -t lp.soyuz.tests.test_packagecopyjob.PlainPackageCopyJobTests.test_copying_closes_bugs -t lp.bugs.scripts.tests.test_bugimport.ImportBugTestCase.test_import_bug -t lp.scripts.tests.test_garbo.TestGarbo.test_OAuthNoncePruner -t lp.bugs.scripts.tests.test_bugnotification.TestEmailNotificationsAttachments.test_added_seen --shuffle --shuffle-seed 342800994878

The test ordering should be the same as the previous call.

NO QA

== lint ==

Linting changed files:
  buildout-templates/bin/test.in
  lib/lp/services/testing/customresult.py
  lib/lp/services/testing/tests/test_customresult.py
-- 
https://code.launchpad.net/~frankban/launchpad/tests-randomization/+merge/109103
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~frankban/launchpad/tests-randomization into lp:launchpad.

=== modified file 'buildout-templates/bin/test.in'
--- buildout-templates/bin/test.in	2011-12-30 06:47:17 +0000
+++ buildout-templates/bin/test.in	2012-06-07 10:59:20 +0000
@@ -220,7 +220,7 @@
         sys.exit(main(sys.argv))
 
     def load_list(option, opt_str, list_name, parser):
-        patch_find_tests(filter_tests(list_name))
+        patch_find_tests(filter_tests(list_name, '--shuffle' in sys.argv))
     options.parser.add_option(
         '--load-list', type=str, action='callback', callback=load_list)
     options.parser.add_option(

=== modified file 'lib/lp/services/testing/customresult.py'
--- lib/lp/services/testing/customresult.py	2012-06-01 15:00:25 +0000
+++ lib/lp/services/testing/customresult.py	2012-06-07 10:59:20 +0000
@@ -31,11 +31,13 @@
     find.find_tests = find_tests
 
 
-def filter_tests(list_name):
+def filter_tests(list_name, reorder_tests=False):
     """Create a hook for `patch_find_tests` that filters tests based on id.
 
     :param list_name: A filename that contains a newline-separated list of
         test ids, as generated by `list_tests`.
+    :param reorder_tests: if True, the tests contained in `list_name`
+        are reordered by id. Default is False: the ordering is preserved.
     :return: A callable that takes a result of `testrunner.find_tests` and
         returns only those tests with ids in the file 'list_name'.
 
@@ -46,12 +48,20 @@
     layers are run and there is nothing to be done about that here.  In
     practice the layers are seen to be run in the same order.
 
+    However, test cases can still be reordered if `reorder_tests` is set to
+    True: this is useful when tests are shuffled and the test shuffler is
+    initialized using a particoular value. This way the same seed produces
+    the same random ordering, regardless of whether the tests are filtered
+    using -t or --load-list.
+
     Should a test be listed, but not present in any of the suites, it is
     silently ignored.
     """
     def do_filter(tests_by_layer_name):
         # Read the tests, filtering out any blank lines.
         tests = filter(None, [line.strip() for line in open(list_name, 'rb')])
+        if reorder_tests:
+            tests.sort()
         test_lookup = {}
         # Multiple unique testcases can be represented by a single id and they
         # must be tracked separately.

=== modified file 'lib/lp/services/testing/tests/test_customresult.py'
--- lib/lp/services/testing/tests/test_customresult.py	2012-06-01 15:26:17 +0000
+++ lib/lp/services/testing/tests/test_customresult.py	2012-06-07 10:59:20 +0000
@@ -40,20 +40,23 @@
         f.flush()
 
     @staticmethod
+    def make_suite(testnames=string.lowercase):
+        """Make a suite containing `testnames` (default: 'a'..'z')."""
+        suite = unittest.TestSuite()
+        for testname in testnames:
+            suite.addTest(FakeTestCase(testname))
+        return suite
+
+    @staticmethod
     def make_suites():
         """Make two suites.
 
         The first has 'a'..'m' and the second 'n'..'z'.
         """
-        suite_am = unittest.TestSuite()
-        suite_nz = unittest.TestSuite()
-        # Create one layer with the 'a'..'m'.
-        for letter in string.lowercase[:13]:
-            suite_am.addTest(FakeTestCase(letter))
-        # And another layer with 'n'..'z'.
-        for letter in string.lowercase[13:]:
-            suite_nz.addTest(FakeTestCase(letter))
-        return suite_am, suite_nz
+        return (
+            TestFilterTests.make_suite(string.lowercase[:13]),
+            TestFilterTests.make_suite(string.lowercase[13:]),
+            )
 
     @staticmethod
     def make_repeated_suite(testnames):
@@ -68,9 +71,7 @@
         # Tests should be returned in the order seen in the testfile.
         layername = 'layer-1'
         testnames = ['d', 'c', 'a']
-        suite = unittest.TestSuite()
-        for letter in string.lowercase:
-            suite.addTest(FakeTestCase(letter))
+        suite = self.make_suite()
         with tempfile.NamedTemporaryFile() as f:
             self.writeFile(f, testnames)
             do_filter = filter_tests(f.name)
@@ -80,6 +81,18 @@
         suite = results[layername]
         self.assertEqual(testnames, [t.id() for t in suite])
 
+    def test_reorder_tests(self):
+        # Tests can optionally be ordered by id.
+        layername = 'layer-1'
+        testnames = ['d', 'c', 'a']
+        suite = self.make_suite()
+        with tempfile.NamedTemporaryFile() as f:
+            self.writeFile(f, testnames)
+            do_filter = filter_tests(f.name, reorder_tests=True)
+            results = do_filter({layername: suite})
+        suite = results[layername]
+        self.assertEqual(sorted(testnames), [t.id() for t in suite])
+
     def test_layer_separation(self):
         # Tests must be kept in their layer.
         suite1, suite2 = self.make_suites()