Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
daantimmer merged 6 commits into from
May 3, 2025
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7d9bd7f
fix(hooks): don't execute the current scope when an error occurs duri…
daantimmer May 2, 2025
e9896b4
Merge branch 'main' into feature/handle-errors-during-hook-execution
daantimmer May 3, 2025
7d4bb7c
feat(hooks): implement throw policies for hook execution and enhance …
daantimmer May 3, 2025
227e678
fix(tests): update hook execution test to use lambda and improve mock…
daantimmer May 3, 2025
87f451a
refactor hook execution error handling to allow skipping tests
daantimmer May 3, 2025
3dc0c50
refactor(hooks): simplify hook execution by removing throw policies
daantimmer May 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 43 additions & 12 deletions cucumber_cpp/library/engine/HookExecutor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include "cucumber_cpp/library/engine/StepInfo.hpp"
#include <functional>
#include <set>
#include <stdexcept>
#include <string>

namespace cucumber_cpp::library::engine
Expand All @@ -18,21 +19,51 @@ namespace cucumber_cpp::library::engine
constexpr HookPair scenarioHooks{ HookType::before, HookType::after };
constexpr HookPair stepHooks{ HookType::beforeStep, HookType::afterStep };

void ExecuteHook(cucumber_cpp::library::engine::RunnerContext& runnerContext, HookType hook, const std::set<std::string, std::less<>>& tags)
struct HookFailed : std::runtime_error
{
try
using runtime_error::runtime_error;
};

struct ThrowPolicy
{
protected:
~ThrowPolicy() = default;

public:
virtual void Throw() const = 0;
};

struct ThrowExceptionPolicy : ThrowPolicy
{
ThrowExceptionPolicy(std::string message)
: message{ message }
{}

void Throw() const override
{
for (const auto& match : HookRegistry::Instance().Query(hook, tags))
{
match.factory(runnerContext)->Execute();
throw HookFailed{ message };
}

if (runnerContext.ExecutionStatus() != cucumber_cpp::library::engine::Result::passed)
return;
}
private:
std::string message;
};

struct NoThrowExceptionPolicy : ThrowPolicy
{
void Throw() const override
{
// No exception thrown
}
catch (...)
};

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))
{
runnerContext.ExecutionStatus(cucumber_cpp::library::engine::Result::failed);
match.factory(runnerContext)->Execute();

if (runnerContext.ExecutionStatus() != cucumber_cpp::library::engine::Result::passed)
throwPolicy.Throw();
}
}
}
Expand All @@ -42,12 +73,12 @@ namespace cucumber_cpp::library::engine
, hookPair{ hookPair }
, tags{ tags }
{
ExecuteHook(runnerContext, hookPair.before, tags);
ExecuteHook(runnerContext, hookPair.before, tags, ThrowExceptionPolicy{ "Hook failed" });
Copy link

Copilot AI May 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ExecuteHook function now accepts a ThrowPolicy parameter but does not invoke throwPolicy.Throw() anywhere in its implementation. Consider adding logic to call throwPolicy.Throw() when a hook error is detected to ensure errors are properly propagated.

Copilot uses AI. Check for mistakes.
}

HookExecutor::ScopedHook::~ScopedHook()
{
ExecuteHook(runnerContext, hookPair.after, tags);
ExecuteHook(runnerContext, hookPair.after, tags, NoThrowExceptionPolicy{});
}

HookExecutor::ProgramScope::ProgramScope(cucumber_cpp::library::engine::ContextManager& contextManager)
Expand Down
46 changes: 32 additions & 14 deletions cucumber_cpp/library/engine/TestRunner.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@

#include "cucumber_cpp/library/engine/TestRunner.hpp"
#include "cucumber_cpp/library/BodyMacro.hpp"
#include "cucumber_cpp/library/engine/FeatureFactory.hpp"
#include "cucumber_cpp/library/engine/FeatureInfo.hpp"
#include "cucumber_cpp/library/engine/RuleInfo.hpp"
Expand Down Expand Up @@ -35,12 +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 scope = testExecution.StartRun();
auto runFeatures = [this, &features]
{
auto scope = testExecution.StartRun();

for (const auto& featurePtr : feature)
RunFeature(*featurePtr);
for (const auto& featurePtr : features)
RunFeature(*featurePtr);
};

ASSERT_NO_THROW(runFeatures());
}

void TestRunnerImpl::NestedStep(StepType type, std::string step)
Expand All @@ -51,20 +57,28 @@ namespace cucumber_cpp::library::engine

void TestRunnerImpl::RunFeature(const FeatureInfo& feature)
{
if (feature.Rules().empty() && feature.Scenarios().empty())
return;
auto runFeature = [this, &feature]
{
if (feature.Rules().empty() && feature.Scenarios().empty())
return;

const auto featureScope = testExecution.StartFeature(feature);
const auto featureScope = testExecution.StartFeature(feature);

RunRules(feature.Rules());
RunScenarios(feature.Scenarios());
RunRules(feature.Rules());
RunScenarios(feature.Scenarios());
};
ASSERT_NO_THROW(runFeature());
}

void TestRunnerImpl::RunRule(const RuleInfo& rule)
{
const auto ruleScope = testExecution.StartRule(rule);
auto runRule = [this, &rule]
{
const auto ruleScope = testExecution.StartRule(rule);

RunScenarios(rule.Scenarios());
RunScenarios(rule.Scenarios());
};
ASSERT_NO_THROW(runRule());
}

void TestRunnerImpl::RunRules(const std::vector<std::unique_ptr<RuleInfo>>& rules)
Expand All @@ -75,11 +89,15 @@ namespace cucumber_cpp::library::engine

void TestRunnerImpl::RunScenario(const ScenarioInfo& scenario)
{
const auto scenarioScope = testExecution.StartScenario(scenario);
auto runScenario = [this, &scenario]
{
const auto scenarioScope = testExecution.StartScenario(scenario);

currentScenario = &scenario;
currentScenario = &scenario;

ExecuteSteps(scenario);
ExecuteSteps(scenario);
};
ASSERT_NO_THROW(runScenario());
}

void TestRunnerImpl::RunScenarios(const std::vector<std::unique_ptr<ScenarioInfo>>& scenarios)
Expand Down
17 changes: 2 additions & 15 deletions cucumber_cpp/library/engine/test/TestFailureHandler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand All @@ -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 };
};
}
Expand Down
62 changes: 40 additions & 22 deletions cucumber_cpp/library/engine/test/TestHookExecutor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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)
Expand All @@ -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());
}
}
5 changes: 5 additions & 0 deletions cucumber_cpp/library/engine/test/TestHookExecutorHooks.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,4 +43,9 @@ namespace cucumber_cpp::library::engine
{
context.InsertAt("hookAfterStep", std::string{ "hookAfterStep" });
}

HOOK_BEFORE_STEP("@errorbefore")
{
ASSERT_FALSE(true);
}
}
Loading
Loading