Make FakeConfiguration a representative configuration object. #219
Conversation
albu-diku
force-pushed
the
test/enhance-fake-configuration
branch
from
40c209e to
b451cff
Compare
albu-diku
force-pushed
the
test/enhance-fake-configuration
branch
from
b451cff to
219c4b4
Compare
albu-diku
force-pushed
the
test/enhance-fake-configuration
branch
4 times, most recently
from
8819c98 to
8bb1fc6
Compare
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 was split out previously to initialize the FakeConfiguration which itself is now a SimpleNamespace. Making it a namespace ensures that attribute lookup, something that normal Configuration objects support, work correctly but in addition forces the attributes to be set "up front". This keeps us honest in the properties we expose. Since a FakeConfiguration needs to track the real Configuration we also prevent the addition of attributes that not keys of a real configuration. Unfortunarely it seems that a lot of properties are set dynamically as part of loading a configuration, but lay the first steps to a canonical configuration object by making a couple of properties used by existing tests static; existing defaults are re-used to avoid functional change.
albu-diku
force-pushed
the
test/enhance-fake-configuration
branch
from
8bb1fc6 to
403e09e
Compare
jonasbardino
requested review from
a team
and removed request for
jonasbardino
|
Reassigned the review to all ops to avoid unnecessary bottlenecks. |
|
|
||
| 'arc_clusters': [], | ||
|
|
||
| 'cloud_services': [], |
There was a problem hiding this comment.
I wonder why cloud_services but not the similar jupyter_services needs to be added here ... if I understood the comment about us 'keeping honest' correctly shouldn't the lack of the latter raise a flag? 🤔
| 'expire_peer': 600, | ||
| 'language': ['English'], | ||
| 'user_interface': ['V2', 'V3'], | ||
| 'new_user_default_ui': 'V2', |
There was a problem hiding this comment.
This needs careful testing as we don't want to downgrade running instances to the deprecated V2. On the other hand we need to preserve V2 on GDP instances like SIF for the time being.
| return peers_dict | ||
|
|
||
| @staticmethod | ||
| def as_dict(thing): |
There was a problem hiding this comment.
This one would like a docstring explaining that it filters out internal attributes.
Minor: consider renaming thing to obj if it's always called on objects.
|
|
||
|
|
||
| def _generate_namespace_kwargs(): | ||
| d = dict(_CONFIGURATION_DEFAULTS) |
There was a problem hiding this comment.
Minor: please try hard to avoid single letter names for anything but simple iterator variables as they are horrible to track down across code sections later.
| """ | ||
|
|
||
| unknown_keys = set(d.keys()) - set(_CONFIGURATION_ARGUMENTS) | ||
| assert len(unknown_keys) == 0, \ |
There was a problem hiding this comment.
PEP8 recommends to instead rely on the fact that empty lists are logically False as in assert not unknown_keys, ... .
(https://peps.python.org/pep-0008/#programming-recommendations
| self.logger = FakeLogger() | ||
|
|
||
| @staticmethod | ||
| def as_dict(thing): |
| configuration_defaults_keys = set(_CONFIGURATION_DEFAULTS.keys()) | ||
| mismatched = _CONFIGURATION_ARGUMENTS - configuration_defaults_keys | ||
|
|
||
| self.assertEqual(len(mismatched), 0, |
There was a problem hiding this comment.
This one may want to go with the same PEP8 recommendation as above.
| # --- BEGIN_HEADER --- | ||
| # | ||
| # test_tests_support_configsupp - unit test of the corresponding tests module | ||
| # Copyright (C) 2003-2024 The MiG Project by the Science HPC Center at UCPH |
| _CONFIGURATION_NOFORWARD_KEYS, _without_noforward_keys | ||
|
|
||
|
|
||
| class MigSharedInstall_FakeConfiguration(MigTestCase): |
There was a problem hiding this comment.
Classes always want docstrings... you can grab the generic one form other tests.
2 participants
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 was split out previously to initialize the FakeConfiguration which itself is now a SimpleNamespace. Making it a namespace ensures that attribute lookup, something that normal Configuration objects support, work correctly but in addition forces the attributes to be set "up front". This keeps us honest in the properties we expose.
Since a FakeConfiguration needs to track the real Configuration we also prevent the addition of attributes that not keys of a real configuration. Unfortunarely it seems that a lot of properties are set dynamically as part of loading a configuration, but laythe first steps to a canonical configuration object by making a couple of properties used by existing tests static; existing defaults are re-used to avoid functional change.