Added unittests for run_slave

slave/tests/run_slave_test.py

Index: slave/tests/run_slave_test.py
diff --git a/slave/tests/run_slave_test.py b/slave/tests/run_slave_test.py
new file mode 100755
index 0000000000000000000000000000000000000000..aafad79be1114f684a07c519876b6809a9482ef2
--- /dev/null
+++ b/slave/tests/run_slave_test.py
@@ -0,0 +1,86 @@
+#!/usr/bin/env python
+# Copyright 2016 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+"""Starts run_slave and verify if it starts successfully.
+"""
+
+from mock import MagicMock, patch

[dsansome, 2016/10/10 05:25:18]

The style guide says not to import functions and classes from modules.  Do
"import mock" instead and use mock.MagicMock and mock.patch

[ddoman1, 2016/10/12 00:42:23]

Done.

+import os
+import re
+import runpy
+import subprocess
+import sys
+import unittest
+
+
+RUN_SLAVE_PATH = os.path.join(os.path.dirname(os.path.abspath(__file__)), "..")
+print RUN_SLAVE_PATH

[dsansome, 2016/10/10 05:25:18]

Remove this line

[ddoman1, 2016/10/12 00:42:23]

Done.

+sys.path.append(RUN_SLAVE_PATH)
+
+import run_slave
+
+# import twisted to mock run()
+_, tw_ver = run_slave.GetThirdPartyVersions(None)
+sys.path.append(os.path.join(RUN_SLAVE_PATH, 'third_party', tw_ver))

[dsansome, 2016/10/10 05:25:18]

Other tests use scripts/common/env.py to do this instead.

  sys.path.insert(0,
      os.path.join(os.path.dirname(__file__), os.pardir, ...))
      # (/path/to/build/scripts)
  import common.env
  common.env.Install()

[dsansome, 2016/10/12 03:54:04]

Did you try this?  Did it work?

+import twisted.scripts.twistd
+
+
+class ExecvExecuted(Exception):
+  # This exeception is raised within a mocked os.execv() so that

[dsansome, 2016/10/10 05:25:18]

typo: s/exeception/exception/

[ddoman1, 2016/10/12 00:42:23]

Done.

+  # the execution flow gets interrupted.
+  pass
+
+
+class RunSlaveTest(unittest.TestCase):
+  def test_run_slave_restart_after_gclient_sync(self):
+    """Tests if run_slave restarts itself after gclient sync."""
+    def MockExecv(executable_path, argv):

[dsansome, 2016/10/10 05:25:18]

Can you use mock.Mock instead?

mock_execv = mock.Mock(side_effect=ExecvExecuted)
...
self.assertIn(run_slave_py_path, mock_execv.call_args[0][0])

[ddoman1, 2016/10/12 00:42:23]

Good point!

+      MockExecv.executable_path = executable_path
+      MockExecv.argv = argv
+      raise ExecvExecuted()
+
+    # test if ryn_slave.py restarts itself after gclient sync

[dsansome, 2016/10/10 05:25:18]

typo: s/ryn/run/

[ddoman1, 2016/10/12 00:42:23]

Done.

+    with patch('subprocess.call') as subprocess_call, \
+         patch('os.execv', MockExecv), \
+         self.assertRaises(ExecvExecuted):
+        runpy.run_module("run_slave", run_name="__main__", alter_sys=True)

[dsansome, 2016/10/10 05:25:18]

Should be 2-space indentation here

[ddoman1, 2016/10/12 00:42:23]

Done.

+
+    # verify that gclient sync has been executed
+    self.assertTrue(subprocess_call.called)
+    args, kwargs = subprocess_call.call_args
+    cmd_args = kwargs['args'] if 'args' in kwargs else args[0]
+    self.assertTrue(cmd_args[0] == run_slave.GetGClientPath() and

[dsansome, 2016/10/10 05:25:18]

Avoid using assertTrue if possible.  You'll get better error messages if you use
assertEqual, and convention is to do assertEqual(expected, actual)

self.assertEqual(run_slave.GetGClientPath(), cmd_args[0])
self.assertEqual('sync', cmd_args[1])

[ddoman1, 2016/10/12 00:42:23]

Done.

+                    cmd_args[1] == 'sync')
+
+    # verify that run_slave.py was execv()-ed with --no-gclient-sync
+    run_slave_py_path = re.sub(
+        r'pyc$', 'py', os.path.abspath(run_slave.__file__))
+
+    self.assertTrue(run_slave_py_path in MockExecv.argv[:2])
+    self.assertTrue('--no-gclient-sync' in MockExecv.argv)
+
+  def test_run_slave_with_no_gclient_sync(self):
+    """Tests if twistd.run() gets invoked when --no-gclient-sync is given."""
+    with patch('subprocess.call') as subprocess_call, \

[dsansome, 2016/10/10 05:25:18]

This is a bit long, consider using mock.patch as a decorator on the test
function instead:

@mock.patch('subprocess.check_call')
@mock.patch('...')
def test_run_slave_with_no_gclient_sync(self, mock_check_call, ...)

[ddoman1, 2016/10/12 00:42:23]

That looks much cleaner. Done

+         patch('subprocess.check_call') as subprocess_check_call, \
+         patch('subprocess.check_output') as subprocess_check_output, \
+         patch('twisted.scripts.twistd.run') as twistd_run, \
+         patch('sys.argv', [run_slave.__file__, '--no-gclient-sync']):
+        os.environ['TESTING_MASTER'] = 'Master1'

[dsansome, 2016/10/10 05:25:18]

Also 2-space indentation

[ddoman1, 2016/10/12 00:42:23]

Done.

+        os.environ['TESTING_SLAVE'] = 'Slave1'
+        runpy.run_module("run_slave", run_name="__main__", alter_sys=True)
+
+    gclient_path = run_slave.GetGClientPath()
+    for mock in (subprocess_call, subprocess_check_call,
+                 subprocess_check_output):
+      args, kwargs = mock.call_args if mock.called else ([["", ""]], {})
+      cmd_args = kwargs['args'] if 'args' in kwargs else args[0]
+      # check if gclient has not been called with sync option.
+      self.assertFalse(gclient_path == cmd_args[0] and 'sync' == args[1])

[dsansome, 2016/10/10 05:25:18]

Use assertEqual

[ddoman1, 2016/10/12 00:42:23]

Done.

+    self.assertTrue(twistd_run.called)
+
+
+if __name__ == '__main__':
+  unittest.main()