From 1f02dfc37f6b9dccc1ed3c82ecaf9ab3938855aa Mon Sep 17 00:00:00 2001 From: Javier Otero Date: Thu, 1 Jul 2021 11:56:44 +0200 Subject: [PATCH 1/7] Bugfix hook override --- reframe/core/hooks.py | 9 +++++++-- reframe/core/meta.py | 3 ++- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/reframe/core/hooks.py b/reframe/core/hooks.py index 69f411968d..0af9ec9f77 100644 --- a/reframe/core/hooks.py +++ b/reframe/core/hooks.py @@ -170,10 +170,15 @@ def __contains__(self, key): def __getattr__(self, name): return getattr(self.__hooks, name) - def update(self, hooks): + def update(self, hooks, *, denied_hooks={}): + '''Update the hook registry with the hooks from another hook registry. + + The optional ``denied_hooks`` argument takes a set of disallowed + hook names, preventing their inclusion into the current hook registry. + ''' for phase, hks in hooks.items(): self.__hooks.setdefault(phase, util.OrderedSet()) - for h in hks: + for h in (hk for hk in hks if hk.__name__ not in denied_hooks): self.__hooks[phase].add(h) def __repr__(self): diff --git a/reframe/core/meta.py b/reframe/core/meta.py index 089986c804..d37c52cc4c 100644 --- a/reframe/core/meta.py +++ b/reframe/core/meta.py @@ -291,7 +291,8 @@ def __init__(cls, name, bases, namespace, **kwargs): # phase if not assigned elsewhere hook_reg = hooks.HookRegistry.create(namespace) for base in (b for b in bases if hasattr(b, '_rfm_pipeline_hooks')): - hook_reg.update(getattr(base, '_rfm_pipeline_hooks')) + hook_reg.update(getattr(base, '_rfm_pipeline_hooks'), + denied_hooks=namespace) cls._rfm_pipeline_hooks = hook_reg From c792bc07d1139c914c7d911dd3f57bf75d13360e Mon Sep 17 00:00:00 2001 From: Javier Otero Date: Thu, 1 Jul 2021 12:00:23 +0200 Subject: [PATCH 2/7] Replace blacklist by directive --- reframe/core/meta.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/reframe/core/meta.py b/reframe/core/meta.py index d37c52cc4c..508d69d6b7 100644 --- a/reframe/core/meta.py +++ b/reframe/core/meta.py @@ -257,11 +257,11 @@ class was created or even at the instance level (e.g. doing constructed. ''' - blacklist = [ + directives = [ 'parameter', 'variable', 'bind', 'run_before', 'run_after', 'require_deps', 'required', 'deferrable', 'sanity_function' ] - for b in blacklist: + for b in directives: namespace.pop(b, None) return super().__new__(metacls, name, bases, dict(namespace), **kwargs) From ec7c62b5fce0ba1e2f361a2faf3471731c36ebe0 Mon Sep 17 00:00:00 2001 From: Javier Otero Date: Thu, 1 Jul 2021 13:20:50 +0200 Subject: [PATCH 3/7] Update docs --- docs/regression_test_api.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/regression_test_api.rst b/docs/regression_test_api.rst index 52d31af751..2ccaa63776 100644 --- a/docs/regression_test_api.rst +++ b/docs/regression_test_api.rst @@ -204,7 +204,7 @@ You can attach arbitrary functions to run before or after any pipeline stage, wh Multiple hooks can be attached before or after the same pipeline stage, in which case the order of execution will match the order in which the functions are defined in the class body of the test. A single hook can also be applied to multiple stages and it will be executed multiple times. All pipeline hooks of a test class are inherited by its subclasses. -Subclasses may override a pipeline hook of their parents by redefining the hook function and re-attaching it at the same pipeline stage. +Subclasses may override a pipeline hook of their parents by redefining the hook function, where the overriding function may be reattached to any pipeline stage. There are seven pipeline stages where you can attach test methods: ``init``, ``setup``, ``compile``, ``run``, ``sanity``, ``performance`` and ``cleanup``. The ``init`` stage is not a real pipeline stage, but it refers to the test initialization. From 347b7870c99d35938f49cc3a8a127ab0413346fb Mon Sep 17 00:00:00 2001 From: Javier Otero Date: Mon, 5 Jul 2021 10:44:42 +0200 Subject: [PATCH 4/7] Update docs. --- docs/regression_test_api.rst | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/regression_test_api.rst b/docs/regression_test_api.rst index 2ccaa63776..3818559127 100644 --- a/docs/regression_test_api.rst +++ b/docs/regression_test_api.rst @@ -204,7 +204,8 @@ You can attach arbitrary functions to run before or after any pipeline stage, wh Multiple hooks can be attached before or after the same pipeline stage, in which case the order of execution will match the order in which the functions are defined in the class body of the test. A single hook can also be applied to multiple stages and it will be executed multiple times. All pipeline hooks of a test class are inherited by its subclasses. -Subclasses may override a pipeline hook of their parents by redefining the hook function, where the overriding function may be reattached to any pipeline stage. +Subclasses may override a pipeline hook of their parents by redefining the hook function. +The overriding function will not be a pipeline hook unless explicitly decorated, and it may be reattached to any pipeline stage. There are seven pipeline stages where you can attach test methods: ``init``, ``setup``, ``compile``, ``run``, ``sanity``, ``performance`` and ``cleanup``. The ``init`` stage is not a real pipeline stage, but it refers to the test initialization. From 8587e211844541732d2fa41144f99c24cc9d86a0 Mon Sep 17 00:00:00 2001 From: Javier Otero Date: Mon, 5 Jul 2021 12:09:03 +0200 Subject: [PATCH 5/7] Bugfix hook registry update args --- reframe/core/hooks.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/reframe/core/hooks.py b/reframe/core/hooks.py index 0af9ec9f77..1a02cc1ae3 100644 --- a/reframe/core/hooks.py +++ b/reframe/core/hooks.py @@ -170,16 +170,18 @@ def __contains__(self, key): def __getattr__(self, name): return getattr(self.__hooks, name) - def update(self, hooks, *, denied_hooks={}): + def update(self, hooks, *, denied_hooks=None): '''Update the hook registry with the hooks from another hook registry. The optional ``denied_hooks`` argument takes a set of disallowed hook names, preventing their inclusion into the current hook registry. ''' + denied_hooks = denied_hooks or set() for phase, hks in hooks.items(): self.__hooks.setdefault(phase, util.OrderedSet()) - for h in (hk for hk in hks if hk.__name__ not in denied_hooks): - self.__hooks[phase].add(h) + for h in hks: + if h.__name__ not in denied_hooks: + self.__hooks[phase].add(h) def __repr__(self): return repr(self.__hooks) From a3d0e4ab6a4ed1edcadcad2807d37d4866dd60fc Mon Sep 17 00:00:00 2001 From: Javier Otero Date: Mon, 5 Jul 2021 12:09:21 +0200 Subject: [PATCH 6/7] Add unit tests --- unittests/test_meta.py | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/unittests/test_meta.py b/unittests/test_meta.py index 20b58645f8..82a8cee918 100644 --- a/unittests/test_meta.py +++ b/unittests/test_meta.py @@ -144,3 +144,44 @@ def my_deferrable(self): pass assert type(MyTest.my_deferrable()) is deferrable._DeferredExpression + + +def test_hook_attachments(MyMeta): + class Foo(MyMeta): + @run_after('setup') + def hook_a(self): + pass + + @run_before('compile') + def hook_b(self): + pass + + @run_after('run') + def hook_c(self): + pass + + @classmethod + def hook_in_stage(cls, hook, stage): + '''Assert that a hook is in a given registry stage.''' + try: + return hook in {h.__name__ + for h in cls._rfm_pipeline_hooks[stage]} + except KeyError: + return False + + assert Foo.hook_in_stage('hook_a', 'post_setup') + assert Foo.hook_in_stage('hook_b', 'pre_compile') + assert Foo.hook_in_stage('hook_c', 'post_run_wait') + + class Bar(Foo): + @run_before('sanity') + def hook_a(self): + "Convert to a pre-sanity hook" + + def hook_b(self): + "No longer a hook" + + assert not Bar.hook_in_stage('hook_a', 'post_setup') + assert not Bar.hook_in_stage('hook_b', 'pre_compile') + assert Bar.hook_in_stage('hook_c', 'post_run_wait') + assert Bar.hook_in_stage('hook_a', 'pre_sanity') From ad505e2526d8037ae751878ccb4706b199e6d27a Mon Sep 17 00:00:00 2001 From: Vasileios Karakasis Date: Mon, 5 Jul 2021 20:37:32 +0200 Subject: [PATCH 7/7] Address PR comments --- unittests/test_meta.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/unittests/test_meta.py b/unittests/test_meta.py index e5e4d77329..4dfa290061 100644 --- a/unittests/test_meta.py +++ b/unittests/test_meta.py @@ -192,10 +192,10 @@ def hook_in_stage(cls, hook, stage): class Bar(Foo): @run_before('sanity') def hook_a(self): - "Convert to a pre-sanity hook" + '''Convert to a pre-sanity hook''' def hook_b(self): - "No longer a hook" + '''No longer a hook''' assert not Bar.hook_in_stage('hook_a', 'post_setup') assert not Bar.hook_in_stage('hook_b', 'pre_compile')