-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Issue: 997 Start of BeforeStep functionality #1058
Changes from 3 commits
0abf18e
faa43e7
65c012a
47ffd9a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
Feature: Before and After Step Hooks | ||
|
||
Background: | ||
Given a file named "features/a.feature" with: | ||
""" | ||
Feature: some feature | ||
Scenario: some scenario | ||
Given a step | ||
""" | ||
And a file named "features/step_definitions/cucumber_steps.js" with: | ||
""" | ||
import {Given} from 'cucumber' | ||
|
||
Given(/^a step$/, function() {}) | ||
""" | ||
|
||
Scenario: Failing before step fails the scenario | ||
Given a file named "features/support/hooks.js" with: | ||
""" | ||
import {BeforeStep} from 'cucumber' | ||
|
||
BeforeStep(function() { throw 'Fail' }) | ||
""" | ||
When I run cucumber-js | ||
Then it fails | ||
|
||
Scenario: Failing after step fails the scenario | ||
Given a file named "features/support/hooks.js" with: | ||
""" | ||
import {AfterStep} from 'cucumber' | ||
|
||
AfterStep(function() { throw 'Fail' }) | ||
""" | ||
When I run cucumber-js | ||
Then it fails | ||
|
||
Scenario: Only run BeforeStep hooks with appropriate tags | ||
Given a file named "features/support/hooks.js" with: | ||
""" | ||
import { BeforeStep } from 'cucumber' | ||
|
||
BeforeStep('@any-tag', function() { | ||
throw Error("Would fail if ran") | ||
}) | ||
""" | ||
When I run cucumber-js | ||
Then it passes | ||
|
||
Scenario: Only run BeforeStep hooks with appropriate tags | ||
Given a file named "features/support/hooks.js" with: | ||
""" | ||
import { AfterStep } from 'cucumber' | ||
|
||
AfterStep('@any-tag', function() { | ||
throw Error("Would fail if ran") | ||
}) | ||
""" | ||
When I run cucumber-js | ||
Then it passes |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,75 @@ | ||
Feature: World in Hooks | ||
|
||
Background: | ||
Given a file named "features/a.feature" with: | ||
""" | ||
Feature: some feature | ||
Scenario: some scenario | ||
Given a step | ||
""" | ||
And a file named "features/step_definitions/cucumber_steps.js" with: | ||
""" | ||
import {Given} from 'cucumber' | ||
|
||
Given(/^a step$/, function() {}) | ||
""" | ||
And a file named "features/support/world.js" with: | ||
""" | ||
import {setWorldConstructor} from 'cucumber' | ||
|
||
function WorldConstructor() { | ||
return { | ||
isWorld: function() { return true } | ||
} | ||
} | ||
|
||
setWorldConstructor(WorldConstructor) | ||
""" | ||
|
||
Scenario: World is this in hooks | ||
Given a file named "features/support/hooks.js" with: | ||
""" | ||
import {After, Before } from 'cucumber' | ||
|
||
Before(function() { | ||
if (!this.isWorld()) { | ||
throw Error("Expected this to be world") | ||
} | ||
}) | ||
|
||
After(function() { | ||
if (!this.isWorld()) { | ||
throw Error("Expected this to be world") | ||
} | ||
}) | ||
""" | ||
When I run cucumber-js | ||
Then it passes | ||
|
||
Scenario: World is this in BeforeStep hooks | ||
Given a file named "features/support/hooks.js" with: | ||
""" | ||
import {BeforeStep } from 'cucumber' | ||
|
||
BeforeStep(function() { | ||
if (!this.isWorld()) { | ||
throw Error("Expected this to be world") | ||
} | ||
}) | ||
""" | ||
When I run cucumber-js | ||
Then it passes | ||
|
||
Scenario: World is this in AfterStep hooks | ||
Given a file named "features/support/hooks.js" with: | ||
""" | ||
import {AfterStep } from 'cucumber' | ||
|
||
AfterStep(function() { | ||
if (!this.isWorld()) { | ||
throw Error("Expected this to be world") | ||
} | ||
}) | ||
""" | ||
When I run cucumber-js | ||
Then it passes |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -120,8 +120,9 @@ describe('RerunFormatter', () => { | |
|
||
it('outputs the references needed to run the scenarios again', function() { | ||
expect(this.output).to.eql( | ||
`${this.feature1Path}:1${separator.expected}${this | ||
.feature2Path}:2` | ||
`${this.feature1Path}:1${separator.expected}${ | ||
this.feature2Path | ||
}:2` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was getting a linting error here. |
||
) | ||
}) | ||
}) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
import PickleFilter from '../pickle_filter' | ||
import StepDefinition from './step_definition' | ||
|
||
export default class TestStepHookDefinition extends StepDefinition { | ||
constructor(data) { | ||
super(data) | ||
this.pickleFilter = new PickleFilter({ | ||
tagExpression: this.options.tags, | ||
}) | ||
} | ||
|
||
appliesToTestCase({ pickle, uri }) { | ||
return this.pickleFilter.matches({ pickle, uri }) | ||
} | ||
|
||
getInvalidCodeLengthMessage() { | ||
return this.buildInvalidCodeLengthMessage('0 or 1', '2') | ||
} | ||
|
||
getInvocationParameters({ hookParameter }) { | ||
return [hookParameter] | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think these hooks need invocation parameters (like what is passed to before/after hooks) |
||
|
||
getValidCodeLengths() { | ||
return [0, 1, 2] | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,6 +29,8 @@ export default class TestCaseRunner { | |
parameters: worldParameters, | ||
}) | ||
this.beforeHookDefinitions = this.getBeforeHookDefinitions() | ||
this.afterStepHookDefinitions = this.getAfterStepHookDefinitions() | ||
this.beforeStepHookDefinitions = this.getBeforeStepHookDefinitions() | ||
this.afterHookDefinitions = this.getAfterHookDefinitions() | ||
this.testStepIndex = 0 | ||
this.result = { | ||
|
@@ -91,6 +93,18 @@ export default class TestCaseRunner { | |
) | ||
} | ||
|
||
getBeforeStepHookDefinitions() { | ||
return this.supportCodeLibrary.beforeTestStepHookDefinitions.filter( | ||
stepHookDefinition => stepHookDefinition.appliesToTestCase(this.testCase) | ||
) | ||
} | ||
|
||
getAfterStepHookDefinitions() { | ||
return this.supportCodeLibrary.afterTestStepHookDefinitions.filter( | ||
stepHookDefinition => stepHookDefinition.appliesToTestCase(this.testCase) | ||
) | ||
} | ||
|
||
getStepDefinitions(step) { | ||
return this.supportCodeLibrary.stepDefinitions.filter(stepDefinition => | ||
stepDefinition.matchesStepName({ | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like we already skip the steps inside There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. removed. |
||
return { status: Status.SKIPPED } | ||
} | ||
return this.invokeStep(step, hookDefinition, hookParameter) | ||
} | ||
|
||
async runHooks(hookDefinitions, hookParameter) { | ||
await Promise.each(hookDefinitions, async hookDefinition => { | ||
await this.aroundTestStep(() => | ||
|
@@ -182,6 +203,25 @@ export default class TestCaseRunner { | |
}) | ||
} | ||
|
||
async runStepHooks(step, hookDefinitions) { | ||
const stepHookResults = await Promise.all( | ||
hookDefinitions.map(async hookDefinition => | ||
this.runStepHook(step, hookDefinition) | ||
) | ||
) | ||
const errors = stepHookResults.filter(result => result.exception) | ||
if (errors && errors.length) { | ||
return { | ||
exception: 'Errors running step hooks', | ||
status: Status.FAILED, | ||
} | ||
} else { | ||
return { | ||
status: Status.PASSED, | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
||
async runStep(step) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @charlierudolph BeforeStep/AfterStep are working. I do have questions about stacktrace reporting and duration calculation. |
||
const stepDefinitions = this.getStepDefinitions(step) | ||
if (stepDefinitions.length === 0) { | ||
|
@@ -194,7 +234,28 @@ export default class TestCaseRunner { | |
} else if (this.isSkippingSteps()) { | ||
return { status: Status.SKIPPED } | ||
} | ||
return this.invokeStep(step, stepDefinitions[0]) | ||
const beforeStepsResult = await this.runStepHooks( | ||
step, | ||
this.beforeStepHookDefinitions | ||
) | ||
if (beforeStepsResult.status !== Status.PASSED) { | ||
return beforeStepsResult | ||
} else { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you remove all extra There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. removed |
||
const stepResult = await this.invokeStep(step, stepDefinitions[0]) | ||
if (stepResult.status !== Status.PASSED) { | ||
return stepResult | ||
} else { | ||
const afterStepResult = await this.runStepHooks( | ||
step, | ||
this.afterStepHookDefinitions | ||
) | ||
if (afterStepResult.status === Status.PASSED) { | ||
return stepResult | ||
} else { | ||
return afterStepResult | ||
} | ||
} | ||
} | ||
} | ||
|
||
async runSteps() { | ||
|
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.
can move this to background