Merge lp:~adam-collard/charm-helpers/amulet-get-process-id-list-fixes into lp:charm-helpers

[Adam Collard]

Extend get_process_id and get_unit_process_ids to allow them to be used for testing that processes are /not/ running.

[Chris Glass (tribaal)]

Thanks for your branch. On principle it looks good, but some tests fail and others clutter stdout/stderr on "make test".

I'm going to have to mark this as need fixing until "make test" produces a clean (and passing) output.

review: Needs Fixing

[lp:~adam-collard/charm-helpers/amulet-get-process-id-list-fixes]

431. By Adam Collard on 2015-08-17

Use six.iteritems() to fix Py3 test failure (tribaal's review)

432. By Adam Collard on 2015-08-17

Capture stdout/stderr around tests that expect to run amulet.raise_status and verify the message

[Adam Collard (adam-collard)]

> Thanks for your branch. On principle it looks good, but some tests fail and
> others clutter stdout/stderr on "make test".

Thanks for the review. Sorry for not running the full test suite before proposing.

The extra tests I added exposed existing failures in the amulet utils when running under Python 3. I've fixed those, and added code to capture stdout.

> I'm going to have to mark this as need fixing until "make test" produces a
> clean (and passing) output.

Please take another look, believed to have been fixed now!

[Chris Glass (tribaal)]

Yep, looks good now, thanks.

+1

review: Approve

=== modified file 'charmhelpers/contrib/amulet/utils.py'
--- charmhelpers/contrib/amulet/utils.py	2015-08-14 17:09:52 +0000
+++ charmhelpers/contrib/amulet/utils.py	2015-08-17 11:03:41 +0000
@@ -460,15 +460,20 @@
                                         cmd, code, output))
         return None
 
-    def get_process_id_list(self, sentry_unit, process_name):
+    def get_process_id_list(self, sentry_unit, process_name,
+                            expect_success=True):
         """Get a list of process ID(s) from a single sentry juju unit
         for a single process name.
 
-        :param sentry_unit: Pointer to amulet sentry instance (juju unit)
+        :param sentry_unit: Amulet sentry instance (juju unit)
         :param process_name: Process name
+        :param expect_success: If False, expect the PID to be missing,
+            raise if it is present.
         :returns: List of process IDs
         """
-        cmd = 'pidof {}'.format(process_name)
+        cmd = 'pidof -x {}'.format(process_name)
+        if not expect_success:
+            cmd += " || exit 0 && exit 1"
         output, code = sentry_unit.run(cmd)
         if code != 0:
             msg = ('{} `{}` returned {} '
@@ -477,14 +482,23 @@
             amulet.raise_status(amulet.FAIL, msg=msg)
         return str(output).split()
 
-    def get_unit_process_ids(self, unit_processes):
+    def get_unit_process_ids(self, unit_processes, expect_success=True):
         """Construct a dict containing unit sentries, process names, and
-        process IDs."""
+        process IDs.
+
+        :param unit_processes: A dictionary of Amulet sentry instance
+            to list of process names.
+        :param expect_success: if False expect the processes to not be
+            running, raise if they are.
+        :returns: Dictionary of Amulet sentry instance to dictionary
+            of process names to PIDs.
+        """
         pid_dict = {}
-        for sentry_unit, process_list in unit_processes.iteritems():
+        for sentry_unit, process_list in six.iteritems(unit_processes):
             pid_dict[sentry_unit] = {}
             for process in process_list:
-                pids = self.get_process_id_list(sentry_unit, process)
+                pids = self.get_process_id_list(
+                    sentry_unit, process, expect_success=expect_success)
                 pid_dict[sentry_unit].update({process: pids})
         return pid_dict
 
@@ -498,7 +512,7 @@
             return ('Unit count mismatch.  expected, actual: {}, '
                     '{} '.format(len(expected), len(actual)))
 
-        for (e_sentry, e_proc_names) in expected.iteritems():
+        for (e_sentry, e_proc_names) in six.iteritems(expected):
             e_sentry_name = e_sentry.info['unit_name']
             if e_sentry in actual.keys():
                 a_proc_names = actual[e_sentry]

=== modified file 'tests/contrib/amulet/test_utils.py'
--- tests/contrib/amulet/test_utils.py	2015-08-14 17:09:52 +0000
+++ tests/contrib/amulet/test_utils.py	2015-08-17 11:03:41 +0000
@@ -3,16 +3,35 @@
 # Authors:
 #  Adam Collard <[e-mail]>
 
+from contextlib import contextmanager
+import sys
 import unittest
 
+import six
+
 from charmhelpers.contrib.amulet.utils import AmuletUtils
 
 
+@contextmanager
+def captured_output():
+    """Simple context manager to capture stdout/stderr.
+
+    Source: http://stackoverflow.com/a/17981937/56219.
+    """
+    new_out, new_err = six.StringIO(), six.StringIO()
+    old_out, old_err = sys.stdout, sys.stderr
+    try:
+        sys.stdout, sys.stderr = new_out, new_err
+        yield sys.stdout, sys.stderr
+    finally:
+        sys.stdout, sys.stderr = old_out, old_err
+
+
 class FakeSentry(object):
 
-    commands = {}
-
-    info = {"unit_name": "foo"}
+    def __init__(self, name="foo"):
+        self.commands = {}
+        self.info = {"unit_name": name}
 
     def run(self, command):
         return self.commands[command]
@@ -193,3 +212,99 @@
 
         self.assertFalse(self.utils.wait_on_action(
             "action-id", _check_output=fake_check_output))
+
+
+class GetProcessIdListTestCase(unittest.TestCase):
+
+    def setUp(self):
+        self.utils = AmuletUtils()
+        self.sentry_unit = FakeSentry()
+
+    def test_returns_pids(self):
+        """
+        Normal execution returns a list of pids
+        """
+        self.sentry_unit.commands["pidof -x foo"] = ("123 124 125", 0)
+        result = self.utils.get_process_id_list(self.sentry_unit, "foo")
+        self.assertEqual(["123", "124", "125"], result)
+
+    def test_fails_if_no_process_found(self):
+        """
+        By default, the expectation is that a process is running. Failure
+        to find a given process results in an amulet.FAIL being
+        raised.
+        """
+        self.sentry_unit.commands["pidof -x foo"] = ("", 1)
+        with self.assertRaises(SystemExit) as cm, captured_output() as (
+                out, err):
+            self.utils.get_process_id_list(self.sentry_unit, "foo")
+        the_exception = cm.exception
+        self.assertEqual(1, the_exception.code)
+        self.assertEqual(
+            "foo `pidof -x foo` returned 1", out.getvalue().rstrip())
+
+    def test_looks_for_scripts(self):
+        """
+        pidof command uses -x to return a list of pids of scripts
+        """
+        self.sentry_unit.commands["pidof foo"] = ("", 1)
+        self.sentry_unit.commands["pidof -x foo"] = ("123 124 125", 0)
+        result = self.utils.get_process_id_list(self.sentry_unit, "foo")
+        self.assertEqual(["123", "124", "125"], result)
+
+    def test_expect_no_pid(self):
+        """
+        By setting expectation that there are no pids running the logic
+        about when to fail is reversed.
+        """
+        self.sentry_unit.commands["pidof -x foo || exit 0 && exit 1"] = ("", 0)
+        self.sentry_unit.commands["pidof -x bar || exit 0 && exit 1"] = ("", 1)
+        result = self.utils.get_process_id_list(
+            self.sentry_unit, "foo", expect_success=False)
+        self.assertEqual([], result)
+        with self.assertRaises(SystemExit) as cm, captured_output() as (
+                out, err):
+            self.utils.get_process_id_list(
+                self.sentry_unit, "bar", expect_success=False)
+        the_exception = cm.exception
+        self.assertEqual(1, the_exception.code)
+        self.assertEqual(
+            "foo `pidof -x bar || exit 0 && exit 1` returned 1",
+            out.getvalue().rstrip())
+
+
+class GetUnitProcessIdsTestCase(unittest.TestCase):
+
+    def setUp(self):
+        self.utils = AmuletUtils()
+        self.sentry_unit = FakeSentry()
+
+    def test_returns_map(self):
+        """
+        Normal execution returns a dictionary mapping process names to
+        PIDs for each unit.
+        """
+        second_sentry = FakeSentry(name="bar")
+        self.sentry_unit.commands["pidof -x foo"] = ("123 124", 0)
+        second_sentry.commands["pidof -x bar"] = ("456 457", 0)
+
+        result = self.utils.get_unit_process_ids({
+            self.sentry_unit: ["foo"], second_sentry: ["bar"]})
+        self.assertEqual({
+            self.sentry_unit: {"foo": ["123", "124"]},
+            second_sentry: {"bar": ["456", "457"]}}, result)
+
+    def test_expect_failure(self):
+        """
+        Expected failures return empty lists.
+        """
+        second_sentry = FakeSentry(name="bar")
+        self.sentry_unit.commands["pidof -x foo || exit 0 && exit 1"] = ("", 0)
+        second_sentry.commands["pidof -x bar || exit 0 && exit 1"] = ("", 0)
+
+        result = self.utils.get_unit_process_ids(
+            {self.sentry_unit: ["foo"], second_sentry: ["bar"]},
+            expect_success=False)
+        self.assertEqual({
+            self.sentry_unit: {"foo": []},
+            second_sentry: {"bar": []}}, result)