Merge branch 'slave-restart-msg' of git://github.com/elmirjagudin/bui…

…ldbot

slave/buildslave/scripts/runner.py

@@ -177,23 +177,33 @@ def createSlave(config):
         print "buildslave configured in %s" % m.basedir
 
 
+class SlaveNotRunning(Exception):
+    """
+    raised when trying to stop slave process that is not running
+    """
 
-def stop(config, signame="TERM", wait=False, returnFalseOnNotRunning=False):
-    import signal
-    basedir = config['basedir']
-    quiet = config['quiet']
 
-    if not base.isBuildslaveDir(config['basedir']):
-        sys.exit(1)
+def stopSlave(basedir, quiet, signame="TERM"):
+    """
+    Stop slave process by sending it a signal.
+
+    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   quite: if False, don't print any messages to stdout
+    @param signame: signal to send to the slave process
+
+    @raise SlaveNotRunning: if slave pid file is not found
+    """
+    import signal
 
     os.chdir(basedir)
     try:
         f = open("twistd.pid", "rt")
     except:
-        if returnFalseOnNotRunning:
-            return False
-        if not quiet: print "buildslave not running."
-        sys.exit(0)
+        raise SlaveNotRunning()
+
     pid = int(f.read().strip())
     signum = getattr(signal, "SIG"+signame)
     timer = 0
@@ -203,10 +213,6 @@ def stop(config, signame="TERM", wait=False, returnFalseOnNotRunning=False):
         if e.errno != 3:
             raise
 
-    if not wait:
-        if not quiet:
-            print "sent SIG%s to process" % signame
-        return
     time.sleep(0.1)
     while timer < 10:
         # poll once per second until twistd.pid goes away, up to 10 seconds
@@ -221,14 +227,31 @@ def stop(config, signame="TERM", wait=False, returnFalseOnNotRunning=False):
     if not quiet:
         print "never saw process go away"
 
+
+def stop(config, signame="TERM"):
+    quiet = config['quiet']
+    basedir = config['basedir']
+
+    if not base.isBuildslaveDir(basedir):
+        sys.exit(1)
+
+    try:
+        stopSlave(basedir, quiet, signame)
+    except SlaveNotRunning:
+        if not quiet:
+            print "buildslave not running"
+
+
 def restart(config):
     quiet = config['quiet']
 
     if not base.isBuildslaveDir(config['basedir']):
         sys.exit(1)
 
     from buildslave.scripts.startup import start
-    if not stop(config, wait=True, returnFalseOnNotRunning=True):
+    try:
+        stopSlave(config['basedir'], quiet)
+    except SlaveNotRunning:
         if not quiet:
             print "no old buildslave process found to stop"
     if not quiet:
@@ -434,7 +457,7 @@ def run():
         from buildslave.scripts.startup import start
         start(so)
     elif command == "stop":
-        stop(so, wait=True)
+        stop(so)
     elif command == "restart":
         restart(so)
     sys.exit(0)

slave/buildslave/test/unit/test_scripts_base.py

@@ -14,13 +14,12 @@
 # Copyright Buildbot Team Members
 
 import sys
-import mock
-import __builtin__
 import cStringIO
 from twisted.trial import unittest
 from buildslave.scripts import base
+from buildslave.test.util import misc
 
-class TestIsBuildslaveDir(unittest.TestCase):
+class TestIsBuildslaveDir(misc.OpenFileMixin, unittest.TestCase):
     """Test buildslave.scripts.base.isBuildslaveDir()"""
 
     def setUp(self):
@@ -34,22 +33,11 @@ def assertReadErrorMessage(self, strerror):
                          "invalid buildslave directory 'testdir'\n" % strerror,
                          "unexpected error message on stdout")
 
-    def setUpMockedTacFile(self, file_contents):
-        """Patch open() to return a file object with specified contents."""
-
-        fileobj_mock = mock.Mock()
-        fileobj_mock.read = mock.Mock(return_value=file_contents)
-        open_mock = mock.Mock(return_value=fileobj_mock)
-        self.patch(__builtin__, "open", open_mock)
-
-        return (fileobj_mock, open_mock)
-
     def test_open_error(self):
         """Test that open() errors are handled."""
 
         # patch open() to raise IOError
-        open_mock = mock.Mock(side_effect=IOError(1, "open-error", "dummy"))
-        self.patch(__builtin__, "open", open_mock)
+        self.setUpOpenError(1, "open-error", "dummy")
 
         # check that isBuildslaveDir() flags directory as invalid
         self.assertFalse(base.isBuildslaveDir("testdir"))
@@ -58,17 +46,13 @@ def test_open_error(self):
         self.assertReadErrorMessage("open-error")
 
         # check that open() was called with correct path
-        open_mock.assert_called_once_with("testdir/buildbot.tac")
+        self.open.assert_called_once_with("testdir/buildbot.tac")
 
     def test_read_error(self):
         """Test that read() errors on buildbot.tac file are handled."""
 
         # patch open() to return file object that raises IOError on read()
-        fileobj_mock = mock.Mock()
-        fileobj_mock.read = mock.Mock(side_effect=IOError(1, "read-error",
-                                                          "dummy"))
-        open_mock = mock.Mock(return_value=fileobj_mock)
-        self.patch(__builtin__, "open", open_mock)
+        self.setUpReadError(1, "read-error", "dummy")
 
         # check that isBuildslaveDir() flags directory as invalid
         self.assertFalse(base.isBuildslaveDir("testdir"))
@@ -77,13 +61,13 @@ def test_read_error(self):
         self.assertReadErrorMessage("read-error")
 
         # check that open() was called with correct path
-        open_mock.assert_called_once_with("testdir/buildbot.tac")
+        self.open.assert_called_once_with("testdir/buildbot.tac")
 
     def test_unexpected_tac_contents(self):
         """Test that unexpected contents in buildbot.tac is handled."""
 
         # patch open() to return file with unexpected contents
-        (fileobj_mock, open_mock) = self.setUpMockedTacFile("dummy-contents")
+        self.setUpOpen("dummy-contents")
 
         # check that isBuildslaveDir() flags directory as invalid
         self.assertFalse(base.isBuildslaveDir("testdir"))
@@ -94,17 +78,16 @@ def test_unexpected_tac_contents(self):
                          "invalid buildslave directory 'testdir'\n",
                          "unexpected error message on stdout")
         # check that open() was called with correct path
-        open_mock.assert_called_once_with("testdir/buildbot.tac")
+        self.open.assert_called_once_with("testdir/buildbot.tac")
 
     def test_slavedir_good(self):
         """Test checking valid buildslave directory."""
 
         # patch open() to return file with valid buildslave tac contents
-        (fileobj_mock, open_mock) = \
-            self.setUpMockedTacFile("Application('buildslave')")
+        self.setUpOpen("Application('buildslave')")
 
         # check that isBuildslaveDir() flags directory as good
         self.assertTrue(base.isBuildslaveDir("testdir"))
 
         # check that open() was called with correct path
-        open_mock.assert_called_once_with("testdir/buildbot.tac")
+        self.open.assert_called_once_with("testdir/buildbot.tac")

slave/buildslave/test/unit/test_scripts_runner.py

@@ -13,12 +13,168 @@
 #
 # Copyright Buildbot Team Members
 
+import os
+import time
 import mock
+import errno
+import signal
 from twisted.trial import unittest
 from twisted.python import usage
 from buildslave.scripts import runner, base
+from buildslave.scripts import startup
+from buildslave.test.util import misc
 
-class TestUpgradeSlave(unittest.TestCase):
+
+class IsBuildslaveDirMixin:
+    """
+    Mixin for setting up mocked base.isBuildslaveDir() function
+    """
+    def setupUpIsBuildslaveDir(self, return_value):
+        self.isBuildslaveDir = mock.Mock(return_value=return_value)
+        self.patch(base, "isBuildslaveDir", self.isBuildslaveDir)
+
+
+class TestStopSlave(misc.OpenFileMixin,
+                    misc.StdoutAssertionsMixin,
+                    unittest.TestCase):
+    """
+    Test buildslave.scripts.runner.stopSlave()
+    """
+    PID = 9876
+    def setUp(self):
+        self.setUpStdoutAssertions()
+
+        # patch os.chdir() to do nothing
+        self.patch(os, "chdir", mock.Mock())
+
+    def test_no_pid_file(self):
+        """
+        test calling stopSlave() when no pid file is present
+        """
+
+        # patch open() to raise 'file not found' exception
+        self.setUpOpenError(2)
+
+        # check that stop() raises SlaveNotRunning exception
+        self.assertRaises(runner.SlaveNotRunning,
+                          runner.stopSlave, None, False)
+
+    def test_successful_stop(self):
+        """
+        test stopSlave() on a successful slave stop
+        """
+
+        def emulated_kill(pid, sig):
+            if sig == 0:
+                # when probed if a signal can be send to the process
+                # emulate that it is dead with 'No such process' error
+                raise OSError(errno.ESRCH, "dummy")
+
+        # patch open() to return a pid file
+        self.setUpOpen(str(self.PID))
+
+        # patch os.kill to emulate successful kill
+        mocked_kill = mock.Mock(side_effect=emulated_kill)
+        self.patch(os, "kill", mocked_kill)
+
+        # don't waste time
+        self.patch(time, "sleep", mock.Mock())
+
+        # check that stopSlave() sends expected signal to right PID
+        # and print correct message to stdout
+        runner.stopSlave(None, False)
+        mocked_kill.assert_has_calls([mock.call(self.PID, signal.SIGTERM),
+                                      mock.call(self.PID, 0)])
+        self.assertStdoutEqual("buildslave process %s is dead\n" % self.PID)
+
+
+class TestStop(IsBuildslaveDirMixin,
+               misc.StdoutAssertionsMixin,
+               unittest.TestCase):
+    """
+    Test buildslave.scripts.runner.stop()
+    """
+    config = {"basedir": "dummy", "quiet": False}
+
+    def setUp(self):
+        # patch basedir check to always succeed
+        self.setupUpIsBuildslaveDir(True)
+
+    def test_no_slave_running(self):
+        """
+        test calling stop() when no slave is running
+        """
+        self.setUpStdoutAssertions()
+
+        # patch stopSlave() to raise an exception
+        mock_stopSlave = mock.Mock(side_effect=runner.SlaveNotRunning())
+        self.patch(runner, "stopSlave", mock_stopSlave)
+
+        runner.stop(self.config)
+        self.assertStdoutEqual("buildslave not running\n")
+
+    def test_successful_stop(self):
+        """
+        test calling stop() when slave is running
+        """
+        # patch stopSlave() to do nothing
+        mock_stopSlave = mock.Mock()
+        self.patch(runner, "stopSlave", mock_stopSlave)
+
+        runner.stop(self.config)
+        mock_stopSlave.assert_called_once_with(self.config["basedir"],
+                                               self.config["quiet"],
+                                               "TERM")
+
+
+class TestRestart(IsBuildslaveDirMixin,
+                  misc.StdoutAssertionsMixin,
+                  unittest.TestCase):
+    """
+    Test buildslave.scripts.runner.restart()
+    """
+    config = {"basedir": "dummy", "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)
+
+    def test_no_slave_running(self):
+        """
+        test calling restart() when no slave is running
+        """
+        # patch stopSlave() to raise an exception
+        mock_stopSlave = mock.Mock(side_effect=runner.SlaveNotRunning())
+        self.patch(runner, "stopSlave", mock_stopSlave)
+
+        # check that restart() calls start() and prints correct messages
+        runner.restart(self.config)
+        self.start.assert_called_once_with(self.config)
+        self.assertStdoutEqual("no old buildslave process found to stop\n"
+                               "now restarting buildslave process..\n")
+
+
+    def test_restart(self):
+        """
+        test calling restart() when slave is running
+        """
+        # patch stopSlave() to do nothing
+        mock_stopSlave = mock.Mock()
+        self.patch(runner, "stopSlave", mock_stopSlave)
+
+        # check that restart() calls start() and prints correct messages
+        runner.restart(self.config)
+        self.assertStdoutEqual("now restarting buildslave process..\n")
+        self.start.assert_called_once_with(self.config)
+
+
+class TestUpgradeSlave(IsBuildslaveDirMixin, unittest.TestCase):
     """
     Test buildslave.scripts.runner.upgradeSlave()
     """
@@ -28,8 +184,7 @@ def test_upgradeSlave_bad_basedir(self):
         test calling upgradeSlave() with bad base directory
         """
         # override isBuildslaveDir() to always fail
-        mocked_isBuildslaveDir = mock.Mock(return_value=False)
-        self.patch(base, "isBuildslaveDir", mocked_isBuildslaveDir)
+        self.setupUpIsBuildslaveDir(False)
 
         # call upgradeSlave() and check that SystemExit exception is raised
         config = {"basedir" : "dummy"}
@@ -39,7 +194,7 @@ def test_upgradeSlave_bad_basedir(self):
         self.assertEqual(exception.code, 1, "unexpected exit code")
 
         # check that isBuildslaveDir was called with correct argument
-        mocked_isBuildslaveDir.assert_called_once_with("dummy")
+        self.isBuildslaveDir.assert_called_once_with("dummy")
 
 
 class OptionsMixin(object):