Merge pull request #3083 from vkarak/enhancement/expand-always-last

[enhancement] Allow `always_last` to be defined in multiple hooks and add a note about users' workaround code for enforcing hook execution order

docs/regression_test_api.rst

@@ -111,10 +111,10 @@ So although a "post-init" and a "pre-setup" hook will both run *after* a test ha
 the post-init hook will execute *right after* the test is initialized.
 The framework will then continue with other activities and it will execute the pre-setup hook *just before* it schedules the test for executing its setup stage.
 
-Pipeline hooks are executed in reverse MRO order, i.e., the hooks of the least specialized class will be executed first.
+Pipeline hooks are normally executed in reverse MRO order, i.e., the hooks of the least specialized class will be executed first.
 In the following example, :func:`BaseTest.x` will execute before :func:`DerivedTest.y`:
 
-.. code:: python
+.. code-block:: python
 
    class BaseTest(rfm.RegressionTest):
        @run_after('setup')
@@ -127,6 +127,34 @@ In the following example, :func:`BaseTest.x` will execute before :func:`DerivedT
            '''Hook y'''
 
 
+This order can be altered using the ``always_last`` argument of the :func:`@run_before <reframe.core.builtins.run_before>` and :func:`@run_after <reframe.core.builtins.run_after>` decorators.
+In this case, all hooks of the same stage defined with ``always_last=True`` will be executed in MRO order at the end of the stage's hook chain.
+For example, given the following hierarchy:
+
+ .. code-block:: python
+
+    class X(rfm.RunOnlyRegressionTest):
+        @run_before('run', always_last=True)
+        def hook_a(self): pass
+
+        @run_before('run')
+        def hook_b(self): pass
+
+    class Y(X):
+        @run_before('run', always_last=True)
+        def hook_c(self): pass
+
+        @run_before('run')
+        def hook_d(self): pass
+
+
+the run hooks of :class:`Y` will be executed as follows:
+
+.. code-block:: console
+
+   X.hook_b, Y.hook_d, Y.hook_c, X.hook_a
+
+
 .. seealso::
    - :func:`@run_before <reframe.core.builtins.run_before>`, :func:`@run_after <reframe.core.builtins.run_after>` decorators
 
@@ -146,6 +174,12 @@ In the following example, :func:`BaseTest.x` will execute before :func:`DerivedT
              with osext.change_dir(self.stagedir):
                  ...
 
+.. note::
+   In versions prior to 4.3.4, overriding a pipeline hook in a subclass would re-attach it from scratch in the stage, therefore changing its execution order relative to other hooks of the superclass.
+   This is fixed in versions >= 4.3.4 where the execution order of the hook is not changed.
+   However, the fix may break existing workaround code that used to call explicitly the base class' hook from the derived one.
+   Check issue `#3012 <https://github.com/reframe-hpc/reframe/issues/3012>`__ for details on how to properly address this.
+
 .. warning::
    .. versionchanged:: 3.7.0
       Declaring pipeline hooks using the same name functions from the :py:mod:`reframe` or :py:mod:`reframe.core.decorators` modules is now deprecated.

reframe/core/builtins.py

@@ -48,15 +48,18 @@ def run_before(stage, *, always_last=False):
     :param stage: The pipeline stage where this function will be attached to.
         See :ref:`pipeline-hooks` for the list of valid stage values.
 
-    :param always_last: Run this hook always as the last one of the stage. In
-        a whole test hierarchy, only a single hook can be explicitly pinned at
-        the end of the same-stage sequence of hooks. If another hook is
-        declared as ``always_last`` in the same stage, an error will be
-        issued.
+    :param always_last: Run this hook at the end of the stage's hook chain
+        instead of the beginning. If multiple tests set this flag for a hook
+        in the same stage, then all ``always_last`` hooks will be executed in
+        MRO order at the end of stage's hook chain. See :ref:`pipeline-hooks`
+        for an example execution.
 
     .. versionchanged:: 4.4
        The ``always_last`` argument was added.
 
+    .. versionchanged:: 4.5
+       Multiple tests can set ``always_last`` in the same stage.
+
     '''
 
     return hooks.attach_to('pre_' + stage, always_last)

reframe/core/pipeline.py

@@ -183,28 +183,23 @@ def pipeline_hooks(cls):
         for hook in cls._rfm_hook_registry:
             for stage, always_last in hook.stages:
                 if always_last:
-                    if stage in last:
-                        hook_name = hook.__qualname__
-                        pinned_name = last[stage].__qualname__
-                        raise ReframeSyntaxError(
-                            f'cannot pin hook {hook_name!r} as last '
-                            f'of stage {stage!r} as {pinned_name!r} '
-                            f'is already pinned last'
-                        )
-
-                    last[stage] = hook
+                    try:
+                        last[stage].append(hook.fn)
+                    except KeyError:
+                        last[stage] = [hook.fn]
                 else:
                     try:
                         ret[stage].append(hook.fn)
                     except KeyError:
                         ret[stage] = [hook.fn]
 
         # Append the last hooks
-        for stage, hook in last.items():
-            try:
-                ret[stage].append(hook.fn)
-            except KeyError:
-                ret[stage] = [hook.fn]
+        for stage, hooks in last.items():
+            for fn in reversed(hooks):
+                try:
+                    ret[stage].append(fn)
+                except KeyError:
+                    ret[stage] = [fn]
 
         return ret
 

unittests/test_pipeline.py

@@ -1183,7 +1183,6 @@ def foo(self):
 
 
 def test_pinned_hooks():
-    @test_util.custom_prefix('unittests/resources/checks')
     class X(rfm.RunOnlyRegressionTest):
         @run_before('run', always_last=True)
         def foo(self):
@@ -1207,34 +1206,43 @@ def bar(self):
 
 
 def test_pinned_hooks_multiple_last():
-    @test_util.custom_prefix('unittests/resources/checks')
     class X(rfm.RunOnlyRegressionTest):
         @run_before('run', always_last=True)
-        def foo(self):
+        def hook_a(self):
+            pass
+
+        @run_before('run')
+        def hook_b(self):
             pass
 
     class Y(X):
         @run_before('run', always_last=True)
-        def bar(self):
+        def hook_c(self):
             pass
 
-    with pytest.raises(ReframeSyntaxError):
-        test = Y()
+        @run_before('run')
+        def hook_d(self):
+            pass
+
+    test = Y()
+    assert test.pipeline_hooks() == {
+        'pre_run': [X.hook_b, Y.hook_d, Y.hook_c, X.hook_a]
+    }
 
 
 def test_pinned_hooks_multiple_last_inherited():
-    @test_util.custom_prefix('unittests/resources/checks')
     class X(rfm.RunOnlyRegressionTest):
         @run_before('run', always_last=True)
         def foo(self):
             pass
 
+    class Y(X):
         @run_before('run', always_last=True)
         def bar(self):
             pass
 
-    with pytest.raises(ReframeSyntaxError):
-        test = X()
+    test = Y()
+    assert test.pipeline_hooks() == {'pre_run': [Y.bar, X.foo]}
 
 
 def test_disabled_hooks(HelloTest, local_exec_ctx):