From bcee161b3d0303e0bea89b1058d21861253a1231 Mon Sep 17 00:00:00 2001 From: Alex Burke Date: Fri, 4 Apr 2025 11:37:30 +0200 Subject: [PATCH] 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. --- mig/shared/configuration.py | 66 ++++++++++++++++--- .../mig_shared_configuration--new.json | 7 ++ tests/support/__init__.py | 13 ++-- tests/support/configsupp.py | 62 ++++++++++++++--- tests/test_mig_shared_configuration.py | 37 ++++++++--- tests/test_support.py | 40 +++++++---- tests/test_tests_support_configsupp.py | 55 ++++++++++++++++ 7 files changed, 237 insertions(+), 43 deletions(-) create mode 100644 tests/test_tests_support_configsupp.py diff --git a/mig/shared/configuration.py b/mig/shared/configuration.py index d69484e3a..8dfaf4bce 100644 --- a/mig/shared/configuration.py +++ b/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.""" @@ -436,6 +448,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. @@ -446,7 +463,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'), @@ -470,6 +491,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': '', @@ -512,6 +534,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': '', @@ -539,6 +562,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': '', @@ -726,6 +755,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': [''], @@ -742,6 +772,8 @@ def get(self, *args, **kwargs): 'auto_add_user_with_peer': [('distinguished_name', '.*')], 'auto_add_filter_method': '', 'auto_add_filter_fields': [], + + 'cloud_services': [], } @@ -758,9 +790,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: @@ -1013,19 +1045,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') @@ -2035,8 +2072,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: @@ -2865,6 +2900,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'), diff --git a/tests/fixture/mig_shared_configuration--new.json b/tests/fixture/mig_shared_configuration--new.json index 611410309..bed1cc598 100644 --- a/tests/fixture/mig_shared_configuration--new.json +++ b/tests/fixture/mig_shared_configuration--new.json @@ -28,6 +28,7 @@ "ca_smtp": "", "ca_user": "mig-ca", "certs_path": "/some/place/certs", + "cloud_services": [], "config_file": null, "cputime_for_empty_jobs": 0, "default_page": [ @@ -129,6 +130,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": "", @@ -152,6 +154,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": [ @@ -172,6 +175,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": [ [ @@ -197,6 +203,7 @@ ] ], "site_skin": "", + "site_user_id_format": "X509", "site_vgrid_creators": [ [ "distinguished_name", diff --git a/tests/support/__init__.py b/tests/support/__init__.py index 62d352a2b..b3c60ae62 100644 --- a/tests/support/__init__.py +++ b/tests/support/__init__.py @@ -219,9 +219,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) @@ -230,7 +230,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): @@ -240,8 +240,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 diff --git a/tests/support/configsupp.py b/tests/support/configsupp.py index a1c5c700a..0846e465d 100644 --- a/tests/support/configsupp.py +++ b/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 diff --git a/tests/test_mig_shared_configuration.py b/tests/test_mig_shared_configuration.py index bda302ed5..1108ea3e0 100644 --- a/tests/test_mig_shared_configuration.py +++ b/tests/test_mig_shared_configuration.py @@ -34,20 +34,37 @@ from tests.support import MigTestCase, TEST_DATA_DIR, PY2, testmain from tests.support.fixturesupp import FixtureAssertMixin -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') + + configuration = Configuration( + test_conf_file, skip_log=True, disable_auth_log=True) -class MigSharedConfiguration(MigTestCase, FixtureAssertMixin): - """Wrap unit tests for the corresponding module""" + self.assertEqual(configuration.new_user_default_ui, 'V3') def test_argument_storage_protocols(self): test_conf_file = os.path.join( @@ -315,6 +332,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, FixtureAssertMixin): + """Coverage of programatically created Configuration instances.""" + @unittest.skipIf(PY2, "Python 3 only") def test_default_object(self): prepared_fixture = self.prepareFixtureAssert( diff --git a/tests/test_support.py b/tests/test_support.py index 7ce5097d0..56b74f594 100644 --- a/tests/test_support.py +++ b/tests/test_support.py @@ -67,7 +67,7 @@ def _wrapped_check_callable(): class SupportTestCase(MigTestCase): - """Coverage of base Support helpers""" + """Coverage of the basic behaviour of a MiG Testcase""" def _class_attribute(self, name, **kwargs): cls = type(self) @@ -76,16 +76,12 @@ def _class_attribute(self, name, **kwargs): else: return getattr(cls, name, None) - def test_provides_a_fake_configuration(self): - configuration = self.configuration - - self.assertIsInstance(configuration, FakeConfiguration) - - def test_provides_a_fake_configuration_for_the_duration_of_the_test(self): - c1 = self.configuration - c2 = self.configuration - - self.assertIs(c2, c1) + def test_requires_requesting_a_configuration(self): + with self.assertRaises(AssertionError) as raised: + self.configuration + theexception = raised.exception + self.assertEqual(str(theexception), + "configuration access but testcase did not request it") @unittest.skipIf(PY2, "Python 3 only") def test_unclosed_files_are_recorded(self): @@ -139,8 +135,26 @@ def test_when_asserting_over_multiple_values_after(self): self.assertTrue(attempt_wrapper.was_check_callable_called()) -class SupportTestCase_overridden_configuration(MigTestCase): - """Coverage of base Support helpers extension with configuration override""" +class SupportTestCase_using_fakeconfig(MigTestCase): + """Coverage of a MiG Testcase hat requests a fakeconfig""" + + def _provide_configuration(self): + return 'fakeconfig' + + def test_provides_a_fake_configuration(self): + configuration = self.configuration + + self.assertIsInstance(configuration, FakeConfiguration) + + def test_provides_a_fake_configuration_for_the_duration_of_the_test(self): + c1 = self.configuration + c2 = self.configuration + + self.assertIs(c2, c1) + + +class SupportTestCase_using_testconfig(MigTestCase): + """Coverage of a MiG Testcase that requests a testconfig""" def _provide_configuration(self): return 'testconfig' diff --git a/tests/test_tests_support_configsupp.py b/tests/test_tests_support_configsupp.py new file mode 100644 index 000000000..0e85cd2c1 --- /dev/null +++ b/tests/test_tests_support_configsupp.py @@ -0,0 +1,55 @@ +# -*- coding: utf-8 -*- +# +# --- BEGIN_HEADER --- +# +# test_tests_support_configsupp - unit test of the corresponding tests module +# Copyright (C) 2003-2025 The MiG Project by the Science HPC Center at UCPH +# +# This file is part of MiG. +# +# MiG is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# MiG is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, +# USA. +# +# --- END_HEADER --- +# + +"""Unit tests for the tests module pointed to in the filename""" + +from tests.support import MigTestCase, testmain +from tests.support.configsupp import FakeConfiguration + +from mig.shared.configuration import Configuration + + +class TestsSupportConfigsupp_FakeConfiguration(MigTestCase): + """Check some basic behaviours of FakeConfiguration instances.""" + + def test_consistent_parameters(self): + default_configuration = Configuration(None) + fake_configuration = FakeConfiguration() + + self.maxDiff = None + self.assertEqual( + Configuration.to_dict(default_configuration), + Configuration.to_dict(fake_configuration) + ) + + def test_only_configuration_keys(self): + with self.assertRaises(AssertionError): + FakeConfiguration(bar='1') + + +if __name__ == '__main__': + testmain()