Issues: 997 - Adding beforeStep/afterStep hooks #1198
Conversation
|
@Eis95, @charlierudolph, could you please check this PR? |
|
Any updates on this PR? This functionality would solve a lot of our problems. Really hoping this gets in soon. |
|
I'm still waiting for some/any feedback, pinged a couple of guys here and in the issue ticket as well, no response yet. |
|
I'm also looking forward to have this solution |
|
@Andras-Marozsi I think one thing that's missing in this PR is actually .feature files for this new functionality. For a similar example, see the "before all hooks" feature: https://github.com/cucumber/cucumber-js/blob/master/features/before_after_all_hooks.feature Feature files provides some documentation around usage, too. You may be able to steal the starting point for that from #1058 |
|
Thanks @loganvolkers, will look at that PR for ideas. |
|
@loganvolkers, I updated the PR with the feature file. Could you be so kind and check it? Let me know if I should add anything else. Thanks! |
|
how can we help to merge this feature into the master? Thanks! |
|
@expphchen, find someone who's willing to provide feedback :) |
|
Sorry haven't really looked at this. There will probably be some changes needed for this after retry goes in as that has ended up being a major overhaul |
|
@charlierudolph, np, thanks for checking now. |
|
Alright retry is in. This should be good to start work on again. Sorry hadn't gotten around to reviewing this until now. I agree more with the process as defined in other PR (before / after steps aren't treated as full steps - don't appear in test case prepared, do not trigger test step events). Given the use case for this as extra validation / debugging I don't think need to be full steps. |
src/runtime/test_case_runner.js
Outdated
| text: step.text, | ||
| arguments: step.arguments, | ||
| location: Object.assign({ uri: this.testCase.uri }, step.locations[0]), | ||
| } |
There was a problem hiding this comment.
I'd suggest for beforeStep the hookParameter is {step, testCase: {sourceLocation, pickle}} where source location / pickle is what we pass to before / after hooks. And for afterStep we use {step, testCase: {sourceLocation, pickle}, result} where result is the step result.
There was a problem hiding this comment.
Ok, I will have a look towards the end of this week.
There was a problem hiding this comment.
I think I found a solution. Still need to test it, but will update the pr soon.
There was a problem hiding this comment.
@charlierudolph, updated the branch. It's not complete, I didn't add additional tests, just made sure nothing breaks. Can you please check it and let me know if you're ok with the solution?
There was a problem hiding this comment.
Sorry way late here. The current solution looks good to me. I like the cumulation of before step / step / after step results together. May be worth creating a "test_step_runner" class to encompass the new logic and to make testing a bit easier.
Also note, a lot of things have changed since you last merged master. (Gherkin 9 + cucumber-messages and the conversion to typescript). If you'd like help migrating in the latest changes, let me know.
Andras-Marozsi
force-pushed
the
add_step_hooks
branch
from
4e69cdb to
3afe03b
Compare
|
sorry didn't come back to check for a while, looks like the branch has conflict. would @Andras-Marozsi needs to resolve the conflict or @charlierudolph able to help. :) Thanks you guys on keep the ball rolling. |
Andras-Marozsi
force-pushed
the
add_step_hooks
branch
from
3afe03b to
210f318
Compare
|
@charlierudolph, I updated the PR, keeping the cumulation approach. I didn't add any new tests, and as of now this is breaking some existing tests, so there's some more work to be done on this PR, but before investing more time I want to double check you're ok with this solution. |
|
Updates look on the right track to me. I do think we should create a TestStepRunner class to encompass the new logic though and probably create a function to cumulate results (update status / message / durations) since appear doing that more now |
|
Closing this PR since beforeStep/AfterSteps Hooks was promoted in #1416 Thanks @Andras-Marozsi for the work you did. It was a big help in getting my PR for this completed and merged. |
This is my proposed solution for the problem: #997
Basically I'm treating the beforeStep/afterStep hooks as steps, which allowed me to integrate them with minimal change.
Please let me know what you think about this solution, or if you want me to update/change something.