Merge lp:~bac/launchpad/repeats into lp:launchpad

[Brad Crittenden]

Ensure --load-list properly runs multiple instances of the same test.

= 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

[Benji York (benji)]

I have some questions about this branch.

Why are test cases that have no layer filtered out? I would think that
we have at least a few tests that aren't in a layer (pure unit tests).

The imports for TestCase and filter_tests only need one line so they
shouldn't use multi-line imports.

FakeTestCase looks like it needs to stretch (it doesn't have any
whitespace between methods).

The TestFilterTests setUp method probably did something in an earlier
edit other than calling the super class' method of the same name, but
now that it doesn't, there's no need for it to be defined at all.

Instead of "self.assertTrue(layername in results)" you should use
assertIn, which generates slightly nicer error messages when the
assertion fails.

[Brad Crittenden (bac)]

I don't think there are any layer-less tests but you make a good point. I've changed the conditional to not filter tests with no layer and added a test to ensure it works.

I've incorporated your other suggestions.

[Benji York (benji)]

Looks great.

(We discussed the accidental change to .testr.conf on IRC and it is being removed.)

review: Approve (code)

[Robert Collins (lifeless)]

testr assumes that test ids are unique: we already have a hard
dependency on uniqueness, and I had totally forgotten about our test
suite containing duplicates.
https://launchpad.net/bugs/682771
and
https://launchpad.net/bugs/682772

were filed when I originally noticed that we had duplicates.

This branch would result in all tests with an identical name being
loaded when its given, but testr will already be assuming that there
is only one such test, so test counts will be permanently off.

It would be simpler, and better, to fix the root of the issue.
Testtools has a feature for renaming the test id of a test, if we have
multiple parameterised versions of a single test (testscenarios uses
that to do its parameterisation).

-Rob

[Gary Poster (gary)]

I'd like to have this landed now. This scenario was working and we made a change that caused this regression. Meanwhile, we haven't been testing with these duplicated tests for weeks, and I don't want to have additional regressions because of that.

I appreciate Robert's concerns about an underlying problem and will communicate with him separately about that.

=== 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-30 11:46:18 +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))
-            if layer is not None:
+            layer, testcases = test_lookup.get(testname, (None, None))
+            if testcases 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-30 11:46:18 +0000
@@ -0,0 +1,120 @@
+# 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
+import unittest
+
+from lp.services.testing.customresult import filter_tests
+from lp.testing.layers import BaseLayer
+
+
+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
+
+
+class TestFilterTests(TestCase):
+
+    layer = BaseLayer
+
+    def writeFile(self, fd, contents):
+        for line in contents:
+            fd.write(line + NEWLINE)
+        fd.flush()
+
+    def make_suites(self):
+        """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
+
+    def test_ordering(self):
+        # 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))
+        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.assertIn(layername, 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, suite2 = self.make_suites()
+        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.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.assertIn(layername, results)
+        suite = results[layername]
+        expected = ['1', '2', '2', '3', '3', '3']
+        self.assertEqual(expected, [t.id() for t in suite])
+
+    def test_no_layer(self):
+        # If tests have no layer (None) work.
+        testnames = ['a', 'b', 'y', 'z']
+        suite1, suite2 = self.make_suites()
+        with tempfile.NamedTemporaryFile() as fd:
+            self.writeFile(fd, testnames)
+            do_filter = filter_tests(fd.name)
+            results = do_filter({'layer1': suite1,
+                                 None: suite2})
+        self.assertEqual(2, len(results))
+        self.assertEqual([None, 'layer1'], sorted(results.keys()))
+        self.assertEqual(['a', 'b'], [t.id() for t in results['layer1']])
+        self.assertEqual(['y', 'z'], [t.id() for t in results[None]])