-
-
Notifications
You must be signed in to change notification settings - Fork 2k
Add Step info to @BeforeStep and @AfterStep hooks #3139
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.
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
base: main
Are you sure you want to change the base?
Add Step info to @BeforeStep and @AfterStep hooks #3139
Conversation
Allow @BeforeStep and @afterstep hooks to receive step information (keyword, text, line number) alongside the existing Scenario parameter. New API: ```java @BeforeStep public void beforeStep(Scenario scenario, Step step) { System.out.println("Running: " + step.getKeyword() + step.getText()); System.out.println("At line: " + step.getLine()); } ``` Changes: - Add new io.cucumber.java.Step interface exposing step details - Add StepInfo wrapper class implementing Step interface - Extend HookDefinition with execute(state, step) default method - Update JavaHookDefinition to support 2-parameter step hooks - Pass step info through HookTestStep execution chain - Maintain full backward compatibility with existing hooks Closes cucumber#1805
mpkorstanje
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this looks like a decent approach.
-
The
HookTestStep::runmethod is duplicated. I don't think that this is necessary. InTestStep::executeStepit should be possible to pass the step argument and propagate it down toHookDefinition::executeby adding an extra argument toStepDefinitionMatch::runStep. If it goes unused in other places, that is okay. -
io.cucumber.plugin.event.StepArgumentshouldn't be visible to users but I don't know what to request that yet.
cucumber-core/src/main/java/io/cucumber/core/backend/HookDefinition.java
Outdated
Show resolved
Hide resolved
cucumber-core/src/main/java/io/cucumber/core/runner/HookTestStep.java
Outdated
Show resolved
Hide resolved
| * @param step the step being executed | ||
| * @throws Throwable if the hook fails | ||
| */ | ||
| public void runStep(TestCaseState state, io.cucumber.plugin.event.Step step) throws Throwable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code doesn't have to be duplicated. The StepDefinitionMatch interface is an package private interface. It is okay t o add parameter too it.
cucumber-java/src/main/java/io/cucumber/java/JavaHookDefinition.java
Outdated
Show resolved
Hide resolved
|
Don't worry about the Semver check btw. |
- Remove duplicated run method from HookTestStep, use parent's implementation - Pass step argument through TestStep::executeStep and ExecutionMode - Add runStep(state, step) default method to StepDefinitionMatch interface - Make HookDefinition.execute(state) a default no-op and deprecate it - Remove getArgument() from Step interface (StepArgument visibility concern) - Refactor JavaHookDefinition validation to explicitly check hook types - Update tests to match new error message format
|
pushed changes based on the comments, if you could re-review |
mpkorstanje
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please review your own code before asking me for a review.
- These changes came in incredibly fast.
- The changes are as requested in letter, but not in spirit. I find that I'm writing the same comments as I did in my previous review, but for exactly one level higher in the call chain.
- There is a rather obvious blunder in the code that regular refactoring tools wouldn't produce.
Normally I'd give you the benefit of the doubt but I do see several LLM based projects on your profile so I am skeptical that you've done the necessary work to create a correct pull request.
|
I will go through all my changes, one by one, apologies |
Summary
This PR implements the feature requested in #1805 - allowing
@BeforeStepand@AfterStephooks to receive step information (keyword, text, line number) alongside the existingScenarioparameter.New API:
Changes
io.cucumber.java.Stepinterface exposing step detailsStepInfowrapper class implementing theStepinterfaceHookDefinitionwithexecute(state, step)default methodJavaHookDefinitionto support 2-parameter step hooksHookTestStepexecution chainBackward Compatibility
Test plan
Closes #1805