Merge lp:~jml/launchpad/launchpad-tester into lp:launchpad

[Jonathan Lange]

Add tests for devscripts.ec2test.remote.LaunchpadTester.

This branch only makes one small behaviour change -- it flushes the summary
log more regularly so that the "Summary" on an ec2 test web server is always
up-to-date.

More interestingly, it adds tests for LaunchpadTester.

I was going to change the behaviour more significantly, but every time I
did I felt uncomfortable and uncertain.

[Robert Collins (lifeless)]

It might be nice to:
 - use fixtures rather than pushing those helpers around
 - flush the detailed log too, so that tribunal stays up to date more.

review: Approve

[Jonathan Lange (jml)]

On Tue, Aug 24, 2010 at 8:41 PM, Robert Collins
<email address hidden> wrote:
> Review: Approve
> It might be nice to:
>  - use fixtures rather than pushing those helpers around

You mean "prefer composition to inheritance", right? Does bzrlib give
me a way to do make_branch_and_tree without subclassing
TestCaseWithRepository?

>  - flush the detailed log too, so that tribunal stays up to date more.

The detailed log is flushed elsewhere.

jml

=== modified file 'lib/devscripts/ec2test/remote.py'
--- lib/devscripts/ec2test/remote.py	2010-08-24 17:51:03 +0000
+++ lib/devscripts/ec2test/remote.py	2010-08-24 17:51:06 +0000
@@ -213,8 +213,6 @@
 class LaunchpadTester:
     """Runs Launchpad tests and gathers their results in a useful way."""
 
-    # XXX: JonathanLange 2010-08-17: LaunchpadTester needs tests.
-
     def __init__(self, logger, test_directory, test_options=()):
         """Construct a TestOnMergeRunner.
 
@@ -236,9 +234,25 @@
 
         Subclasses must provide their own implementation of this method.
         """
-        command = ['make', 'check', 'TESTOPTS="%s"' % self._test_options]
+        command = ['make', 'check']
+        if self._test_options:
+            command.append('TESTOPTS="%s"' % self._test_options)
         return command
 
+    def _spawn_test_process(self):
+        """Actually run the tests.
+
+        :return: A `subprocess.Popen` object for the test run.
+        """
+        call = self.build_test_command()
+        self._logger.write_line("Running %s" % (call,))
+        # bufsize=0 means do not buffer any of the output. We want to
+        # display the test output as soon as it is generated.
+        return subprocess.Popen(
+            call, bufsize=0,
+            stdout=subprocess.PIPE, stderr=subprocess.STDOUT,
+            cwd=self._test_directory)
+
     def test(self):
         """Run the tests, log the results.
 
@@ -247,20 +261,9 @@
         the user the test results.
         """
         self._logger.prepare()
-
-        call = self.build_test_command()
-
         try:
-            self._logger.write_line("Running %s" % (call,))
-            # XXX: JonathanLange 2010-08-18: bufsize=-1 implies "use the
-            # system buffering", when we actually want no buffering at all.
-            popen = subprocess.Popen(
-                call, bufsize=-1,
-                stdout=subprocess.PIPE, stderr=subprocess.STDOUT,
-                cwd=self._test_directory)
-
+            popen = self._spawn_test_process()
             self._gather_test_output(popen.stdout, self._logger)
-
             exit_status = popen.wait()
         except:
             self._logger.error_in_testrunner(sys.exc_info())
@@ -277,6 +280,7 @@
         for line in input_stream:
             subunit_server.lineReceived(line)
             logger.got_line(line)
+            summary_stream.flush()
 
 
 class Request:

=== modified file 'lib/devscripts/ec2test/tests/test_remote.py'
--- lib/devscripts/ec2test/tests/test_remote.py	2010-08-24 17:51:03 +0000
+++ lib/devscripts/ec2test/tests/test_remote.py	2010-08-24 17:51:06 +0000
@@ -29,6 +29,7 @@
     FlagFallStream,
     gunzip_data,
     gzip_data,
+    LaunchpadTester,
     remove_pidfile,
     Request,
     SummaryResult,
@@ -37,6 +38,69 @@
     )
 
 
+class RequestHelpers:
+
+    def patch(self, obj, name, value):
+        orig = getattr(obj, name)
+        setattr(obj, name, value)
+        self.addCleanup(setattr, obj, name, orig)
+        return orig
+
+    def make_trunk(self, parent_url='http://example.com/bzr/trunk'):
+        """Make a trunk branch suitable for use with `Request`.
+
+        `Request` expects to be given a path to a working tree that has a
+        branch with a configured parent URL, so this helper returns such a
+        working tree.
+        """
+        nick = parent_url.strip('/').split('/')[-1]
+        tree = self.make_branch_and_tree(nick)
+        tree.branch.set_parent(parent_url)
+        return tree
+
+    def make_request(self, branch_url=None, revno=None,
+                     trunk=None, sourcecode_path=None,
+                     emails=None, pqm_message=None):
+        """Make a request to test, specifying only things we care about.
+
+        Note that the returned request object will not ever send email, but
+        will instead log "sent" emails to `request.emails_sent`.
+        """
+        if trunk is None:
+            trunk = self.make_trunk()
+        if sourcecode_path is None:
+            sourcecode_path = self.make_sourcecode(
+                [('a', 'http://example.com/bzr/a', 2),
+                 ('b', 'http://example.com/bzr/b', 3),
+                 ('c', 'http://example.com/bzr/c', 5)])
+        request = Request(
+            branch_url, revno, trunk.basedir, sourcecode_path, emails,
+            pqm_message)
+        request.emails_sent = []
+        request._send_email = request.emails_sent.append
+        return request
+
+    def make_sourcecode(self, branches):
+        """Make a sourcecode directory with sample branches.
+
+        :param branches: A list of (name, parent_url, revno) tuples.
+        :return: The path to the sourcecode directory.
+        """
+        self.build_tree(['sourcecode/'])
+        for name, parent_url, revno in branches:
+            tree = self.make_branch_and_tree('sourcecode/%s' % (name,))
+            tree.branch.set_parent(parent_url)
+            for i in range(revno):
+                tree.commit(message=str(i))
+        return 'sourcecode/'
+
+    def make_logger(self, request=None, echo_to_stdout=False):
+        if request is None:
+            request = self.make_request()
+        return WebTestLogger(
+            'full.log', 'summary.log', 'index.html', request, echo_to_stdout)
+
+
 class TestFlagFallStream(TestCase):
     """Tests for `FlagFallStream`."""
 
@@ -122,6 +186,104 @@
         self.assertEqual(1, len(flush_calls))
 
 
+class FakePopen:
+    """Fake Popen object so we don't have to spawn processes in tests."""
+
+    def __init__(self, output, exit_status):
+        self.stdout = StringIO(output)
+        self._exit_status = exit_status
+
+    def wait(self):
+        return self._exit_status
+
+
+class TestLaunchpadTester(TestCaseWithTransport, RequestHelpers):
+
+    def make_tester(self, logger=None, test_directory=None, test_options=()):
+        if not logger:
+            logger = self.make_logger()
+        if not test_directory:
+            test_directory = 'unspecified-test-directory'
+        return LaunchpadTester(logger, test_directory, test_options)
+
+    def test_build_test_command_no_options(self):
+        # The LaunchpadTester runs "make check" if given no options.
+        tester = self.make_tester()
+        command = tester.build_test_command()
+        self.assertEqual(['make', 'check'], command)
+
+    def test_build_test_command_options(self):
+        # The LaunchpadTester runs 'make check TESTOPTIONS="<options>"' if
+        # given options.
+        tester = self.make_tester(test_options=('-vvv', '--subunit'))
+        command = tester.build_test_command()
+        self.assertEqual(
+            ['make', 'check', 'TESTOPTS="-vvv --subunit"'], command)
+
+    def test_spawn_test_process(self):
+        # _spawn_test_process uses subprocess.Popen to run the command
+        # returned by build_test_command. stdout & stderr are piped together,
+        # the cwd is the test directory specified in the constructor, and the
+        # bufsize is zore, meaning "don't buffer".
+        popen_calls = []
+        self.patch(
+            subprocess, 'Popen',
+            lambda *args, **kwargs: popen_calls.append((args, kwargs)))
+        tester = self.make_tester(test_directory='test-directory')
+        tester._spawn_test_process()
+        self.assertEqual(
+            [((tester.build_test_command(),),
+              {'bufsize': 0,
+               'stdout': subprocess.PIPE,
+               'stderr': subprocess.STDOUT,
+               'cwd': 'test-directory'})], popen_calls)
+
+    def test_running_test(self):
+        # LaunchpadTester.test() runs the test command, and then calls
+        # got_result with the result.  This test is more of a smoke test to
+        # make sure that everything integrates well.
+        message = {'Subject': "One Crowded Hour"}
+        request = self.make_request(pqm_message=message)
+        logger = self.make_logger(request=request)
+        tester = self.make_tester(logger=logger)
+        output = "test output\n"
+        tester._spawn_test_process = lambda: FakePopen(output, 0)
+        tester.test()
+        # Message being sent implies got_result thought it got a success.
+        self.assertEqual([message], request.emails_sent)
+
+    def test_error_in_testrunner(self):
+        # Any exception is raised within LaunchpadTester.test() is an error in
+        # the testrunner. When we detect these, we do three things:
+        #   1. Log the error to the logger using error_in_testrunner
+        #   2. Call got_result with a False value, indicating test suite
+        #      failure.
+        #   3. Re-raise the error. In the script, this triggers an email.
+        message = {'Subject': "One Crowded Hour"}
+        request = self.make_request(pqm_message=message)
+        logger = self.make_logger(request=request)
+        tester = self.make_tester(logger=logger)
+        # Break the test runner deliberately. In production, this is more
+        # likely to be a system error than a programming error.
+        tester._spawn_test_process = lambda: 1/0
+        self.assertRaises(ZeroDivisionError, tester.test)
+        # Message not being sent implies got_result thought it got a failure.
+        self.assertEqual([], request.emails_sent)
+        self.assertIn("ERROR IN TESTRUNNER", logger.get_summary_contents())
+        self.assertIn("ZeroDivisionError", logger.get_summary_contents())
+
+    def test_nonzero_exit_code(self):
+        message = {'Subject': "One Crowded Hour"}
+        request = self.make_request(pqm_message=message)
+        logger = self.make_logger(request=request)
+        tester = self.make_tester(logger=logger)
+        output = "test output\n"
+        tester._spawn_test_process = lambda: FakePopen(output, 10)
+        tester.test()
+        # Message not being sent implies got_result thought it got a failure.
+        self.assertEqual([], request.emails_sent)
+
+
 class TestPidfileHelpers(TestCase):
     """Tests for `write_pidfile` and `remove_pidfile`."""
 
@@ -163,63 +325,6 @@
         self.assertEqual(data, gunzip_data(compressed))
 
 
-class RequestHelpers:
-
-    def make_trunk(self, parent_url='http://example.com/bzr/trunk'):
-        """Make a trunk branch suitable for use with `Request`.
-
-        `Request` expects to be given a path to a working tree that has a
-        branch with a configured parent URL, so this helper returns such a
-        working tree.
-        """
-        nick = parent_url.strip('/').split('/')[-1]
-        tree = self.make_branch_and_tree(nick)
-        tree.branch.set_parent(parent_url)
-        return tree
-
-    def make_request(self, branch_url=None, revno=None,
-                     trunk=None, sourcecode_path=None,
-                     emails=None, pqm_message=None):
-        """Make a request to test, specifying only things we care about.
-
-        Note that the returned request object will not ever send email, but
-        will instead log "sent" emails to `request.emails_sent`.
-        """
-        if trunk is None:
-            trunk = self.make_trunk()
-        if sourcecode_path is None:
-            sourcecode_path = self.make_sourcecode(
-                [('a', 'http://example.com/bzr/a', 2),
-                 ('b', 'http://example.com/bzr/b', 3),
-                 ('c', 'http://example.com/bzr/c', 5)])
-        request = Request(
-            branch_url, revno, trunk.basedir, sourcecode_path, emails,
-            pqm_message)
-        request.emails_sent = []
-        request._send_email = request.emails_sent.append
-        return request
-
-    def make_sourcecode(self, branches):
-        """Make a sourcecode directory with sample branches.
-
-        :param branches: A list of (name, parent_url, revno) tuples.
-        :return: The path to the sourcecode directory.
-        """
-        self.build_tree(['sourcecode/'])
-        for name, parent_url, revno in branches:
-            tree = self.make_branch_and_tree('sourcecode/%s' % (name,))
-            tree.branch.set_parent(parent_url)
-            for i in range(revno):
-                tree.commit(message=str(i))
-        return 'sourcecode/'
-
-    def make_logger(self, request=None, echo_to_stdout=False):
-        if request is None:
-            request = self.make_request()
-        return WebTestLogger(
-            'full.log', 'summary.log', 'index.html', request, echo_to_stdout)
-
-
 class TestRequest(TestCaseWithTransport, RequestHelpers):
     """Tests for `Request`."""
 
@@ -437,12 +542,6 @@
 
 class TestWebTestLogger(TestCaseWithTransport, RequestHelpers):
 
-    def patch(self, obj, name, value):
-        orig = getattr(obj, name)
-        setattr(obj, name, value)
-        self.addCleanup(setattr, obj, name, orig)
-        return orig
-
     def test_make_in_directory(self):
         # WebTestLogger.make_in_directory constructs a logger that writes to a
         # bunch of specific files in a directory.