Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow to directly import constants from easybuild.framework.easyconfig.constants #4144

Merged
merged 2 commits into from
Feb 15, 2023
Merged

allow to directly import constants from easybuild.framework.easyconfig.constants #4144

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c799364
Allow to directly import EasyConfig constants from the module
Flamefire Dec 19, 2022
c2860fb
Split into 2 tests
Flamefire Feb 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions easybuild/framework/easyconfig/constants.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,3 +82,7 @@ def _get_arch_constant():
'OS_PKG_PAM_DEV': (('pam-devel', 'libpam0g-dev'),
"OS packages providing Pluggable Authentication Module (PAM) developement support"),
}

# Add EasyConfig constants to export list
globals().update({name: value for name, (value, _) in EASYCONFIG_CONSTANTS.items()})
__all__ = ['EXTERNAL_MODULE_MARKER', 'EASYCONFIG_CONSTANTS'] + list(EASYCONFIG_CONSTANTS.keys())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, does __all__.extend(list(EASYCONFIG_CONSTANTS.keys()) work too?

Just to make sure we don't overlooked adding stuff to all if additional (real) constants are added to constants.py

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested that and no. __all__ has a default value if it is not set (only), so at that point it is an undefined variable.

It actually isn't really required because (almost) nobody does from easyconfig.constants import * which is the only use case of __all__, everything else works without it.

11 changes: 11 additions & 0 deletions test/framework/easyconfig.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1278,6 +1278,17 @@ def test_constant_doc(self):
]
self.assertEqual(len(doc.split('\n')), sum([len(temps)] + [len(x) for x in temps]))

def test_constants_import(self):
"""Test importing constants works"""
# Sanity check that importing an EC constant works as-if using EASYCONFIG_CONSTANTS
from easybuild.framework.easyconfig.constants import SYSTEM
self.assertEqual(SYSTEM, easyconfig.constants.EASYCONFIG_CONSTANTS['SYSTEM'][0])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For SYSTEM, I would make this a hard check, rather than relying on EASYCONFIG_CONSTANTS:

self.assertEqual(SYSTEM, {'name': 'system', 'version': 'system'})

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But that is not what is tested here. Only importing the constants should be tested here and that means that doing such an explicit import is the same as accessing the EASYCONFIG_CONSTANTS list, see the comment.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not saying you should not do the explicit import, you should.

I'm saying you should compare the value of SYSTEM directly to a hardcoded dict here, rather than relying on EASYCONFIG_CONSTANTS, otherwise the tests would still pass when the value for SYSTEM in EASYCONFIG_CONSTANTS in changed in the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

otherwise the tests would still pass when the value for SYSTEM in EASYCONFIG_CONSTANTS in changed in the code.

Erm, This is exactly what you want, don't you?

  • The value of SYSTEM in ECs is that of EASYCONFIG_CONSTANTS
  • Previously you would need to use easyconfig.constants.EASYCONFIG_CONSTANTS['SYSTEM'][0] in hooks etc.
  • With this PR you can instead use from easybuild.framework.easyconfig.constants import SYSTEM to the same effect
  • Hence the test checks that the imported value matches the value in EASYCONFIG_CONSTANTS to verify it is indeed the same effect and has a matching comment

So I don't understand why the test shouldn't pass when easybuild.framework.easyconfig.constants.SYSTEM == easyconfig.constants.EASYCONFIG_CONSTANTS['SYSTEM'][0]. Could you explain your thought here?

Additionally I really dislike using hardcoded values as this results in unrelated tests failing. This was e.g. the case where a spelling fix to a doc caused another test to fail: #3219 -> #4158 So if some previously run other test for any reason changed the EASYCONFIG_CONSTANTS (which shouldn't happen, but I guess is what you had in mind) this test still passing is IMO actually a good thing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I consider a change to the value of a constant a breaking change, so tests should fail because of it.

Although no code should be relying on the value of the constant, it's not unlikely that some stuff does, and hence the value of the constant should never be changed.
Hardcoding the expected value in the tests is a way of catching that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I consider a change to the value of a constant a breaking change, so tests should fail because of it.

Ok understood. But I wouldn't do that here as that is not the purpose of the test: "importing an EC constant works as-if using EASYCONFIG_CONSTANTS".

I split the test into 2 where the 2nd ensures exactly that and the first is more like what a user would do and uses the hard-coded value. I'm still not a fan of doing that for the mentioned reason and would rather have dedicated tests to catch constant changes that shouldn't change or just say that the purpose of the constant is that the value does not matter so anything relying on it is a bug anyway.

# Check each individual constant
constants = __import__('easybuild.framework.easyconfig.constants', fromlist=[None])
for name, (value, _doc) in easyconfig.constants.EASYCONFIG_CONSTANTS.items():
self.assertTrue(hasattr(constants, name), 'Missing ' + name)
self.assertEqual(getattr(constants, name), value)

def test_build_options(self):
"""Test configure/build/install options, both strings and lists."""
orig_contents = '\n'.join([
Expand Down