feat(hooks): implement throw policies for hook execution and enhance error handling

Author: daantimmer

cucumber_cpp/library/engine/HookExecutor.cpp

@@ -24,14 +24,46 @@ namespace cucumber_cpp::library::engine
             using runtime_error::runtime_error;
         };
 
-        void ExecuteHook(cucumber_cpp::library::engine::RunnerContext& runnerContext, HookType hook, const std::set<std::string, std::less<>>& tags)
+        struct ThrowPolicy
+        {
+        protected:
+            ~ThrowPolicy() = default;
+
+        public:
+            virtual void Throw() const = 0;
+        };
+
+        struct ThrowExceptionPolicy : ThrowPolicy
+        {
+            ThrowExceptionPolicy(std::string message)
+                : message{ message }
+            {}
+
+            void Throw() const override
+            {
+                throw HookFailed{ message };
+            }
+
+        private:
+            std::string message;
+        };
+
+        struct NoThrowExceptionPolicy : ThrowPolicy
+        {
+            void Throw() const override
+            {
+                // No exception thrown
+            }
+        };
+
+        void ExecuteHook(cucumber_cpp::library::engine::RunnerContext& runnerContext, HookType hook, const std::set<std::string, std::less<>>& tags, const ThrowPolicy& throwPolicy)
         {
             for (const auto& match : HookRegistry::Instance().Query(hook, tags))
             {
                 match.factory(runnerContext)->Execute();
 
                 if (runnerContext.ExecutionStatus() != cucumber_cpp::library::engine::Result::passed)
-                    throw HookFailed{ "hook failed" };
+                    throwPolicy.Throw();
             }
         }
     }
@@ -41,31 +73,12 @@ namespace cucumber_cpp::library::engine
         , hookPair{ hookPair }
         , tags{ tags }
     {
-        try
-        {
-            ExecuteHook(runnerContext, hookPair.before, tags);
-        }
-        catch (HookFailed&)
-        {
-            /* This throw ensures no tests are executed in and below the current scope. */
-            throw HookFailed{ "Error during BEFORE_x_HOOK" };
-        }
+        ExecuteHook(runnerContext, hookPair.before, tags, ThrowExceptionPolicy{ "Hook failed" });
     }
 
     HookExecutor::ScopedHook::~ScopedHook()
     {
-        try
-        {
-            ExecuteHook(runnerContext, hookPair.after, tags);
-        }
-        catch (HookFailed&)
-        {
-            /*
-            Can't throw from a destructor. Error state will be handled by calling functions.
-
-            Errors during AFTER_ALL hook is not a problem, because no other hooks will be executed.
-            */
-        }
+        ExecuteHook(runnerContext, hookPair.after, tags, NoThrowExceptionPolicy{});
     }
 
     HookExecutor::ProgramScope::ProgramScope(cucumber_cpp::library::engine::ContextManager& contextManager)

cucumber_cpp/library/engine/TestRunner.cpp

@@ -36,17 +36,17 @@ 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 run = [this, &feature]
+        auto runFeatures = [this, &features]
         {
             auto scope = testExecution.StartRun();
 
-            for (const auto& featurePtr : feature)
+            for (const auto& featurePtr : features)
                 RunFeature(*featurePtr);
         };
 
-        ASSERT_NO_THROW(run());
+        ASSERT_NO_THROW(runFeatures());
     }
 
     void TestRunnerImpl::NestedStep(StepType type, std::string step)
@@ -57,7 +57,7 @@ namespace cucumber_cpp::library::engine
 
     void TestRunnerImpl::RunFeature(const FeatureInfo& feature)
     {
-        auto run = [this, &feature]
+        auto runFeature = [this, &feature]
         {
             if (feature.Rules().empty() && feature.Scenarios().empty())
                 return;
@@ -67,18 +67,18 @@ namespace cucumber_cpp::library::engine
             RunRules(feature.Rules());
             RunScenarios(feature.Scenarios());
         };
-        ASSERT_NO_THROW(run());
+        ASSERT_NO_THROW(runFeature());
     }
 
     void TestRunnerImpl::RunRule(const RuleInfo& rule)
     {
-        auto run = [this, &rule]
+        auto runRule = [this, &rule]
         {
             const auto ruleScope = testExecution.StartRule(rule);
 
             RunScenarios(rule.Scenarios());
         };
-        ASSERT_NO_THROW(run());
+        ASSERT_NO_THROW(runRule());
     }
 
     void TestRunnerImpl::RunRules(const std::vector<std::unique_ptr<RuleInfo>>& rules)
@@ -89,15 +89,15 @@ namespace cucumber_cpp::library::engine
 
     void TestRunnerImpl::RunScenario(const ScenarioInfo& scenario)
     {
-        auto run = [this, &scenario]
+        auto runScenario = [this, &scenario]
         {
             const auto scenarioScope = testExecution.StartScenario(scenario);
 
             currentScenario = &scenario;
 
             ExecuteSteps(scenario);
         };
-        ASSERT_NO_THROW(run());
+        ASSERT_NO_THROW(runScenario());
     }
 
     void TestRunnerImpl::RunScenarios(const std::vector<std::unique_ptr<ScenarioInfo>>& scenarios)

cucumber_cpp/library/engine/test/TestFailureHandler.cpp

@@ -2,13 +2,9 @@
 #include "cucumber_cpp/library/engine/FailureHandler.hpp"
 #include "cucumber_cpp/library/engine/Result.hpp"
 #include "cucumber_cpp/library/engine/test_helper/ContextManagerInstance.hpp"
-#include "cucumber_cpp/library/report/Report.hpp"
-#include <cstddef>
-#include <filesystem>
+#include "cucumber_cpp/library/report/test_helper/ReportForwarderMock.hpp"
 #include <gmock/gmock.h>
 #include <gtest/gtest.h>
-#include <optional>
-#include <string>
 
 namespace cucumber_cpp::library::engine
 {
@@ -29,20 +25,11 @@ namespace cucumber_cpp::library::engine
             CucumberAssertHelper{ testing::TestPartResult::kNonFatalFailure, "custom_file.cpp", 0, failure } = testing::Message() << user;
         }
 
-        struct ReportForwarderMock : report::ReportForwarderImpl
-        {
-            using ReportForwarderImpl::ReportForwarderImpl;
-            virtual ~ReportForwarderMock() = default;
-
-            MOCK_METHOD(void, Failure, (const std::string& error, std::optional<std::filesystem::path> path, std::optional<std::size_t> line, std::optional<std::size_t> column), (override));
-            MOCK_METHOD(void, Error, (const std::string& error, std::optional<std::filesystem::path> path, std::optional<std::size_t> line, std::optional<std::size_t> column), (override));
-        };
-
         struct TestFailureHandler : testing::Test
         {
 
             test_helper::ContextManagerInstance contextManager;
-            ReportForwarderMock reportHandler{ contextManager };
+            report::test_helper::ReportForwarderMock reportHandler{ contextManager };
             TestAssertionHandlerImpl testAssertionHandler{ contextManager, reportHandler };
         };
     }

cucumber_cpp/library/engine/test/TestHookExecutor.cpp

@@ -1,13 +1,20 @@
+#include "cucumber_cpp/library/engine/FailureHandler.hpp"
 #include "cucumber_cpp/library/engine/HookExecutor.hpp"
 #include "cucumber_cpp/library/engine/test_helper/ContextManagerInstance.hpp"
+#include "cucumber_cpp/library/report/test_helper/ReportForwarderMock.hpp"
+#include <functional>
 #include <gtest/gtest.h>
+#include <optional>
+#include <set>
+#include <string>
+#include <utility>
 
 namespace cucumber_cpp::library::engine
 {
     struct TestHookExecutor : testing::Test
     {
-        test_helper::ContextManagerInstance contextManagerInstance;
-        HookExecutorImpl hookExecutor{ contextManagerInstance };
+        std::optional<test_helper::ContextManagerInstance> contextManagerInstance{ std::in_place };
+        std::optional<HookExecutorImpl> hookExecutor{ *contextManagerInstance };
     };
 
     TEST_F(TestHookExecutor, Construct)
@@ -16,49 +23,60 @@ namespace cucumber_cpp::library::engine
 
     TEST_F(TestHookExecutor, ExecuteProgramHooks)
     {
-        ASSERT_FALSE(contextManagerInstance.ProgramContext().Contains("hookBeforeAll"));
-        ASSERT_FALSE(contextManagerInstance.ProgramContext().Contains("hookAfterAll"));
+        ASSERT_FALSE(contextManagerInstance->ProgramContext().Contains("hookBeforeAll"));
+        ASSERT_FALSE(contextManagerInstance->ProgramContext().Contains("hookAfterAll"));
 
         {
-            auto scope = hookExecutor.BeforeAll();
-            EXPECT_TRUE(contextManagerInstance.ProgramContext().Contains("hookBeforeAll"));
+            auto scope = hookExecutor->BeforeAll();
+            EXPECT_TRUE(contextManagerInstance->ProgramContext().Contains("hookBeforeAll"));
         }
-        EXPECT_TRUE(contextManagerInstance.ProgramContext().Contains("hookAfterAll"));
+        EXPECT_TRUE(contextManagerInstance->ProgramContext().Contains("hookAfterAll"));
     }
 
     TEST_F(TestHookExecutor, ExecuteBeforeFeature)
     {
-        ASSERT_FALSE(contextManagerInstance.FeatureContext().Contains("hookBeforeFeature"));
-        ASSERT_FALSE(contextManagerInstance.FeatureContext().Contains("hookAfterFeature"));
+        ASSERT_FALSE(contextManagerInstance->FeatureContext().Contains("hookBeforeFeature"));
+        ASSERT_FALSE(contextManagerInstance->FeatureContext().Contains("hookAfterFeature"));
 
         {
-            auto scope = hookExecutor.FeatureStart();
-            EXPECT_TRUE(contextManagerInstance.FeatureContext().Contains("hookBeforeFeature"));
+            auto scope = hookExecutor->FeatureStart();
+            EXPECT_TRUE(contextManagerInstance->FeatureContext().Contains("hookBeforeFeature"));
         }
-        EXPECT_TRUE(contextManagerInstance.FeatureContext().Contains("hookAfterFeature"));
+        EXPECT_TRUE(contextManagerInstance->FeatureContext().Contains("hookAfterFeature"));
     }
 
     TEST_F(TestHookExecutor, ExecuteBeforeScenario)
     {
-        ASSERT_FALSE(contextManagerInstance.ScenarioContext().Contains("hookBeforeScenario"));
-        ASSERT_FALSE(contextManagerInstance.ScenarioContext().Contains("hookAfterScenario"));
+        ASSERT_FALSE(contextManagerInstance->ScenarioContext().Contains("hookBeforeScenario"));
+        ASSERT_FALSE(contextManagerInstance->ScenarioContext().Contains("hookAfterScenario"));
 
         {
-            auto scope = hookExecutor.ScenarioStart();
-            EXPECT_TRUE(contextManagerInstance.ScenarioContext().Contains("hookBeforeScenario"));
+            auto scope = hookExecutor->ScenarioStart();
+            EXPECT_TRUE(contextManagerInstance->ScenarioContext().Contains("hookBeforeScenario"));
         }
-        EXPECT_TRUE(contextManagerInstance.ScenarioContext().Contains("hookAfterScenario"));
+        EXPECT_TRUE(contextManagerInstance->ScenarioContext().Contains("hookAfterScenario"));
     }
 
     TEST_F(TestHookExecutor, ExecuteBeforeStep)
     {
-        ASSERT_FALSE(contextManagerInstance.ScenarioContext().Contains("hookBeforeStep"));
-        ASSERT_FALSE(contextManagerInstance.ScenarioContext().Contains("hookAfterStep"));
+        ASSERT_FALSE(contextManagerInstance->ScenarioContext().Contains("hookBeforeStep"));
+        ASSERT_FALSE(contextManagerInstance->ScenarioContext().Contains("hookAfterStep"));
 
         {
-            auto scope = hookExecutor.StepStart();
-            EXPECT_TRUE(contextManagerInstance.ScenarioContext().Contains("hookBeforeStep"));
+            auto scope = hookExecutor->StepStart();
+            EXPECT_TRUE(contextManagerInstance->ScenarioContext().Contains("hookBeforeStep"));
         }
-        EXPECT_TRUE(contextManagerInstance.ScenarioContext().Contains("hookAfterStep"));
+        EXPECT_TRUE(contextManagerInstance->ScenarioContext().Contains("hookAfterStep"));
+    }
+
+    TEST_F(TestHookExecutor, ThrowOnBeforeHookError)
+    {
+        contextManagerInstance.emplace(std::set<std::string, std::less<>>{ "@errorbefore" });
+        hookExecutor.emplace(*contextManagerInstance);
+
+        report::test_helper::ReportForwarderMock reportHandler{ *contextManagerInstance };
+        TestAssertionHandlerImpl assertionHandler{ *contextManagerInstance, reportHandler };
+
+        EXPECT_ANY_THROW(auto scope = hookExecutor->StepStart());
     }
 }

cucumber_cpp/library/engine/test/TestHookExecutorHooks.cpp

@@ -43,4 +43,9 @@ namespace cucumber_cpp::library::engine
     {
         context.InsertAt("hookAfterStep", std::string{ "hookAfterStep" });
     }
+
+    HOOK_BEFORE_STEP("@errorbefore")
+    {
+        ASSERT_FALSE(true);
+    }
 }