don't call isBuildslaveDir() twice on buildslave restart

Broke out code to start buildslave process into a utility function
startSlave(). Use it from restart() and startCommand().

slave/buildslave/scripts/runner.py

@@ -190,7 +190,7 @@ def stopSlave(basedir, quiet, signame="TERM"):
     Using the specified basedir path, read slave process's pid file and
     try to terminate that process with specified signal.
 
-    @param basedir: buildslave basedir path
+    @param basedir: buildslave's basedir path
     @param   quite: if False, don't print any messages to stdout
     @param signame: signal to send to the slave process
 
@@ -244,19 +244,21 @@ def stop(config, signame="TERM"):
 
 def restart(config):
     quiet = config['quiet']
+    basedir = config['basedir']
 
-    if not base.isBuildslaveDir(config['basedir']):
+    if not base.isBuildslaveDir(basedir):
         sys.exit(1)
 
-    from buildslave.scripts.startup import start
     try:
-        stopSlave(config['basedir'], quiet)
+        stopSlave(basedir, quiet)
     except SlaveNotRunning:
         if not quiet:
             print "no old buildslave process found to stop"
     if not quiet:
         print "now restarting buildslave process.."
-    start(config)
+
+    from buildslave.scripts.startup import startSlave
+    startSlave(basedir, quiet, config['nodaemon'])
 
 
 class MakerBase(usage.Options):
@@ -454,8 +456,8 @@ def run():
     elif command == "upgrade-slave":
         upgradeSlave(so)
     elif command == "start":
-        from buildslave.scripts.startup import start
-        start(so)
+        from buildslave.scripts.startup import startCommand
+        startCommand(so)
     elif command == "stop":
         stop(so)
     elif command == "restart":

slave/buildslave/scripts/startup.py

@@ -78,18 +78,37 @@ def _failure(self, why):
         reactor.stop()
 
 
-def start(config):
-    if not base.isBuildslaveDir(config['basedir']):
+def startCommand(config):
+    basedir = config['basedir']
+    if not base.isBuildslaveDir(basedir):
         sys.exit(1)
 
-    os.chdir(config['basedir'])
-    if config['quiet'] or config['nodaemon']:
-        return launch(config)
+    startSlave(basedir, config['quiet'], config['nodaemon'])
+
+def startSlave(basedir, quiet, nodaemon):
+    """
+    Start slave process.
+
+    Fork and start twisted application described in basedir buildbot.tac file.
+    Print it's log messages to stdout for a while and try to figure out if
+    start was successful.
+
+    If quiet or nodaemon parameters are True, or we are running on a win32
+    system, will not fork and log will not be printed to stdout.
+
+    @param  basedir: buildslave's basedir path
+    @param    quiet: don't display startup log messages
+    @param nodaemon: don't daemonize (stay in foreground)
+    """
+
+    os.chdir(basedir)
+    if quiet or nodaemon:
+        return launch(nodaemon)
 
     # we probably can't do this os.fork under windows
     from twisted.python.runtime import platformType
     if platformType == "win32":
-        return launch(config)
+        return launch(nodaemon)
 
     # fork a child to launch the daemon, while the parent process tails the
     # logfile
@@ -100,9 +119,9 @@ def start(config):
     # this is the child: give the logfile-watching parent a chance to start
     # watching it before we start the daemon
     time.sleep(0.2)
-    launch(config)
+    launch(nodaemon)
 
-def launch(config):
+def launch(nodaemon):
     sys.path.insert(0, os.path.abspath(os.getcwd()))
 
     # see if we can launch the application without actually having to
@@ -113,7 +132,7 @@ def launch(config):
             "--no_save",
             "--logfile=twistd.log", # windows doesn't use the same default
             "--python=buildbot.tac"]
-    if config['nodaemon']:
+    if nodaemon:
         argv.extend(['--nodaemon'])
     sys.argv = argv
 

slave/buildslave/test/unit/test_scripts_runner.py

@@ -123,17 +123,17 @@ class TestRestart(misc.IsBuildslaveDirMixin,
     """
     Test buildslave.scripts.runner.restart()
     """
-    config = {"basedir": "dummy", "quiet": False}
+    config = {"basedir": "dummy", "nodaemon": False, "quiet": False}
 
     def setUp(self):
         self.setUpStdoutAssertions()
 
         # patch basedir check to always succeed
         self.setupUpIsBuildslaveDir(True)
 
-        # patch startup.start() to do nothing
-        self.start = mock.Mock()
-        self.patch(startup, "start", self.start)
+        # patch startup.startSlave() to do nothing
+        self.startSlave = mock.Mock()
+        self.patch(startup, "startSlave", self.startSlave)
 
     def test_no_slave_running(self):
         """
@@ -143,9 +143,11 @@ def test_no_slave_running(self):
         mock_stopSlave = mock.Mock(side_effect=runner.SlaveNotRunning())
         self.patch(runner, "stopSlave", mock_stopSlave)
 
-        # check that restart() calls start() and prints correct messages
+        # check that restart() calls startSlave() and prints correct messages
         runner.restart(self.config)
-        self.start.assert_called_once_with(self.config)
+        self.startSlave.assert_called_once_with(self.config["basedir"],
+                                                self.config["quiet"],
+                                                self.config["nodaemon"]);
         self.assertStdoutEqual("no old buildslave process found to stop\n"
                                "now restarting buildslave process..\n")
 
@@ -158,10 +160,12 @@ def test_restart(self):
         mock_stopSlave = mock.Mock()
         self.patch(runner, "stopSlave", mock_stopSlave)
 
-        # check that restart() calls start() and prints correct messages
+        # check that restart() calls startSlave() and prints correct messages
         runner.restart(self.config)
+        self.startSlave.assert_called_once_with(self.config["basedir"],
+                                                self.config["quiet"],
+                                                self.config["nodaemon"]);
         self.assertStdoutEqual("now restarting buildslave process..\n")
-        self.start.assert_called_once_with(self.config)
 
 
 class TestUpgradeSlave(misc.IsBuildslaveDirMixin, unittest.TestCase):

slave/buildslave/test/unit/test_scripts_startup.py

@@ -0,0 +1,64 @@
+# This file is part of Buildbot.  Buildbot is free software: you can
+# redistribute it and/or modify it under the terms of the GNU General Public
+# License as published by the Free Software Foundation, version 2.
+#
+# This program is distributed in the hope that it will be useful, but WITHOUT
+# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
+# FOR A PARTICULAR PURPOSE.  See the GNU General Public License for more
+# details.
+#
+# You should have received a copy of the GNU General Public License along with
+# this program; if not, write to the Free Software Foundation, Inc., 51
+# Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+#
+# Copyright Buildbot Team Members
+
+import mock
+from twisted.trial import unittest
+from buildslave.scripts import startup
+from buildslave.test.util import misc
+
+
+class TestStartCommand(unittest.TestCase, misc.IsBuildslaveDirMixin):
+    """
+    Test buildslave.scripts.startup.startCommand()
+    """
+    def test_start_command_bad_basedir(self):
+        """
+        test calling startCommand() with invalid basedir path
+        """
+
+        # patch isBuildslaveDir() to fail
+        self.setupUpIsBuildslaveDir(False)
+
+        # call start() and check that SystemExit exception is raised
+        config = {"basedir": "dummy"}
+        exception = self.assertRaises(SystemExit, startup.startCommand, config)
+
+        # check exit code
+        self.assertEqual(exception.code, 1, "unexpected exit code")
+
+        # check that isBuildslaveDir was called with correct argument
+        self.isBuildslaveDir.assert_called_once_with("dummy")
+
+    def test_start_command_good(self):
+        """
+        test successful startCommand() call
+        """
+
+        # patch basedir check to always succeed
+        self.setupUpIsBuildslaveDir(True)
+
+        # patch startSlave() to do nothing
+        mocked_startSlave = mock.Mock()
+        self.patch(startup, "startSlave", mocked_startSlave)
+
+        config = {"basedir": "dummy", "nodaemon": False, "quiet": False}
+        startup.startCommand(config)
+
+        # check that isBuildslaveDir() and startSlave() were called
+        # with correct argument
+        self.isBuildslaveDir.assert_called_once_with("dummy")
+        mocked_startSlave.assert_called_once_with(config["basedir"],
+                                                  config["quiet"],
+                                                  config["nodaemon"])