Make FakeConfiguration a representative configuration object.

As of this commit fake configuration instances are populated with
the same properties, including the same default values, as a genuine
Configuration object instance. This ensures that logic under test is
going to behave far more as it would with a real configuration object
which means a great deal more assurance in the tests.

Achieve this by using the dictionary of defaults that were split out
and made statically defined some time ago. FakeConfiguration becomes
a SimpleNamespace thereby ensuring both that normal attribute lookup
works correctly but also disallows unknown properties being attached
to configuration arbitrarily. This keeps us honest in the properties
we expose and prevents accidental additions.

Since FakeConfiguration needs to track the real Configuration the only
permissible keys are those of a genuine object. Unfortunately it seems
that beyond the specified defaults many properties are set dynamically
when loading a configuration. Lay the first steps for these objects
becoming regular by making a couple of properties used by existing
tests static; either existing defaults are re-used to avoid functional
change or properties are explicitly defined with auto load-time behaviour.
Of particular note is the use of keyword_auto for new_user_default_ui.

Finally, use the opportunity to improve the integration of the various
fake objects into test cases. The provided fake configuration will now
be passed the fake logger attached provided by the MiG testcase, which
means that it will be included in the logger message detection logic.

Author: albu-diku

mig/shared/configuration.py

@@ -37,6 +37,7 @@
 import copy
 import datetime
 import functools
+import inspect
 import os
 import pwd
 import re
@@ -74,6 +75,17 @@
     print("could not import migrid modules")
 
 
+_CONFIGURATION_NOFORWARD_KEYS = set([
+    'self',
+    'config_file',
+    'mig_server_id',
+    'disable_auth_log',
+    'skip_log',
+    'verbose',
+    'logger',
+])
+
+
 def include_section_contents(logger, config, section, load_path, verbose=False,
                              reject_overrides=[]):
     """Include additional section contents from load_path in config."""
@@ -435,6 +447,11 @@ def fix_missing(config_file, verbose=True):
         fd.close()
 
 
+def _configuraton_dict_without_noforward_keys(configuration_like):
+    return { k: v for k, v in configuration_like.__dict__.items()
+             if k not in _CONFIGURATION_NOFORWARD_KEYS }
+
+
 class NativeConfigParser(ConfigParser):
     """Wraps configparser.ConfigParser to force get method to return native
     string instead of always returning unicode.
@@ -445,7 +462,11 @@ def get(self, *args, **kwargs):
         return force_native_str(ConfigParser.get(self, *args, **kwargs))
 
 
-_CONFIGURATION_DEFAULTS = {
+# TODO: this static definition is incomplete with many properties being
+#       dynamically assigned to Configuration objects at the point they are
+#       loaded - expand these as code that makes use of those properties comes
+#       under test with the ultimate goal that this becomes exhaustive
+_CONFIGURATION_PROPERTIES = {
     # Optional conf options with default values
     'state_path': os.path.expanduser('~/state'),
     'mig_path': os.path.expanduser('~/mig'),
@@ -469,6 +490,7 @@ def get(self, *args, **kwargs):
     'ca_smtp': '',
     'ca_user': 'mig-ca',
     'resource_home': '',
+    'short_title': 'MiG',
     'vgrid_home': '',
     'vgrid_public_base': '',
     'vgrid_private_base': '',
@@ -511,6 +533,7 @@ def get(self, *args, **kwargs):
     'workflows_vgrid_patterns_home': '',
     'workflows_vgrid_recipes_home': '',
     'workflows_vgrid_history_home': '',
+    'site_user_id_format': DEFAULT_USER_ID_FORMAT,
     'site_prefer_python3': False,
     'site_autolaunch_page': '',
     'site_landing_page': '',
@@ -538,6 +561,12 @@ def get(self, *args, **kwargs):
     'site_password_policy': POLICY_MEDIUM,
     'site_password_legacy_policy': False,
     'site_password_cracklib': False,
+
+    # Salt values which are undonditionally populated on configuration load
+    'site_crypto_salt': '',
+    'site_password_salt': '',
+    'site_digest_salt': '',
+
     'site_extra_userpage_scripts': "",
     'site_extra_userpage_styles': "",
     'hg_path': '',
@@ -725,6 +754,7 @@ def get(self, *args, **kwargs):
     'expire_peer': 600,
     'language': ['English'],
     'user_interface': ['V2', 'V3'],
+    'new_user_default_ui': keyword_auto,
     'submitui': ['fields', 'textarea', 'files'],
     # Init user default page with no selection to use site landing page
     'default_page': [''],
@@ -747,6 +777,8 @@ def get(self, *args, **kwargs):
     # fyrgrid, benedict. Otherwise, ldap://bla.bla:2135/...
 
     'arc_clusters': [],
+
+    'cloud_services': [],
 }
 
 
@@ -763,9 +795,9 @@ def __init__(self, config_file, verbose=False, skip_log=False,
         self.auth_logger_obj = None
         self.gdp_logger_obj = None
 
-        configuration_options = copy.deepcopy(_CONFIGURATION_DEFAULTS)
+        configuration_properties = copy.deepcopy(_CONFIGURATION_PROPERTIES)
 
-        for k, v in configuration_options.items():
+        for k, v in configuration_properties.items():
             setattr(self, k, v)
 
         if config_file is not None:
@@ -1018,19 +1050,24 @@ def reload_config(self, verbose, skip_log=False, disable_auth_log=False,
             self.site_title = "Minimum intrusion Grid"
         if config.has_option('SITE', 'short_title'):
             self.short_title = config.get('SITE', 'short_title')
-        else:
-            self.short_title = "MiG"
+
         if config.has_option('SITE', 'user_interface'):
             self.user_interface = config.get(
                 'SITE', 'user_interface').split()
         else:
             self.user_interface = ['V2']
+
         # Allow gradual transition to new user interface - only new sign ups
         if config.has_option('SITE', 'new_user_default_ui'):
             self.new_user_default_ui = config.get(
                 'SITE', 'new_user_default_ui').strip()
-        else:
+        elif self.new_user_default_ui == keyword_auto and self.user_interface:
+            # an explicit default ui value for new users was not specified so
+            # use the first entry in supported user interfaces as a fallback
             self.new_user_default_ui = self.user_interface[0]
+        else:
+            print("No usable value supplied for default new user ui version.")
+            raise IOError
 
         if config.has_option('GLOBAL', 'state_path'):
             self.state_path = config.get('GLOBAL', 'state_path')
@@ -2034,8 +2071,6 @@ def reload_config(self, verbose, skip_log=False, disable_auth_log=False,
                 logger.warning("invalid user_id_format %r - using default" %
                                self.site_user_id_format)
                 self.site_user_id_format = DEFAULT_USER_ID_FORMAT
-        else:
-            self.site_user_id_format = DEFAULT_USER_ID_FORMAT
         if config.has_option('SITE', 'autolaunch_page'):
             self.site_autolaunch_page = config.get('SITE', 'autolaunch_page')
         else:
@@ -2863,6 +2898,21 @@ def parse_peers(self, peerfile):
                     peerfile)
         return peers_dict
 
+    @staticmethod
+    def is_configuration_like(obj):
+        """Does the given object quack like a MiG Configuration."""
+        return inspect.ismethod(getattr(obj, 'reload_config', None))
+
+    @staticmethod
+    def to_dict(obj):
+        """Return a plain dictionary of the loaded configuration values only
+        given a Configuration-like object as input."""
+        assert Configuration.is_configuration_like(obj)
+        return _configuraton_dict_without_noforward_keys(obj)
+
+
+_CONFIGURATION_ARGUMENTS = set(_CONFIGURATION_PROPERTIES.keys()) - _CONFIGURATION_NOFORWARD_KEYS
+
 
 if '__main__' == __name__:
     conf = Configuration(os.path.expanduser('~/mig/server/MiGserver.conf'),

tests/fixture/mig_shared_configuration--new.json

@@ -29,6 +29,7 @@
   "ca_smtp": "",
   "ca_user": "mig-ca",
   "certs_path": "/some/place/certs",
+  "cloud_services": [],
   "config_file": null,
   "cputime_for_empty_jobs": 0,
   "default_page": [
@@ -130,6 +131,7 @@
   "min_seconds_between_live_update_requests": 0,
   "mrsl_files_dir": "",
   "myfiles_py_location": "",
+  "new_user_default_ui": "AUTO",
   "notify_home": "",
   "openid_store": "",
   "passphrase_file": "",
@@ -153,6 +155,7 @@
   "sessid_to_jupyter_mount_link_home": "",
   "sessid_to_mrsl_link_home": "",
   "sharelink_home": "",
+  "short_title": "MiG",
   "site_advanced_vgrid_links": [],
   "site_autolaunch_page": "",
   "site_cloud_access": [
@@ -173,6 +176,9 @@
   "site_password_cracklib": false,
   "site_password_legacy_policy": false,
   "site_password_policy": "MEDIUM",
+  "site_crypto_salt": "",
+  "site_password_salt": "",
+  "site_digest_salt": "",
   "site_peers_notice": "",
   "site_peers_permit": [
     [
@@ -198,6 +204,7 @@
     ]
   ],
   "site_skin": "",
+  "site_user_id_format": "X509",
   "site_vgrid_creators": [
     [
       "distinguished_name",

tests/support/__init__.py

@@ -218,9 +218,9 @@ def _reset_logging(self, stream):
     # testcase defaults
 
     @staticmethod
-    def _make_configuration_instance(configuration_to_make):
+    def _make_configuration_instance(testcase, configuration_to_make):
         if configuration_to_make == 'fakeconfig':
-            return FakeConfiguration()
+            return FakeConfiguration(logger=testcase.logger)
         elif configuration_to_make == 'testconfig':
             from mig.shared.conf import get_configuration_object
             return get_configuration_object(skip_log=True, disable_auth_log=True)
@@ -229,7 +229,7 @@ def _make_configuration_instance(configuration_to_make):
                 "MigTestCase: unknown configuration %r" % (configuration_to_make,))
 
     def _provide_configuration(self):
-        return 'fakeconfig'
+        return 'unspecified'
 
     @property
     def configuration(self):
@@ -239,8 +239,13 @@ def configuration(self):
             return self._configuration
 
         configuration_to_make = self._provide_configuration()
+
+        if configuration_to_make == 'unspecified':
+            raise AssertionError(
+                "configuration access but testcase did not request it")
+
         configuration_instance = self._make_configuration_instance(
-            configuration_to_make)
+            self, configuration_to_make)
 
         if configuration_to_make == 'testconfig':
             # use the paths defined by the loaded configuration to create

tests/support/configsupp.py

@@ -29,19 +29,61 @@
 
 from tests.support.loggersupp import FakeLogger
 
+from mig.shared.compat import SimpleNamespace
+from mig.shared.configuration import \
+    _CONFIGURATION_ARGUMENTS, _CONFIGURATION_PROPERTIES
+
+
+def _ensure_only_configuration_keys(thedict):
+    """Check a dictionary contains only keys valid as Configuration properties.
+    """
+
+    unknown_keys = set(thedict.keys()) - set(_CONFIGURATION_ARGUMENTS)
+    assert len(unknown_keys) == 0, \
+        "non-Configuration keys: %s" % (', '.join(unknown_keys),)
+
+
+def _generate_namespace_kwargs():
+    """Create plain dictionary with supported properties and keys that map to
+    their default values suitable for use in fabricating a namespace.
+    """
+
+    properties_and_defaults = dict(_CONFIGURATION_PROPERTIES)
+    properties_and_defaults['logger'] = None
+    return properties_and_defaults
+
+
+class FakeConfiguration(SimpleNamespace):
+    """An object that can act as a representative Configuration which can be
+    programmed with particular values required to exercise code under test.
+
+    This object will track standard values as would be present on a genuine
+    Configuration instance such that code under test expecting such can be
+    handed something. The defaults are overlaid by any explicit keyword args.
 
-class FakeConfiguration:
-    """A simple helper to pretend we have a real Configuration object with any
-    required attributes explicitly passed.
     Automatically attaches a FakeLogger instance if no logger is provided in
     kwargs.
     """
 
-    def __init__(self, **kwargs):
-        """Initialise instance attributes to be any named args provided and a
-        FakeLogger instance attached if not provided.
+    def __init__(self, logger=None, **kwargs):
+        """Initialise instance attributes based on the defaults plus any
+        supplied additional options.
         """
-        self.__dict__.update(kwargs)
-        if not 'logger' in self.__dict__:
-            dummy_logger = FakeLogger()
-            self.__dict__.update({'logger': dummy_logger})
+
+        SimpleNamespace.__init__(self, **_generate_namespace_kwargs())
+
+        if logger is None:
+            # TODO: remove this conditional once all callers that require a
+            #       FakeConfiguration request it via _provide_configuration()
+            logger = FakeLogger()
+        self.logger = logger
+
+        if kwargs:
+            _ensure_only_configuration_keys(kwargs)
+            for k, v in kwargs.items():
+                setattr(self, k, v)
+
+    def reload_config(self, *args, **kwargs):
+        """Stub defined to quack like Configuration."""
+
+        pass

tests/test_mig_lib_janitor.py

@@ -37,6 +37,9 @@
 class MigLibJanitor(MigTestCase):
     """Unit tests for janitor related helper functions"""
 
+    def _provide_configuration(self):
+        return 'fakeconfig'
+
     def test_last_run_bookkeeping(self):
         """Register a last run timestamp and check it"""
         expect = -1

tests/test_mig_shared_configuration.py

@@ -33,20 +33,38 @@
 
 from tests.support import MigTestCase, TEST_DATA_DIR, PY2, testmain, \
     fixturefile
-from mig.shared.configuration import Configuration
-
-
-def _is_method(value):
-    return type(value).__name__ == 'method'
+from mig.shared.configuration import Configuration, \
+    _CONFIGURATION_ARGUMENTS, _CONFIGURATION_PROPERTIES
 
 
 def _to_dict(obj):
     return {k: v for k, v in inspect.getmembers(obj)
-            if not (k.startswith('__') or _is_method(v))}
+            if not (k.startswith('__') or inspect.ismethod(v) or inspect.isfunction(v))}
+
+
+class MigSharedConfiguration__static_definitions(MigTestCase):
+    """Coverage of the static definitions underlying Configuration objects."""
+
+    def test_consistent_parameters(self):
+        configuration_defaults_keys = set(_CONFIGURATION_PROPERTIES.keys())
+        mismatched = _CONFIGURATION_ARGUMENTS - configuration_defaults_keys
+
+        self.assertEqual(len(mismatched), 0,
+                         "configuration defaults do not match arguments")
+
+
+class MigSharedConfiguration__loaded_configurations(MigTestCase):
+    """Coverage of loaded Configuration instances."""
+
 
+    def test_argument_new_user_default_ui_is_replaced(self):
+        test_conf_file = os.path.join(
+            TEST_DATA_DIR, 'MiGserver--customised.conf')
 
-class MigSharedConfiguration(MigTestCase):
-    """Wrap unit tests for the corresponding module"""
+        configuration = Configuration(
+            test_conf_file, skip_log=True, disable_auth_log=True)
+
+        self.assertEqual(configuration.new_user_default_ui, 'V3')
 
     def test_argument_storage_protocols(self):
         test_conf_file = os.path.join(
@@ -61,6 +79,7 @@ def test_argument_storage_protocols(self):
         # self.assertEqual(configuration.storage_protocols, ['sftp'])
         self.assertEqual(configuration.storage_protocols, [])
 
+
     def test_argument_wwwserve_max_bytes(self):
         test_conf_file = os.path.join(
             TEST_DATA_DIR, 'MiGserver--customised.conf')
@@ -314,6 +333,10 @@ def test_argument_include_sections_multi_ignores_other_sections(self):
         # TODO: rename file to valid section name we can check and enable next?
         # self.assertEqual(configuration.multi, 'blabla')
 
+
+class MigSharedConfiguration__new_instance(MigTestCase):
+    """Coverage of programatically created Configuration instances."""
+
     @unittest.skipIf(PY2, "Python 3 only")
     def test_default_object(self):
         prepared_fixture = self.prepareFixtureAssert(