From 31059e34631f2af67a1772d2f307a063b6ea3ab1 Mon Sep 17 00:00:00 2001 From: Javier Otero Date: Tue, 10 Aug 2021 20:51:25 +0200 Subject: [PATCH 1/6] Redesign test registry --- reframe/core/decorators.py | 75 ++++++++++++++++++++++++++++++++++++-- reframe/frontend/loader.py | 32 +++++++++++----- 2 files changed, 94 insertions(+), 13 deletions(-) diff --git a/reframe/core/decorators.py b/reframe/core/decorators.py index 7f4756b05f..9d7963e815 100644 --- a/reframe/core/decorators.py +++ b/reframe/core/decorators.py @@ -27,7 +27,71 @@ from reframe.utility.versioning import VersionValidator -def _register_test(cls, args=None): +class TestRegistry: + def __init__(self): + self._tests = [] + self._skip_tests = set() + + @classmethod + def create(cls, test, *args, **kwargs): + obj = cls() + obj.add(test, *args, **kwargs) + return obj + + def add(self, test, *args, **kwargs): + self._tests.append((test, args, kwargs)) + + # FIXME: To drop with the required_version decorator + def skip(self, test): + self._skip_tests.add(test) + + def instantiate_all(self): + '''Instantiate all the registered tests.''' + ret = [] + for test, args, kwargs in self._tests: + if test in self._skip_tests: + continue + + try: + ret.append(test(*args, **kwargs)) + except SkipTestError as e: + getlogger().warning( + f'skipping test {test.__qualname__!r}: {e}' + ) + except Exception: + exc_info = sys.exc_info() + getlogger().warning( + f"skipping test {test.__qualname__!r}: {what(*exc_info)} " + f"(rerun with '-v' for more information)" + ) + getlogger().verbose(traceback.format_exc()) + + return ret + + def __iter__(self): + '''Iterate over all the registered tests.''' + return (test[0] for test in self._tests) + + # FIXME: To drop with the required_version decorator + def __contains__(self, test): + for t in self: + if test is t: + return True + + return False + + +def _register_test(cls, *args, **kwargs): + '''Register a test and its construction arguments into the registry.''' + + mod = inspect.getmodule(cls) + if not hasattr(mod, '_rfm_test_registry'): + mod._rfm_test_registry = TestRegistry.create(cls, *args, **kwargs) + else: + mod._rfm_test_registry.add(cls, *args, **kwargs) + + +def _register_parameterized_test(cls, args=None): '''Register the test. Register the test with _rfm_use_params=True. This additional argument flags @@ -111,7 +175,7 @@ def simple_test(cls): ''' if _validate_test(cls): for _ in cls.param_space: - _register_test(cls) + _register_test(cls, _rfm_use_params=True) return cls @@ -153,7 +217,7 @@ def _do_register(cls): ) for args in inst: - _register_test(cls, args) + _register_parameterized_test(cls, args) return cls @@ -215,7 +279,10 @@ def _skip_tests(cls): f"skipping incompatible test '{cls.__qualname__}': not valid " f"for ReFrame version {osext.reframe_version().split('-')[0]}" ) - mod.__rfm_skip_tests.add(cls) + if cls in mod._rfm_test_registry: + mod._rfm_test_registry.skip(cls) + else: + mod.__rfm_skip_tests.add(cls) return cls diff --git a/reframe/frontend/loader.py b/reframe/frontend/loader.py index 6eb24dc57d..cb20d1f437 100644 --- a/reframe/frontend/loader.py +++ b/reframe/frontend/loader.py @@ -118,8 +118,14 @@ def recurse(self): def load_from_module(self, module): '''Load user checks from module. - This method tries to call the `_rfm_gettests()` method of the user - check and validates its return value.''' + This method tries to load the test registry from a given module and + instantiates all the tests in the registry. The instantiated checks + are validated before return. + + For legacy reasons, a module might have the additional legacy registry + `_rfm_gettests`, which is a method that instantiates all the tests + registered with the deprecated `parameterized_test` decorator. + ''' from reframe.core.pipeline import RegressionTest # Warn in case of old syntax @@ -129,16 +135,24 @@ def load_from_module(self, module): f'in test files: please use @reframe.simple_test decorator' ) - if not hasattr(module, '_rfm_gettests'): + # FIXME: Remove the legacy_registry after dropping parameterized_test + registry = getattr(module, '_rfm_test_registry', None) + legacy_registry = getattr(module, '_rfm_gettests', None) + if not any((registry, legacy_registry)): getlogger().debug('No tests registered') return [] - candidates = module._rfm_gettests() - if not isinstance(candidates, collections.abc.Sequence): - getlogger().warning( - f'Tests not registered correctly in {module.__name__!r}' - ) - return [] + candidates = registry.instantiate_all() if registry else [] + legacy_candidates = legacy_registry() if legacy_registry else [] + for c in (candidates, legacy_candidates): + if not isinstance(c, collections.abc.Sequence): + getlogger().warning( + f'Tests not registered correctly in {module.__name__!r}' + ) + return [] + + # Merge registries + candidates += legacy_candidates ret = [] for c in candidates: From 709799fbc425a4ed3c055e6e60618a79146abf66 Mon Sep 17 00:00:00 2001 From: Javier Otero Date: Wed, 11 Aug 2021 09:29:10 +0200 Subject: [PATCH 2/6] Update unit tests --- unittests/test_parameters.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/unittests/test_parameters.py b/unittests/test_parameters.py index dfc5556a06..24dac7c71d 100644 --- a/unittests/test_parameters.py +++ b/unittests/test_parameters.py @@ -150,7 +150,7 @@ class MyTest(ExtendParams): pass mod = inspect.getmodule(MyTest) - tests = mod._rfm_gettests() + tests = mod._rfm_test_registry.instantiate_all() assert len(tests) == 8 for test in tests: assert test.P0 is not None From 5ccfa4d94a639e61630afa264b089906c1c377d3 Mon Sep 17 00:00:00 2001 From: Javier Otero Date: Wed, 11 Aug 2021 11:46:39 +0200 Subject: [PATCH 3/6] Bugfix AttributeError --- reframe/core/decorators.py | 63 +++++++++++++++++++++++--------------- 1 file changed, 39 insertions(+), 24 deletions(-) diff --git a/reframe/core/decorators.py b/reframe/core/decorators.py index 9d7963e815..1abfe487c8 100644 --- a/reframe/core/decorators.py +++ b/reframe/core/decorators.py @@ -28,8 +28,19 @@ class TestRegistry: + '''Regression test registry. + + The tests are stored in a dictionary where the test class is the key + and the constructor arguments for the different instantiations of the + test are stored as the dictionary value as a list of (args, kwargs) + tuples. + + For backward compatibility reasons, the registry also contains a set of + tests to be skipped. The machinery related to this should be dropped with + the ``required_version`` decorator. + ''' def __init__(self): - self._tests = [] + self._tests = dict() self._skip_tests = set() @classmethod @@ -39,46 +50,47 @@ def create(cls, test, *args, **kwargs): return obj def add(self, test, *args, **kwargs): - self._tests.append((test, args, kwargs)) + if test in self._tests: + self._tests[test].append((args, kwargs)) + else: + self._tests[test] = [(args, kwargs)] # FIXME: To drop with the required_version decorator def skip(self, test): + '''Add a test to the skip set.''' self._skip_tests.add(test) def instantiate_all(self): '''Instantiate all the registered tests.''' ret = [] - for test, args, kwargs in self._tests: + for test, variants in self._tests.items(): if test in self._skip_tests: continue - try: - ret.append(test(*args, **kwargs)) - except SkipTestError as e: - getlogger().warning( - f'skipping test {test.__qualname__!r}: {e}' - ) - except Exception: - exc_info = sys.exc_info() - getlogger().warning( - f"skipping test {test.__qualname__!r}: {what(*exc_info)} " - f"(rerun with '-v' for more information)" - ) - getlogger().verbose(traceback.format_exc()) + for args, kwargs in variants: + try: + ret.append(test(*args, **kwargs)) + except SkipTestError as e: + getlogger().warning( + f'skipping test {test.__qualname__!r}: {e}' + ) + except Exception: + exc_info = sys.exc_info() + getlogger().warning( + f"skipping test {test.__qualname__!r}: " + f"{what(*exc_info)} " + f"(rerun with '-v' for more information)" + ) + getlogger().verbose(traceback.format_exc()) return ret def __iter__(self): - '''Iterate over all the registered tests.''' - return (test[0] for test in self._tests) + '''Iterate over the registered test classes.''' + return iter(self._tests.keys()) - # FIXME: To drop with the required_version decorator def __contains__(self, test): - for t in self: - if test is t: - return True - - return False + return test in self._tests def _register_test(cls, *args, **kwargs): @@ -274,6 +286,9 @@ def _skip_tests(cls): if not hasattr(mod, '__rfm_skip_tests'): mod.__rfm_skip_tests = set() + if not hasattr(mod, '_rfm_test_registry'): + mod._rfm_test_registry = TestRegistry() + if not any(c.validate(osext.reframe_version()) for c in conditions): getlogger().warning( f"skipping incompatible test '{cls.__qualname__}': not valid " From 049832637ad112998fc5ab99c79f46b65b0ecbae Mon Sep 17 00:00:00 2001 From: Javier Otero Date: Wed, 11 Aug 2021 11:50:24 +0200 Subject: [PATCH 4/6] Fix PEP complaints --- reframe/core/decorators.py | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/reframe/core/decorators.py b/reframe/core/decorators.py index 1abfe487c8..ec5f861c15 100644 --- a/reframe/core/decorators.py +++ b/reframe/core/decorators.py @@ -39,6 +39,7 @@ class TestRegistry: tests to be skipped. The machinery related to this should be dropped with the ``required_version`` decorator. ''' + def __init__(self): self._tests = dict() self._skip_tests = set() @@ -68,20 +69,20 @@ def instantiate_all(self): continue for args, kwargs in variants: - try: - ret.append(test(*args, **kwargs)) - except SkipTestError as e: - getlogger().warning( - f'skipping test {test.__qualname__!r}: {e}' - ) - except Exception: - exc_info = sys.exc_info() - getlogger().warning( - f"skipping test {test.__qualname__!r}: " - f"{what(*exc_info)} " - f"(rerun with '-v' for more information)" - ) - getlogger().verbose(traceback.format_exc()) + try: + ret.append(test(*args, **kwargs)) + except SkipTestError as e: + getlogger().warning( + f'skipping test {test.__qualname__!r}: {e}' + ) + except Exception: + exc_info = sys.exc_info() + getlogger().warning( + f"skipping test {test.__qualname__!r}: " + f"{what(*exc_info)} " + f"(rerun with '-v' for more information)" + ) + getlogger().verbose(traceback.format_exc()) return ret From 9ad58b50b109faf8213e0e3b99e2eec37978452d Mon Sep 17 00:00:00 2001 From: Javier Otero Date: Thu, 12 Aug 2021 19:48:24 +0200 Subject: [PATCH 5/6] Address PR comments --- reframe/core/decorators.py | 6 ++---- reframe/frontend/loader.py | 6 ------ 2 files changed, 2 insertions(+), 10 deletions(-) diff --git a/reframe/core/decorators.py b/reframe/core/decorators.py index ec5f861c15..d31f71c967 100644 --- a/reframe/core/decorators.py +++ b/reframe/core/decorators.py @@ -51,10 +51,8 @@ def create(cls, test, *args, **kwargs): return obj def add(self, test, *args, **kwargs): - if test in self._tests: - self._tests[test].append((args, kwargs)) - else: - self._tests[test] = [(args, kwargs)] + self._tests.setdefault(test, []) + self._tests[test].append((args, kwargs)) # FIXME: To drop with the required_version decorator def skip(self, test): diff --git a/reframe/frontend/loader.py b/reframe/frontend/loader.py index cb20d1f437..01d7e7c3da 100644 --- a/reframe/frontend/loader.py +++ b/reframe/frontend/loader.py @@ -144,12 +144,6 @@ def load_from_module(self, module): candidates = registry.instantiate_all() if registry else [] legacy_candidates = legacy_registry() if legacy_registry else [] - for c in (candidates, legacy_candidates): - if not isinstance(c, collections.abc.Sequence): - getlogger().warning( - f'Tests not registered correctly in {module.__name__!r}' - ) - return [] # Merge registries candidates += legacy_candidates From 4e155cea69c78666d7b21d4970fc0b3933a933eb Mon Sep 17 00:00:00 2001 From: Javier Otero Date: Thu, 12 Aug 2021 19:57:06 +0200 Subject: [PATCH 6/6] Remove unused imports --- reframe/frontend/loader.py | 1 - 1 file changed, 1 deletion(-) diff --git a/reframe/frontend/loader.py b/reframe/frontend/loader.py index 01d7e7c3da..e928c04e19 100644 --- a/reframe/frontend/loader.py +++ b/reframe/frontend/loader.py @@ -8,7 +8,6 @@ # import ast -import collections.abc import inspect import os import sys