Integrate suggestions from ajkavanagh

zaza/openstack/charm_tests/keystone/tests.py

@@ -17,7 +17,7 @@ import collections
 import json
 import logging
 import pprint
-import tenacity
+import unittest
 import keystoneauth1
 
 import zaza.model
@@ -145,11 +145,11 @@ class CharmOperationTest(BaseKeystoneTest):
                 if unit_repo != lead_repo:
                     raise zaza_exceptions.KeystoneKeyRepositoryError(
                         'expect: "{}" actual({}): "{}"'
-                        ''.format(pprint.pformat(lead_repo), unit.entity_id,
-                                  pprint.pformat(unit_repo)))
+                        .format(pprint.pformat(lead_repo), unit.entity_id,
+                                pprint.pformat(unit_repo)))
                 logging.info('"{}" == "{}"'
-                             ''.format(pprint.pformat(unit_repo),
-                                       pprint.pformat(lead_repo)))
+                             .format(pprint.pformat(unit_repo),
+                                     pprint.pformat(lead_repo)))
 
 
 class AuthenticationAuthorizationTest(BaseKeystoneTest):
@@ -258,8 +258,7 @@ class AuthenticationAuthorizationTest(BaseKeystoneTest):
         logging.info('OK')
 
     def test_end_user_access_and_token(self):
-        """
-        Verify regular end-user access resources and validate token data.
+        """Verify regular end-user access resources and validate token data.
 
         In effect this also validates user creation, presence of standard
         roles (`_member_`, `Member`), effect of policy and configuration
@@ -280,7 +279,7 @@ class AuthenticationAuthorizationTest(BaseKeystoneTest):
                 if len(token) != 32:
                     raise zaza_exceptions.KeystoneWrongTokenProvider(
                         'We expected a UUID token and got this: "{}"'
-                        ''.format(token))
+                        .format(token))
             else:
                 if len(token) < 180:
                     raise zaza_exceptions.KeystoneWrongTokenProvider(
@@ -382,36 +381,19 @@ class LdapTests(BaseKeystoneTest):
         """Run class setup for running Keystone ldap-tests."""
         super(LdapTests, cls).setUpClass()
 
-    def _get_ldap_config(self):
+    @staticmethod
+    def _get_ldap_config():
         ldap_ips = zaza.model.get_app_ips("ldap-server")
-        ldap_server_ip = ldap_ips[0]
-
-        keystone_ldap_config = {
-            'ldap-server': "ldap://{}".format(ldap_server_ip),
-            'ldap-user': 'cn=admin,dc=test,dc=com',
-            'ldap-password': 'crapper',
-            'ldap-suffix': 'dc=test,dc=com',
-            'domain-name': 'userdomain',
-        }
-        if all(keystone_ldap_config.values()):
-            self.ldap_configured = True
-            return keystone_ldap_config
-        else:
-            # NOTE(jamespage): Use mock values to check deployment only
-            #                  as no test fixture has been supplied
-            self.ldap_configured = False
-            return {
-                'ldap-server': 'myserver',
-                'ldap-user': 'myuser',
-                'ldap-password': 'mypassword',
-                'ldap-suffix': 'mysuffix',
+        if ldap_ips:
+            return True, {
+                'ldap-server': "ldap://{}".format(ldap_ips[0]),
+                'ldap-user': 'cn=admin,dc=test,dc=com',
+                'ldap-password': 'crapper',
+                'ldap-suffix': 'dc=test,dc=com',
                 'domain-name': 'userdomain',
             }
+        return False, {}
 
-    # NOTE: intermittent authentication fails. Wrap in a retry loop.
-    @tenacity.retry(wait=tenacity.wait_exponential(multiplier=1,
-                                                   min=5, max=10),
-                    reraise=True)
     def _find_keystone_v3_user(self, username, domain):
         """Find a user within a specified keystone v3 domain."""
         client = self.admin_keystone_client
@@ -423,36 +405,35 @@ class LdapTests(BaseKeystoneTest):
             usernames.append(user.name)
             if username.lower() == user.name.lower():
                 return user
-        logging.debug("User {} was not in these users: {}. Returning None."
-                      "".format(username, usernames))
+        logging.debug(
+            "User {} was not in these users: {}. Returning None."
+            .format(username, usernames)
+        )
         return None
 
     def test_100_keystone_ldap_users(self):
         """Validate basic functionality of keystone API with ldap."""
-        zaza.model.set_application_config('keystone-ldap',
-                                          self._get_ldap_config())
+        application_name = 'keystone-ldap'
+        can_config, config = self._get_ldap_config()
+        if not can_config:
+            raise unittest.SkipTest("Skipping API tests as no LDAP test fixture")
 
-        if not self.ldap_configured:
-            msg = 'Skipping API tests as no LDAP test fixture'
-            logging.info(msg)
-            return
-
-        logging.info('Waiting for ldap config-changes to complete...')
-        states = {
-            'keystone-ldap': {
-                'workload-status': 'idle',
-                'workload-status-messages': 'Unit is ready'
-            },
-            'keystone': {
-                'workload-status': 'idle',
-                'workload-status-messages': 'Unit is ready'
+        with self.config_change(
+                self.config_current(application_name),
+                config,
+                application_name=application_name):
+            logging.info('Waiting for users to become available in keystone...')
+            states = {
+                'keystone': {
+                    'workload-status': 'idle',
+                    'workload-status-messages': 'Unit is ready'
+                }
             }
-        }
-        zaza.model.wait_for_application_states(states=states)
+            zaza.model.wait_for_application_states(states=states)
 
-        # NOTE(jamespage): Test fixture should have johndoe and janedoe
-        #                  accounts
-        johndoe = self._find_keystone_v3_user('john doe', 'userdomain')
-        assert johndoe is not None
-        janedoe = self._find_keystone_v3_user('jane doe', 'userdomain')
-        assert janedoe is not None
+            # NOTE(jamespage): Test fixture should have johndoe and janedoe
+            #                  accounts
+            johndoe = self._find_keystone_v3_user('john doe', 'userdomain')
+            self.assertIsNotNone(johndoe, "user 'john doe' was unknown")
+            janedoe = self._find_keystone_v3_user('jane doe', 'userdomain')
+            self.assertIsNotNone(janedoe, "user 'jane doe' was unknown")

zaza/openstack/charm_tests/test_utils.py

@@ -133,6 +133,29 @@ class OpenStackBaseTest(unittest.TestCase):
             model_name=cls.model_name)
         logging.debug('Leader unit is {}'.format(cls.lead_unit))
 
+    def config_current(self, application_name=None, keys=None):
+        """Get Current Config of an application normalized into key-values
+
+        :param application_name: String application name for use when called
+                                 by a charm under test other than the object's
+                                 application.
+        :type application_name: str
+        :param keys: iterable of strs to index into the current config.  If
+                     None, return all keys from the config
+        :type keys: Union[iterable[str], None]
+        """
+        if not application_name:
+            application_name = self.application_name
+        _app_config = model.get_application_config(application_name)
+        # convert the more elaborate config structure from libjuju to key-value
+        keys = keys or _app_config.keys()
+        # note that conversion to string for all values is due to
+        # attempting to set any config with other types lead to Traceback
+        return {
+            str(k): str(_app_config.get(k, {}).get('value', ''))
+            for k in keys
+        }
+
     @contextlib.contextmanager
     def config_change(self, default_config, alternate_config,
                       application_name=None):
@@ -158,17 +181,12 @@ class OpenStackBaseTest(unittest.TestCase):
         """
         if not application_name:
             application_name = self.application_name
+
         # we need to compare config values to what is already applied before
         # attempting to set them.  otherwise the model will behave differently
         # than we would expect while waiting for completion of the change
-        _app_config = model.get_application_config(application_name)
-        app_config = {}
-        # convert the more elaborate config structure from libjuju to something
-        # we can compare to what the caller supplies to this function
-        for k in alternate_config.keys():
-            # note that conversion to string for all values is due to
-            # attempting to set any config with other types lead to Traceback
-            app_config[k] = str(_app_config.get(k, {}).get('value', ''))
+        app_config = self.config_current(application_name, keys=alternate_config.keys())
+
         if all(item in app_config.items()
                 for item in alternate_config.items()):
             logging.debug('alternate_config equals what is already applied '