fix(hooks): don't execute the current scope when an error occurs during a before hook (#174)

Author: daantimmer

cucumber_cpp/acceptance_test/MainCustom.cpp

@@ -5,6 +5,7 @@ int main(int argc, char** argv)
     cucumber_cpp::Application application{};
 
     application.CliParser().add_flag("--required", *application.ProgramContext().EmplaceAt<bool>("--required"))->required();
+    application.CliParser().add_flag("--failprogramhook", *application.ProgramContext().EmplaceAt<bool>("--failprogramhook"));
 
     return application.Run(argc, argv);
 }

cucumber_cpp/acceptance_test/hooks/Hooks.cpp

@@ -6,6 +6,9 @@
 HOOK_BEFORE_ALL()
 {
     std::cout << "HOOK_BEFORE_ALL\n";
+
+    if (context.Contains("--failprogramhook") && context.Get<bool>("--failprogramhook"))
+        ASSERT_THAT(false, testing::IsTrue());
 }
 
 HOOK_AFTER_ALL()
@@ -47,3 +50,9 @@ HOOK_BEFORE_SCENARIO("@throw_scenariohook")
 {
     throw std::string{ "error" };
 }
+
+HOOK_BEFORE_SCENARIO()
+{
+    if (context.Contains("--failprogramhook") && context.Get<bool>("--failprogramhook"))
+        std::cout << "should not be executed\n";
+}

cucumber_cpp/acceptance_test/test.bats

@@ -176,3 +176,12 @@ teardown() {
     assert_output --partial "skipped Given a given step"
     assert_output --partial "tests   : 0/1 passed"
 }
+
+
+@test "Test error program hook results in error and skipped steps" {
+    run .build/Host/cucumber_cpp/acceptance_test/Debug/cucumber_cpp.acceptance_test.custom run --feature cucumber_cpp/acceptance_test/features  --tag "@smoke and @result:OK" --report console --required --failprogramhook
+    assert_failure
+    assert_output --partial "skipped Given a given step"
+    refute_output --partial "should not be executed"
+    # assert_output --partial "tests   : 0/1 passed"
+}

cucumber_cpp/library/Application.cpp

@@ -224,7 +224,7 @@ namespace cucumber_cpp::library
 
     int Application::GetExitCode() const
     {
-        if (contextManager.ProgramContext().ExecutionStatus() == engine::Result::passed)
+        if (contextManager.ProgramContext().EffectiveExecutionStatus() == engine::Result::passed)
             return 0;
         else
             return 1;

cucumber_cpp/library/engine/ContextManager.cpp

@@ -5,6 +5,7 @@
 #include "cucumber_cpp/library/engine/RuleInfo.hpp"
 #include "cucumber_cpp/library/engine/ScenarioInfo.hpp"
 #include "cucumber_cpp/library/engine/StepInfo.hpp"
+#include <algorithm>
 #include <memory>
 #include <string>
 #include <string_view>
@@ -48,11 +49,29 @@ namespace cucumber_cpp::library::engine
         Start();
     }
 
+    [[nodiscard]] Result CurrentContext::InheritedExecutionStatus() const
+    {
+        if (parent == nullptr)
+            return executionStatus;
+        else
+            return std::max(executionStatus, parent->InheritedExecutionStatus());
+    }
+
+    [[nodiscard]] Result CurrentContext::EffectiveExecutionStatus() const
+    {
+        return std::max(executionStatus, nestedExecutionStatus);
+    }
+
     [[nodiscard]] Result CurrentContext::ExecutionStatus() const
     {
         return executionStatus;
     }
 
+    [[nodiscard]] Result CurrentContext::NestedExecutionStatus() const
+    {
+        return nestedExecutionStatus;
+    }
+
     void CurrentContext::Start()
     {
         traceTime.Start();
@@ -69,7 +88,16 @@ namespace cucumber_cpp::library::engine
             executionStatus = result;
 
         if (parent != nullptr)
-            parent->ExecutionStatus(result);
+            parent->NestedExecutionStatus(result);
+    }
+
+    void CurrentContext::NestedExecutionStatus(Result result)
+    {
+        if (result > nestedExecutionStatus)
+            nestedExecutionStatus = result;
+
+        if (parent != nullptr)
+            parent->NestedExecutionStatus(result);
     }
 
     ProgramContext::ProgramContext(std::shared_ptr<ContextStorageFactory> contextStorageFactory)
@@ -189,14 +217,11 @@ namespace cucumber_cpp::library::engine
     ContextManager::ScopedStepContext ContextManager::CreateStepContext(const StepInfo& stepInfo)
     {
         stepContext.push(std::make_shared<cucumber_cpp::library::engine::StepContext>(*scenarioContext, stepInfo));
-        runnerContext.push(stepContext.top());
-
         return ScopedStepContext{ *this };
     }
 
     void ContextManager::DisposeStepContext()
     {
-        runnerContext.pop();
         stepContext.pop();
     }
 
@@ -212,4 +237,12 @@ namespace cucumber_cpp::library::engine
     {
         return *runnerContext.top();
     }
+
+    cucumber_cpp::library::engine::RunnerContext* ContextManager::CurrentStepContext()
+    {
+        if (stepContext.empty())
+            return nullptr;
+
+        return stepContext.top().get();
+    }
 }

cucumber_cpp/library/engine/ContextManager.hpp

@@ -19,7 +19,10 @@ namespace cucumber_cpp::library::engine
         explicit CurrentContext(std::shared_ptr<ContextStorageFactory> contextStorageFactory);
         explicit CurrentContext(CurrentContext* parent);
 
+        [[nodiscard]] Result InheritedExecutionStatus() const;
+        [[nodiscard]] Result EffectiveExecutionStatus() const;
         [[nodiscard]] Result ExecutionStatus() const;
+        [[nodiscard]] Result NestedExecutionStatus() const;
 
         void Start();
         [[nodiscard]] TraceTime::Duration Duration() const;
@@ -28,9 +31,12 @@ namespace cucumber_cpp::library::engine
         void ExecutionStatus(Result result);
 
     private:
+        void NestedExecutionStatus(Result result);
+
         CurrentContext* parent{ nullptr };
 
         Result executionStatus{ Result::passed };
+        Result nestedExecutionStatus{ Result::passed };
         TraceTime traceTime;
     };
 
@@ -93,6 +99,7 @@ namespace cucumber_cpp::library::engine
         cucumber_cpp::library::engine::StepContext& StepContext();
 
         cucumber_cpp::library::engine::RunnerContext& CurrentContext();
+        cucumber_cpp::library::engine::RunnerContext* CurrentStepContext();
 
     private:
         void DisposeFeatureContext();

cucumber_cpp/library/engine/FailureHandler.cpp

@@ -53,6 +53,9 @@ namespace cucumber_cpp::library::engine
 
         contextManager.CurrentContext().ExecutionStatus(cucumber_cpp::library::engine::Result::failed);
 
+        if (auto* stepContext = contextManager.CurrentStepContext(); stepContext != nullptr)
+            stepContext->ExecutionStatus(cucumber_cpp::library::engine::Result::failed);
+
         reportHandler.Failure(message, relativeFilePath, line);
     }
 

cucumber_cpp/library/engine/HookExecutor.cpp

@@ -7,7 +7,9 @@
 #include "cucumber_cpp/library/engine/StepInfo.hpp"
 #include <functional>
 #include <set>
+#include <stdexcept>
 #include <string>
+#include <utility>
 
 namespace cucumber_cpp::library::engine
 {
@@ -20,20 +22,14 @@ namespace cucumber_cpp::library::engine
 
         void ExecuteHook(cucumber_cpp::library::engine::RunnerContext& runnerContext, HookType hook, const std::set<std::string, std::less<>>& tags)
         {
-            try
-            {
+            if (runnerContext.InheritedExecutionStatus() == Result::passed)
                 for (const auto& match : HookRegistry::Instance().Query(hook, tags))
                 {
                     match.factory(runnerContext)->Execute();
 
                     if (runnerContext.ExecutionStatus() != cucumber_cpp::library::engine::Result::passed)
                         return;
                 }
-            }
-            catch (...)
-            {
-                runnerContext.ExecutionStatus(cucumber_cpp::library::engine::Result::failed);
-            }
         }
     }
 

cucumber_cpp/library/engine/TestExecution.cpp

@@ -84,7 +84,8 @@ namespace cucumber_cpp::library::engine
     void TestExecutionImpl::RunStep(const cucumber_cpp::library::engine::StepInfo& stepInfo)
     {
         auto scopedContext = contextManager.CreateStepContext(stepInfo);
-        if (contextManager.ScenarioContext().ExecutionStatus() == cucumber_cpp::library::engine::Result::passed)
+        if (contextManager.ScenarioContext().InheritedExecutionStatus() == Result::passed &&
+            contextManager.ScenarioContext().EffectiveExecutionStatus() == Result::passed)
         {
             const auto scopedStepReport = reportHandler.StepStart();
             const auto scopedStepHook = hookExecution.StepStart();

cucumber_cpp/library/engine/TestRunner.cpp

@@ -35,11 +35,11 @@ namespace cucumber_cpp::library::engine
     {
     }
 
-    void TestRunnerImpl::Run(const std::vector<std::unique_ptr<FeatureInfo>>& feature)
+    void TestRunnerImpl::Run(const std::vector<std::unique_ptr<FeatureInfo>>& features)
     {
         auto scope = testExecution.StartRun();
 
-        for (const auto& featurePtr : feature)
+        for (const auto& featurePtr : features)
             RunFeature(*featurePtr);
     }