Merge pull request #2997 from vkarak/bugfix/overriden-hooks-exec-order

[bugfix] Respect execution order of overriden hooks

Author: vkarak

reframe/core/hooks.py

@@ -6,7 +6,13 @@
 import functools
 import inspect
 
-import reframe.utility as util
+
+def is_hook(func):
+    return hasattr(func, '_rfm_attach')
+
+
+def is_dep_hook(func):
+    return hasattr(func, '_rfm_resolve_deps')
 
 
 def attach_to(phase):
@@ -15,7 +21,7 @@ def attach_to(phase):
     :meta private:
     '''
     def deco(func):
-        if hasattr(func, '_rfm_attach'):
+        if is_hook(func):
             func._rfm_attach.append(phase)
         else:
             func._rfm_attach = [phase]
@@ -118,7 +124,7 @@ class Hook:
 
     def __init__(self, fn):
         self.__fn = fn
-        if not hasattr(fn, '_rfm_attach'):
+        if not is_hook(fn):
             raise ValueError(f'{fn.__name__} is not a hook')
 
     @property
@@ -152,7 +158,7 @@ class HookRegistry:
     '''Global hook registry.'''
 
     def __init__(self, hooks=None):
-        self.__hooks = util.OrderedSet()
+        self.__hooks = []
         if hooks is not None:
             self.update(hooks)
 
@@ -172,30 +178,36 @@ def add(self, v):
         of the pipeline where they must be attached. Dependencies will be
         resolved first in the post-setup phase if not assigned elsewhere.
         '''
-
-        if hasattr(v, '_rfm_attach'):
+        if is_hook(v):
             # Always override hooks with the same name
             h = Hook(v)
-            self.__hooks.discard(h)
-            self.__hooks.add(h)
-        elif hasattr(v, '_rfm_resolve_deps'):
+            try:
+                pos = self.__hooks.index(h)
+            except ValueError:
+                self.__hooks.append(h)
+            else:
+                self.__hooks[pos] = h
+        elif is_dep_hook(v):
             v._rfm_attach = ['post_setup']
-            self.__hooks.add(Hook(v))
-
-    def update(self, other, *, denied_hooks=None):
-        '''Update the hook registry with the hooks from another hook registry.
+            self.__hooks.append(Hook(v))
 
-        The optional ``denied_hooks`` argument takes a set of disallowed
-        hook names, preventing their inclusion into the current hook registry.
-        '''
+    def update(self, other, *, forbidden_names=None):
+        '''Update the hook registry with the hooks from another hook
+        registry.'''
 
         assert isinstance(other, HookRegistry)
-        denied_hooks = denied_hooks or set()
+        forbidden_names = forbidden_names or {}
         for h in other:
-            if h.__name__ not in denied_hooks:
-                # Hooks in `other` override ours
-                self.__hooks.discard(h)
-                self.__hooks.add(h)
+            if (h.__name__ in forbidden_names and
+                not is_hook(forbidden_names[h.__name__])):
+                continue
+
+            try:
+                pos = self.__hooks.index(h)
+            except ValueError:
+                self.__hooks.append(h)
+            else:
+                self.__hooks[pos] = h
 
     def __repr__(self):
         return repr(self.__hooks)

reframe/core/meta.py

@@ -348,9 +348,8 @@ def __init__(cls, name, bases, namespace, **kwargs):
         # parent classes in reverse MRO order
         for c in list(reversed(cls.mro()))[:-1]:
             if hasattr(c, '_rfm_local_hook_registry'):
-                cls._rfm_hook_registry.update(
-                    c._rfm_local_hook_registry, denied_hooks=namespace
-                )
+                cls._rfm_hook_registry.update(c._rfm_local_hook_registry,
+                                              forbidden_names=namespace)
 
         cls._rfm_hook_registry.update(cls._rfm_local_hook_registry)
 

unittests/test_pipeline.py

@@ -1162,6 +1162,26 @@ def foo(self):
     assert test.pipeline_hooks() == {'post_setup': [MyTest.foo]}
 
 
+def test_overriden_hook_exec_order():
+    @test_util.custom_prefix('unittests/resources/checks')
+    class X(rfm.RunOnlyRegressionTest):
+        @run_before('run')
+        def foo(self):
+            pass
+
+        @run_before('run')
+        def bar(self):
+            pass
+
+    class Y(X):
+        @run_before('run')
+        def foo(self):
+            pass
+
+    test = Y()
+    assert test.pipeline_hooks() == {'pre_run': [Y.foo, X.bar]}
+
+
 def test_disabled_hooks(HelloTest, local_exec_ctx):
     @test_util.custom_prefix('unittests/resources/checks')
     class BaseTest(HelloTest):