Merge lp:~chris.macnaughton/charm-helpers/pids-can-be-a-list into lp:charm-helpers

[Chris MacNaughton]

Allow the validate_unit_process_ids function to accept a list of valid values

[Ryan Beisner (1chb1n)]

1. See inline comments. That existing validation block is already particularly non-beautiful ;-) and the commentary helps clarify. Let's keep that consistent in that block.

2. The variable name e_pids_length (expected process IDs length) makes sense for the existing validation blocks, but it is confusing/mis-named in the context of it being a list type. Ideas to address that?

3. I'd like to see this validated before merge by syncing this c-h LP branch into ceph-mon, then updating basic_deployment.py like so:

<icey> a ceph-mon test
<icey> looking...
<icey> beisner: https://review.openstack.org/#/c/287446/7/tests/basic_deployment.py
<icey> line 185, changed from {'ceph-osd': 2} to {'ceph-osd': True}
<icey> would rather be: {'ceph-osd': [2, 3]}

4. And finally by running the test, capturing and commenting here with a pastebin of the test
output.

review: Needs Fixing

[Ryan Beisner (1chb1n)]

4. (amulet) test that is.

[Ryan Beisner (1chb1n)]

5. Please also add a simple 1-liner docstring to each new unit test case so that when nosetests are run verbosely, we see that output.

review: Needs Fixing

[Ryan Beisner (1chb1n)]

Ha! Never mind 5. You've already done that. I just can't read apparently.

[lp:~chris.macnaughton/charm-helpers/pids-can-be-a-list]

537. By Chris MacNaughton on 2016-03-15

review updates

[Chris MacNaughton (chris.macnaughton)]

Do you still want to see the changes against ceph-osd Ryan, or are the test cases enough to make you comfortable?

[Ryan Beisner (1chb1n)]

Thanks for the unit test coverage on this. That shows that the behavior is as intended. If we are to omit a sync/amulet test, we should also add negative unit tests here, where you pass bad scenarios and assert that the validation method has returned something other than None.

Can you add 1-per? That would make this rock solid.

review: Needs Information

[lp:~chris.macnaughton/charm-helpers/pids-can-be-a-list]

538. By Chris MacNaughton on 2016-04-05

add negative proofs

[Ryan Beisner (1chb1n)]

I'm seeing test_accepts_bool_wrong & test_accepts_string_wrong fail in this rev (fresh venv, via `make test`).

review: Needs Fixing

[Ryan Beisner (1chb1n)]

Please update the *_wrong docstrings to reflect the negative aspect.

Also, I believe the *_wrong assertions should be IsNotNone.

review: Needs Fixing

[lp:~chris.macnaughton/charm-helpers/pids-can-be-a-list]

539. By Chris MacNaughton on 2016-04-05

fix test

[Ryan Beisner (1chb1n)]

Thanks for your work on this. Looks good!

review: Approve

=== modified file 'charmhelpers/contrib/amulet/utils.py'
--- charmhelpers/contrib/amulet/utils.py	2016-03-18 19:08:25 +0000
+++ charmhelpers/contrib/amulet/utils.py	2016-04-05 21:46:27 +0000
@@ -601,7 +601,7 @@
                 return ('Process name count mismatch.  expected, actual: {}, '
                         '{}'.format(len(expected), len(actual)))
 
-            for (e_proc_name, e_pids_length), (a_proc_name, a_pids) in \
+            for (e_proc_name, e_pids), (a_proc_name, a_pids) in \
                     zip(e_proc_names.items(), a_proc_names.items()):
                 if e_proc_name != a_proc_name:
                     return ('Process name mismatch.  expected, actual: {}, '
@@ -610,25 +610,30 @@
                 a_pids_length = len(a_pids)
                 fail_msg = ('PID count mismatch. {} ({}) expected, actual: '
                             '{}, {} ({})'.format(e_sentry_name, e_proc_name,
-                                                 e_pids_length, a_pids_length,
+                                                 e_pids, a_pids_length,
                                                  a_pids))
 
-                # If expected is not bool, ensure PID quantities match
-                if not isinstance(e_pids_length, bool) and \
-                        a_pids_length != e_pids_length:
+                # If expected is a list, ensure at least one PID quantity match
+                if isinstance(e_pids, list) and \
+                        a_pids_length not in e_pids:
+                    return fail_msg
+                # If expected is not bool and not list, ensure PID quantities match
+                elif not isinstance(e_pids, bool) and \
+                        not isinstance(e_pids, list) and \
+                        a_pids_length != e_pids:
                     return fail_msg
                 # If expected is bool True, ensure 1 or more PIDs exist
-                elif isinstance(e_pids_length, bool) and \
-                        e_pids_length is True and a_pids_length < 1:
+                elif isinstance(e_pids, bool) and \
+                        e_pids is True and a_pids_length < 1:
                     return fail_msg
                 # If expected is bool False, ensure 0 PIDs exist
-                elif isinstance(e_pids_length, bool) and \
-                        e_pids_length is False and a_pids_length != 0:
+                elif isinstance(e_pids, bool) and \
+                        e_pids is False and a_pids_length != 0:
                     return fail_msg
                 else:
                     self.log.debug('PID check OK: {} {} {}: '
                                    '{}'.format(e_sentry_name, e_proc_name,
-                                               e_pids_length, a_pids))
+                                               e_pids, a_pids))
         return None
 
     def validate_list_of_identical_dicts(self, list_of_dicts):
 
=== modified file 'tests/contrib/amulet/test_utils.py'
--- tests/contrib/amulet/test_utils.py	2015-09-09 12:03:14 +0000
+++ tests/contrib/amulet/test_utils.py	2016-04-05 21:46:27 +0000
@@ -334,3 +334,64 @@
             "status-get: command not found", 127)
         self.assertEqual(self.utils.status_get(self.sentry_unit),
                          (u"unknown", u""))
+
+
+class ValidateServicesByProcessIDTestCase(unittest.TestCase):
+
+    def setUp(self):
+        self.utils = AmuletUtils()
+        self.sentry_unit = FakeSentry()
+
+    def test_accepts_list_wrong(self):
+        """
+        Validates that it can accept a list
+        """
+        expected = {self.sentry_unit: {"foo": [3, 4]}}
+        actual = {self.sentry_unit: {"foo": [12345, 67890]}}
+        result = self.utils.validate_unit_process_ids(expected, actual)
+        self.assertIsNotNone(result)
+
+    def test_accepts_list(self):
+        """
+        Validates that it can accept a list
+        """
+        expected = {self.sentry_unit: {"foo": [2, 3]}}
+        actual = {self.sentry_unit: {"foo": [12345, 67890]}}
+        result = self.utils.validate_unit_process_ids(expected, actual)
+        self.assertIsNone(result)
+
+    def test_accepts_string(self):
+        """
+        Validates that it can accept a string
+        """
+        expected = {self.sentry_unit: {"foo": 2}}
+        actual = {self.sentry_unit: {"foo": [12345, 67890]}}
+        result = self.utils.validate_unit_process_ids(expected, actual)
+        self.assertIsNone(result)
+
+    def test_accepts_string_wrong(self):
+        """
+        Validates that it can accept a string
+        """
+        expected = {self.sentry_unit: {"foo": 3}}
+        actual = {self.sentry_unit: {"foo": [12345, 67890]}}
+        result = self.utils.validate_unit_process_ids(expected, actual)
+        self.assertIsNotNone(result)
+
+    def test_accepts_bool(self):
+        """
+        Validates that it can accept a boolean
+        """
+        expected = {self.sentry_unit: {"foo": True}}
+        actual = {self.sentry_unit: {"foo": [12345, 67890]}}
+        result = self.utils.validate_unit_process_ids(expected, actual)
+        self.assertIsNone(result)
+
+    def test_accepts_bool_wrong(self):
+        """
+        Validates that it can accept a boolean
+        """
+        expected = {self.sentry_unit: {"foo": True}}
+        actual = {self.sentry_unit: {"foo": []}}
+        result = self.utils.validate_unit_process_ids(expected, actual)
+        self.assertIsNotNone(result)