Merge lp:~bac/launchpad/ordered-load-list into lp:launchpad

[Brad Crittenden]

Change bin/test --load-list to preserve the order presented in the original test list, at least within layers.

= Summary =

"bin/test --load-list <fn>" loads the tests but then does not respect
their ordering due to the use of set() and the way the tests are
assigned to layers.

The approach presently taken makes sense from an efficiency standpoint
but at the cost of ordering. One of the great uses of --load-list is
to repeat a test run to find test isolation problems and that
utility is defeated if the ordering is not maintained.

== Proposed fix ==

Change 'filter_tests' to maintain order within a layer.

The order in which the layers are run may change, though that has not
been seen in practice. Should the layer ordering change, the tests
are still run in order per layer, which is probably sufficient for
isolating test failures.

== Pre-implementation notes ==

Chat with Gary.

== Implementation details ==

As above.

== Tests ==

bin/test --load-list in.txt --list-tests > out.txt

Then verify in.txt and out.txt are the same modulo the "Listing layer"
messages and any layer rearrangement issues as described above.

== Demo and Q/A ==

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/services/testing/customresult.py

[Gary Poster (gary)]

Cool, thank you. This will be a big help.

A brief summary of our IRC conversation follows. All of these are negotiable.

 - I don't think we should raise an exception if LP no longer has a test in the test list. The previous code did not have that behavior.

- ordered_layers isn't doing anything, and we should delete it.

- The suites_by_layer data structure is unnecessarily inefficient for the question you want to answer. If instead you build a map of test name to (layer, test), you can replace find_layers with a single hash lookup.

- We should include a comment as to why the code is maintaining order, so that future code navigators will have a clue as to intent.

[Brad Crittenden (bac)]

Thanks for the review Gary. I incorporated all of your suggestions.

[Gary Poster (gary)]

review: Approve

=== modified file 'lib/lp/services/testing/customresult.py'
--- lib/lp/services/testing/customresult.py	2012-01-01 02:58:52 +0000
+++ lib/lp/services/testing/customresult.py	2012-05-01 20:53:23 +0000
@@ -11,7 +11,6 @@
 
 from unittest import TestSuite
 
-from testtools import iterate_tests
 from zope.testing.testrunner import find
 
 
@@ -25,8 +24,10 @@
         structure.
     """
     real_find_tests = find.find_tests
+
     def find_tests(*args):
         return hook(real_find_tests(*args))
+
     find.find_tests = find_tests
 
 
@@ -37,16 +38,30 @@
         test ids, as generated by `list_tests`.
     :return: A callable that takes a result of `testrunner.find_tests` and
         returns only those tests with ids in the file 'list_name'.
+
+    Note great care is taken to preserve the ordering of the original test
+    cases, which is quite important if trying to figure out test isolation
+    bugs.  The original ordering is maintained within layers but since the
+    results are returned as a dictionary the caller may shuffle the way the
+    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.
+
+    Should a test be listed, but not present in any of the suites, it is
+    silently ignored.
     """
     def do_filter(tests_by_layer_name):
-        tests = sorted(set(line.strip() for line in open(list_name, 'rb')))
+        # Read the tests, filtering out any blank lines.
+        tests = filter(None, [line.strip() for line in open(list_name, 'rb')])
+        test_lookup = {}
+        for layer_name, suite in tests_by_layer_name.iteritems():
+            for testcase in suite:
+                test_lookup[testcase.id()] = (testcase, layer_name)
+
         result = {}
-        for layer_name, suite in tests_by_layer_name.iteritems():
-            new_suite = TestSuite()
-            for test in iterate_tests(suite):
-                if test.id() in tests:
-                    new_suite.addTest(test)
-            if new_suite.countTestCases():
-                result[layer_name] = new_suite
+        for testname in tests:
+            testcase, layer = test_lookup.get(testname, (None, None))
+            if layer is not None:
+                suite = result.setdefault(layer, TestSuite())
+                suite.addTest(testcase)
         return result
     return do_filter