Issue: 997 Start of BeforeStep functionality #1058
Conversation
src/runtime/test_case_runner.js
Outdated
| await this.runStepHooks(this.beforeStepHookDefinitions, { | ||
| sourceLocation: this.testCaseSourceLocation, | ||
| pickle: this.testCase.pickle, | ||
| }) |
There was a problem hiding this comment.
I think this should be within the call to this.aroundTestStep. That function emits the events test-step-started/finished. I think these before/after step hooks should actually be thought of as part of the step. We would run the beforeStep/step/afterStep and merge the results of each to get the cumulative step result. The step fails if the beforeStep, step, or afterStep fails. We would also not run the step if the beforeStep fails.
There was a problem hiding this comment.
does it make sense that i move into the runStep function? aroundTestStep is where I initially tried it but it gets called for each of the Before hooks. I think I would also need a check in aroundTestStep to make it work there.
src/runtime/test_case_runner.js
Outdated
| // TODO handle failure | ||
| await this.runStepHooks(this.beforeStepHookDefinitions, { | ||
| sourceLocation: this.testCaseSourceLocation, | ||
| pickle: this.testCase.pickle, |
There was a problem hiding this comment.
Hmm. I don't expect the beforeStep would need these parameters. I think we could start with passing in only the step. The after step will end up getting the step and the stepResult.
features/hooks.feature
Outdated
| BeforeStep(function() { throw 'Fail' }) | ||
| """ | ||
| When I run cucumber-js | ||
| Then it fails |
There was a problem hiding this comment.
Can we please put these new features in their own feature file? I think we will have enough new features by the end to warrant their own file. The world in hooks would get its own step hook feature
…turing failures and not running step on failure.
| } | ||
|
|
||
| setWorldConstructor(WorldConstructor) | ||
| """ |
There was a problem hiding this comment.
can move this to background
| }) | ||
| """ | ||
| When I run cucumber-js | ||
| Then it passes |
| .feature2Path}:2` | ||
| `${this.feature1Path}:1${separator.expected}${ | ||
| this.feature2Path | ||
| }:2` |
There was a problem hiding this comment.
I was getting a linting error here.
src/runtime/test_case_runner.js
Outdated
| const errors = responses.filter(response => response.exception) | ||
| if (errors && errors.length) { | ||
| return { | ||
| exception: 'BeforeStep hook failed', // TODO return stacktrace(s)? |
There was a problem hiding this comment.
I'm curious how we should handle multiple exceptions? And should I instead check if status is failed here instead of checking for exception in any of the responses? Or both?
src/runtime/test_case_runner.js
Outdated
| return { | ||
| status: Status.PASSED, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| async runStep(step) { |
There was a problem hiding this comment.
@charlierudolph BeforeStep/AfterStep are working. I do have questions about stacktrace reporting and duration calculation.
For duration my current thinking is to calculate the duration for the beforeSteps, the step, and the afterSteps and update that value on the stepResult prior to returning.
I'm interested in hearing what you feel would be the best way to handle multiple errors in the hooks .
src/runtime/test_case_runner.js
Outdated
| ) | ||
| if (beforeStepsResult.status !== Status.PASSED) { | ||
| return beforeStepsResult | ||
| } else { |
There was a problem hiding this comment.
Could you remove all extra else for more readability
src/runtime/test_case_runner.js
Outdated
| @@ -174,6 +188,13 @@ export default class TestCaseRunner { | |||
| return this.invokeStep(null, hookDefinition, hookParameter) | |||
| } | |||
|
|
|||
| async runStepHook(step, hookDefinition, hookParameter) { | |||
| if (this.skip) { | |||
There was a problem hiding this comment.
Looks like we already skip the steps inside runStep with isSkippingSteps check
| return { | ||
| status: Status.PASSED, | ||
| } | ||
| } |
There was a problem hiding this comment.
I disagree with how this function works. I think we should run the step hooks sequentially and if any does not pass we return the step result of the not passing hook. If all pass, we return status passed.
| status: Status.PASSED, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Thinking on this function, we are currently not adding the duration of the step hooks to the overall duration of the step. That should be testable with unit tests
|
|
||
| getInvocationParameters({ hookParameter }) { | ||
| return [hookParameter] | ||
| } |
There was a problem hiding this comment.
I don't think these hooks need invocation parameters (like what is passed to before/after hooks)
|
Has there been any progress on this issue? I have a project using an older version of CucumberJS and I think this is really the last piece that I need to bring it over. Is there anything I can do to help move this forward? |
|
@Eis95 are you still on this? This feature is pretty important, not only for me but for the testing community itself, e.g. for creating screenshots after each step and so on. |
This is not ready to be pulled in. I'm hung up on getting the step to fail when an error is thrown from the function passed to the
BeforeStep. There is a failing feature test for that scenario. It functions just like theBefore. There is also some code that could be refactored as it currently isn't DRY. But I wanted to make sure I was covering the 'fail' case first. I will also add theAfterStepas I assume it will work much like theBeforeStep.