launchpad-reviewers team mailing list archive

[Brad Crittenden <bac@xxxxxxxxxxxxx>]

Brad Crittenden has proposed merging lp:~bac/launchpad/repeats into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1004625 in Launchpad itself: "bin/test --load-list collapses testcases with the same name"
  https://bugs.launchpad.net/launchpad/+bug/1004625

For more details, see:
https://code.launchpad.net/~bac/launchpad/repeats/+merge/107651

= Summary =

The change to ensure --load-list ran tests in the same order as
presented in the test list file incorrectly assumed that testcase ids
were unique and added them to a hash.  This approach broke for the
doctests that get registered manually and run multiple times.  (See
the 'special' set in lp/answers/tests/test_doc.py for examples.)

== Proposed fix ==

Rather than assume a 1:1 mapping between test name and the test cases
discovered by zope.testing, assume it is 1:n.

== Pre-implementation notes ==

Productive pair programming with Benji to help find the culprit.

== Implementation details ==

As above.

== Tests ==

bin/test --vv bin/test -vv lp.services.testing.tests.test_customresult TestFilterTests

== Demo and Q/A ==

It can be demoed on the command-line.  First create a file with one
repeated testcase:

% cat testlist
lib/lp/answers/doc/faqcollection.txt

Then run using --load-list:
% bin/test --load-list testlist
Running lp.testing.layers.DatabaseFunctionalLayer tests:
  Set up lp.testing.layers.BaseLayer in 0.088 seconds.
  Set up lp.testing.layers.DatabaseLayer in 1.627 seconds.
  Set up lp.testing.layers.FunctionalLayer in 5.041 seconds.
  Set up lp.testing.layers.DatabaseFunctionalLayer in 0.000 seconds.
  Ran 3 tests with 0 failures and 0 errors in 1.026 seconds.
Tearing down left over layers:
  Tear down lp.testing.layers.DatabaseFunctionalLayer in 0.000 seconds.
  Tear down lp.testing.layers.DatabaseLayer in 0.468 seconds.
  Tear down lp.testing.layers.FunctionalLayer ... not supported
  Tear down lp.testing.layers.BaseLayer in 0.001 seconds.

You'll note the test was run three times.

If you repeat the same exercise in trunk it'll just show one test,
which is wrong.


= Launchpad lint =

Checking for conflicts and issues in changed files.

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

./lib/lp/services/testing/tests/test_customresult.py
      31: E301 expected 1 blank line, found 0
      33: E301 expected 1 blank line, found 0
      35: E301 expected 1 blank line, found 0
      37: E301 expected 1 blank line, found 0
-- 
https://code.launchpad.net/~bac/launchpad/repeats/+merge/107651
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~bac/launchpad/repeats into lp:launchpad.

=== modified file 'lib/lp/services/testing/customresult.py'
--- lib/lp/services/testing/customresult.py	2012-05-01 20:49:04 +0000
+++ lib/lp/services/testing/customresult.py	2012-05-28 15:08:20 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Support code for using a custom test result in test.py."""
@@ -53,15 +53,20 @@
         # Read the tests, filtering out any blank lines.
         tests = filter(None, [line.strip() for line in open(list_name, 'rb')])
         test_lookup = {}
+        # Multiple unique testcases can be represented by a single id and they
+        # must be tracked separately.
         for layer_name, suite in tests_by_layer_name.iteritems():
             for testcase in suite:
-                test_lookup[testcase.id()] = (testcase, layer_name)
+                layer, testlist = test_lookup.setdefault(
+                    testcase.id(), (layer_name, []))
+                testlist.append(testcase)
 
         result = {}
         for testname in tests:
-            testcase, layer = test_lookup.get(testname, (None, None))
+            layer, testcases = test_lookup.get(testname, (None, None))
             if layer is not None:
                 suite = result.setdefault(layer, TestSuite())
-                suite.addTest(testcase)
+                for testcase in testcases:
+                    suite.addTest(testcase)
         return result
     return do_filter

=== added file 'lib/lp/services/testing/tests/test_customresult.py'
--- lib/lp/services/testing/tests/test_customresult.py	1970-01-01 00:00:00 +0000
+++ lib/lp/services/testing/tests/test_customresult.py	2012-05-28 15:08:20 +0000
@@ -0,0 +1,109 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Parallel test glue."""
+
+__metaclass__ = type
+
+import string
+import tempfile
+
+from testtools import (
+    TestCase,
+    )
+
+from lp.services.testing.customresult import (
+    filter_tests,
+    )
+from lp.testing.layers import BaseLayer
+
+import unittest
+
+
+NEWLINE = '\n'
+
+
+class FakeTestCase(unittest.TestCase):
+    """A minimal TestCase that can be instantiated."""
+    def __init__(self, name, *args, **kwargs):
+        super(FakeTestCase, self).__init__(*args, **kwargs)
+        self.name = name
+    def id(self):
+        return self.name
+    def runTest(self):
+        pass
+    def __str__(self):
+        return self.id()
+    def __repr__(self):
+        return str(self)
+
+
+class TestFilterTests(TestCase):
+
+    layer = BaseLayer
+
+    def setUp(self):
+        super(TestFilterTests, self).setUp()
+
+    def writeFile(self, fd, contents):
+        for line in contents:
+            fd.write(line + NEWLINE)
+        fd.flush()
+
+    def test_ordering(self):
+        # Tests should be returned in the order seen in the testfile.
+        layername = 'layer-1'
+        testnames = ['d', 'c', 'a']
+        suite = unittest.suite.TestSuite()
+        for letter in string.lowercase:
+            suite.addTest(FakeTestCase(letter))
+        with tempfile.NamedTemporaryFile() as fd:
+            self.writeFile(fd, testnames)
+            do_filter = filter_tests(fd.name)
+            results = do_filter({layername: suite})
+        self.assertEqual(1, len(results))
+        self.assertTrue(layername in results)
+        suite = results[layername]
+        self.assertEqual(testnames, [t.id() for t in suite])
+
+    def test_layer_separation(self):
+        # Tests must be kept in their layer.
+        suite1 = unittest.suite.TestSuite()
+        suite2 = unittest.suite.TestSuite()
+        # Create one layer with the 'a'..'m'.
+        for letter in string.lowercase[:13]:
+            suite1.addTest(FakeTestCase(letter))
+        # And another layer with 'n'..'z'.
+        for letter in string.lowercase[13:]:
+            suite2.addTest(FakeTestCase(letter))
+        testnames = ['a', 'b', 'c', 'z', 'y', 'x']
+        with tempfile.NamedTemporaryFile() as fd:
+            self.writeFile(fd, testnames)
+            do_filter = filter_tests(fd.name)
+            results = do_filter({'layer1': suite1,
+                                 'layer2': suite2})
+        self.assertEqual(2, len(results))
+        self.assertEqual(['layer1', 'layer2'], sorted(results.keys()))
+        self.assertEqual(['a', 'b', 'c'], [t.id() for t in results['layer1']])
+        self.assertEqual(['z', 'y', 'x'], [t.id() for t in results['layer2']])
+
+    def test_repeated_names(self):
+        # Some doctests are run repeatedly with different scenarios.  They
+        # have the same name but different testcases.  Those tests must not be
+        # collapsed and lost.
+        layername = 'layer-1'
+        testnames = ['1', '2', '3']
+        suite = unittest.suite.TestSuite()
+        for t in testnames:
+            # Each test will be repeated equal to the number represented.
+            for i in range(int(t)):
+                suite.addTest(FakeTestCase(t))
+        with tempfile.NamedTemporaryFile() as fd:
+            self.writeFile(fd, testnames)
+            do_filter = filter_tests(fd.name)
+            results = do_filter({layername: suite})
+        self.assertEqual(1, len(results))
+        self.assertTrue(layername in results)
+        suite = results[layername]
+        expected = ['1', '2', '2', '3', '3', '3']
+        self.assertEqual(expected, [t.id() for t in suite])