Merge lp:~bigkevmcd/offspring/slave-testing-integration into lp:offspring

[Kevin McDermott]

More tests...now hit 100% coverage for the slave classes

Still to do, TaskMonitor and integration, I want to add at least one test that kicks off a script that "builds" successfully.

[lp:~bigkevmcd/offspring/slave-testing-integration]

139. By Kevin McDermott on 2011-12-08

Merge trunk and fix conflicts.

[Cody A.W. Somerville (cody-somerville)]

I'm going to provide a review of the changes you make specifically in this merge proposal. I have been reviewing the other test code you've written and already merged and I do have some questions but I'll contact you regarding them OOB to this MP.

>>> === modified file 'lib/offspring/slave/tests/test_slave.py'
>>> --- lib/offspring/slave/tests/test_slave.py 2011-12-08 17:29:41 +0000
>>> +++ lib/offspring/slave/tests/test_slave.py 2011-12-08 17:48:24 +0000
<snip>
>>> @@ -191,12 +193,41 @@
>>> """
>>> We can instantiate a new LexbuilderSlaveServer.
>>> """
>>> - log_file = self.capture_logging()
>>> + log_file = self.capture_logging(log_level=logging.DEBUG)
>>> +
>>> slave_server = self.create_slave_server()
>>> self.assertEqual(self.fake_slave, slave_server.slave)
>>> self.assertEqual(("127.0.0.1", 8760), slave_server.server_address)
>>> self.assertEqual(self.config, slave_server.config)
>>> - self.assertEqual("", log_file.getvalue())
>>> + self.assertEqual("Request timeout set to 60.0 seconds\n",
>>> + log_file.getvalue())
>>> +

To avoid causing unnecessary breakage if the value for request_timeout changes in LexbuilderSlaveTestMixin, we should probably construct the string we compare against the log file using the value we configured.

>>> + def test_create_with_missing_request_timeout(self):
>>> + """
>>> + LexbuilderSlaveServer should set the request timeout to the value
>>> + specified in the configuration.
>>> + """
>>> + log_file = self.capture_logging(log_level=logging.DEBUG)
>>> + self.config.set("slave", "request_timeout", "30.0")
>>> +
>>> + slave_server = LexbuilderSlaveServer(
>>> + self.fake_slave, ("localhost", 8760), self.config)
>>> + self.assertEqual("Request timeout set to 30.0 seconds\n",
>>> + log_file.getvalue())
>>> +

Is the name of this test method accurate? It doesn't appear to be.

Also, what is the value of this test since we call 'self.config.set("slave", "request_timeout", "60")' in LexbuilderSlaveTestMixin.setUp?

>>> + def test_create_with_invalid_configuration(self):
>>> + """
>>> + LexbuilderSlaveServer should handle invalid request timeout values by
>>> + falling back to the default.
>>> + """
>>> + log_file = self.capture_logging(log_level=logging.DEBUG)
>>> + self.config.set("slave", "request_timeout", "broken value")
>>> +
>>> +
>>> + slave_server = LexbuilderSlaveServer(
>>> + self.fake_slave, ("localhost", 8760), self.config)
>>> + self.assertEqual("Request timeout set to 60.0 seconds\n",
>>> + log_file.getvalue())
>>>

As with test_create, we should probably not assume the default value / configured value. In this case, you'd have to inspect the __init__ method on LexbuilderSlaveServer or update it to use a constant that you can import.

Additionally, we don't actually get a connection and test that calling gettimeout on the socket returns the value we expect - I imagine this is something we should do to...

review: Approve

[Kevin McDermott (bigkevmcd)]

> >>> + self.assertEqual("Request timeout set to 60.0 seconds\n",
> >>> + log_file.getvalue())
> >>> +
>
> To avoid causing unnecessary breakage if the value for request_timeout changes
> in LexbuilderSlaveTestMixin, we should probably construct the string we
> compare against the log file using the value we configured.

Given that we're testing the code, and if we change the code, we should have changed the tests to match, this is expected behaviour we're testing for...if you want to change the coded default value, change the test first...

> Is the name of this test method accurate? It doesn't appear to be.
>
> Also, what is the value of this test since we call 'self.config.set("slave",
> "request_timeout", "60")' in LexbuilderSlaveTestMixin.setUp?

Ooops...dunno what I was thinking with that test, renamed the test, and added an additional test for when it's actually missing.

> >>>
>
> As with test_create, we should probably not assume the default value /
> configured value. In this case, you'd have to inspect the __init__ method on
> LexbuilderSlaveServer or update it to use a constant that you can import.

See above...

> Additionally, we don't actually get a connection and test that calling
> gettimeout on the socket returns the value we expect - I imagine this is
> something we should do to properly test this.

The Slave doesn't actually do this, it's the responsibility of LexbuilderXMLRPCServer to set the timeout (which needs tests written to do this).

=== modified file 'lib/offspring/slave/tests/test_slave.py'
--- lib/offspring/slave/tests/test_slave.py	2011-12-08 17:29:41 +0000
+++ lib/offspring/slave/tests/test_slave.py	2011-12-08 17:48:24 +0000
@@ -1,12 +1,16 @@
 # Copyright 2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 import platform
+import logging
 from datetime import datetime
 from ConfigParser import ConfigParser
 import xmlrpclib
 
+from mocker import MockerTestCase
+
 from offspring.tests.helpers import CaptureLoggingTestCase
 
+from offspring.enums import ProjectBuildStates
 from offspring.slave import (
     LexbuilderSlave, ProjectBuildProcess, LexbuilderSlaveServer,
     ShutdownThread)
@@ -89,16 +93,13 @@
         self.slave = LexbuilderSlave(("localhost", 8760), self.config,
                                      self.fake_server)
 
-    def tearDown(self):
-        super(TestLexbuilderSlave, self).tearDown()
-
     def test_slave_run_starts_server_and_logs(self):
         """
         LexbuilderSlave.run should start the LexbuilderSlaveServer.
         """
         log_file = self.capture_logging()
         self.slave.run()
-        self.assertIn("Slave ready to accept builds", log_file.getvalue())
+        self.assertEqual("Slave ready to accept builds\n", log_file.getvalue())
         self.assertTrue(self.fake_server.started)
 
     def test_slave_stop(self):
@@ -155,11 +156,12 @@
 
 class FakeLexbuilderSlave(object):
     """Fake slave for use in tests."""
-    def __init__(self, state=LexbuilderSlave.STATE_IDLE):
+    def __init__(self, config=None, state=LexbuilderSlave.STATE_IDLE):
         self.project_name = None
         self.config_url = None
         self.stopped = False
         self.state = state
+        self.config = config
 
     def startBuild(self, project_name, config_url):
         self.project_name = project_name
@@ -178,7 +180,7 @@
 
     def setUp(self):
         super(TestLexbuilderSlaveServer, self).setUp()
-        self.fake_slave = FakeLexbuilderSlave()
+        self.fake_slave = FakeLexbuilderSlave(self.config)
 
     def create_slave_server(self):
         """
@@ -191,12 +193,41 @@
         """
         We can instantiate a new LexbuilderSlaveServer.
         """
-        log_file = self.capture_logging()
+        log_file = self.capture_logging(log_level=logging.DEBUG)
+
         slave_server = self.create_slave_server()
         self.assertEqual(self.fake_slave, slave_server.slave)
         self.assertEqual(("127.0.0.1", 8760), slave_server.server_address)
         self.assertEqual(self.config, slave_server.config)
-        self.assertEqual("", log_file.getvalue())
+        self.assertEqual("Request timeout set to 60.0 seconds\n",
+                         log_file.getvalue())
+
+    def test_create_with_missing_request_timeout(self):
+        """
+        LexbuilderSlaveServer should set the request timeout to the value
+        specified in the configuration.
+        """
+        log_file = self.capture_logging(log_level=logging.DEBUG)
+        self.config.set("slave", "request_timeout", "30.0")
+
+        slave_server = LexbuilderSlaveServer(
+            self.fake_slave, ("localhost", 8760), self.config)
+        self.assertEqual("Request timeout set to 30.0 seconds\n",
+                         log_file.getvalue())
+
+    def test_create_with_invalid_configuration(self):
+        """
+        LexbuilderSlaveServer should handle invalid request timeout values by
+        falling back to the default.
+        """
+        log_file = self.capture_logging(log_level=logging.DEBUG)
+        self.config.set("slave", "request_timeout", "broken value")
+
+
+        slave_server = LexbuilderSlaveServer(
+            self.fake_slave, ("localhost", 8760), self.config)
+        self.assertEqual("Request timeout set to 60.0 seconds\n",
+                         log_file.getvalue())
 
     def test_api_start_build(self):
         """
@@ -485,3 +516,189 @@
         self.assertEqual(LexbuilderSlaveServer.REQUEST_OKAY,
                          slave_server.api_shutdown())
         self.assertTrue(fake_shutdown.started)
+
+
+class TestProjectBuildProcess(LexbuilderSlaveTestMixin, CaptureLoggingTestCase):
+
+    def setUp(self):
+        super(TestProjectBuildProcess, self).setUp()
+        self.master = FakeLexbuilderSlave(config=self.config)
+        self.build_process = ProjectBuildProcess(
+            self.master, u"testing", "http://example.com/project")
+
+    def test_create(self):
+        """
+        We can instantiate a new ProjectBuildProcess.
+        """
+        self.assertEqual(u"testing", self.build_process.projectName)
+        self.assertEqual([u"testing", "http://example.com/project"],
+                         self.build_process.arguments)
+
+    def test_get_state_currently_active(self):
+        """
+        If ProjectBuildProcess.state is STATE_ACTIVE then
+        ProjectBuildProcess.get_state() should return
+        ProjectBuildStates.PENDING.
+        """
+        self.build_process.state = self.build_process.STATE_ACTIVE
+        self.assertEqual(ProjectBuildStates.PENDING,
+                         self.build_process.getState())
+
+    def test_get_state_currently_failed(self):
+        """
+        If ProjectBuildProcess.state is STATE_FAILED then
+        ProjectBuildProcess.get_state() should return
+        ProjectBuildStates.FAILED.
+        """
+        self.build_process.state = self.build_process.STATE_FAILED
+        self.assertEqual(ProjectBuildStates.FAILED,
+                         self.build_process.getState())
+
+    def test_get_state_currently_success(self):
+        """
+        If ProjectBuildProcess.state is STATE_SUCCESS then
+        ProjectBuildProcess.get_state() should return
+        ProjectBuildStates.SUCCESS.
+        """
+        self.build_process.state = self.build_process.STATE_SUCCESS
+        self.assertEqual(ProjectBuildStates.SUCCESS,
+                         self.build_process.getState())
+
+    def test_get_state_currently_unknown_state(self):
+        """
+        If ProjectBuildProcess.state is not one of STATE_ACTIVE, STATE_FAILED
+        or STATE_SUCCESS ProjectBuildProcess.get_state() should return
+        ProjectBuildStates.UNKNOWN.
+        """
+        self.build_process.state = None
+        self.assertEqual(ProjectBuildStates.UNKNOWN,
+                         self.build_process.getState())
+
+    def test_pre_execution(self):
+        """
+        ProjectBuildProcess.preExecution should log the fact that the slave is
+        starting a build, and set the current state of the LexbuilderSlave to
+        STATE_BUILDING.
+        """
+        log_file = self.capture_logging(log_level=logging.DEBUG)
+
+        self.build_process.preExecution()
+        self.assertEqual(LexbuilderSlave.STATE_BUILDING,
+                         self.build_process.master.state)
+        self.assertEqual("Setting state of slave to BUILDING\n",
+                         log_file.getvalue())
+
+    def test_post_execution(self):
+        """
+        ProjectBuildProcess.preExecution should log the fact that the slave is
+        finishing a build, and set the current state of the LexbuilderSlave to
+        STATE_IDLE.
+        """
+        log_file = self.capture_logging(log_level=logging.DEBUG)
+
+        self.build_process.postExecution()
+        self.assertEqual(LexbuilderSlave.STATE_IDLE,
+                         self.build_process.master.state)
+        self.assertEqual("Setting state of slave to IDLE\n",
+                         log_file.getvalue())
+
+    def test_get_command_path(self):
+        """
+        ProjectBuildProcess.getCommandPath should return the configured
+        build_command.
+        """
+        self.config.set("slave", "build_command",
+                        "/usr/bin/offspring-build")
+        self.assertEqual("/usr/bin/offspring-build",
+                         self.build_process.getCommandPath())
+
+    def test_get_project_name(self):
+        """
+        ProjectBuildProcess.getProjectName should return the name of the
+        project currently configured for building.
+        """
+        self.build_process.projectName = u"testing"
+        self.assertEqual(u"testing",
+                         self.build_process.getProjectName())
+
+    def test_get_build_name_with_known_build_name(self):
+        """
+        ProjectBuildProcess.getBuildName should return the name of the current
+        build if known.
+        """
+        self.build_process.buildName = u"testing"
+        self.assertEqual(u"testing", self.build_process.getBuildName())
+
+    def test_get_build_name_with_unknown_build_inactive_process(self):
+        """
+        If the build name is unknown, and the process is not currently active,
+        then we ProjectBuildProcess.getBuildName should examine the output of
+        the build process and look for START <projectname> <buildname> and
+        extract the buildname and return it.
+        """
+        self.build_process.stdoutdata = "START project testproject mybuild\n"
+        self.assertEqual("mybuild", self.build_process.getBuildName())
+
+    def test_get_build_name_with_unknown_build_active_process(self):
+        """
+        If the build name is unknown, and the process is currently active,
+        then we ProjectBuildProcess.getBuildName should examine the the build
+        statefile from the configuration and parse the project name and build
+        name from there.
+        """
+        self.build_process.state = self.build_process.STATE_ACTIVE
+        self.build_process.projectName = "newproject"
+        build_statefile_name = self.makeFile("newproject newbuild\n")
+        self.config.set("slave", "build_statefile", build_statefile_name)
+        self.assertEqual("newbuild", self.build_process.getBuildName())
+
+    def test_get_build_name_with_unknown_build_active_invalid_project(self):
+        """
+        If the build name is unknown, and the process is currently active,
+        then we ProjectBuildProcess.getBuildName should examine the the build
+        statefile from the configuration and parse the project name and build
+        name from there.
+
+        If the project name parsed from the build file doesn't match the
+        project we're building, then we should get an error.
+        """
+        self.build_process.state = self.build_process.STATE_ACTIVE
+        self.build_process.projectName = "anotherprojectentirely"
+        build_statefile_name = self.makeFile("newproject newbuild\n")
+        self.config.set("slave", "build_statefile", build_statefile_name)
+
+        self.assertEqual(LexbuilderSlaveServer.REQUEST_ERROR,
+                         self.build_process.getBuildName())
+
+    def test_get_build_name_with_unknown_build_active_process_error(self):
+        """
+        If the build name is unknown, and the process is currently active,
+        then we ProjectBuildProcess.getBuildName should examine the the build
+        statefile from the configuration, any error reading this should result
+        in an error return.
+        """
+        self.build_process.state = self.build_process.STATE_ACTIVE
+        self.build_process.projectName = "newproject"
+        build_statefile_name = self.makeFile()
+        self.config.set("slave", "build_statefile", build_statefile_name)
+        self.assertEqual(LexbuilderSlaveServer.REQUEST_ERROR,
+                         self.build_process.getBuildName())
+
+
+class TestShutdownThread(MockerTestCase):
+    """
+    Tests for the ShutdownThread class.
+    """
+
+    def test_run(self):
+        """
+        """
+        sleep_mock = self.mocker.replace("time.sleep")
+        sleep_mock(1)
+        self.mocker.replay()
+        master = FakeLexbuilderSlaveServer()
+
+        shutdown_thread = ShutdownThread(master)
+        shutdown_thread.run()
+
+        self.assertTrue(master.stopped)