Skip to content
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

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
18 changes: 17 additions & 1 deletion features/hooks.feature
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,16 @@ Feature: Environment Hooks
Then it fails
And the "After" hook has status "passed"

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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Definitely


Scenario: World is this in hooks
Given a file named "features/support/world.js" with:
"""
Expand All @@ -78,14 +88,20 @@ Feature: Environment Hooks
"""
Given a file named "features/support/hooks.js" with:
"""
import {After, Before} from 'cucumber'
import {After, Before, BeforeStep } from 'cucumber'

Before(function() {
if (!this.isWorld()) {
throw Error("Expected this to be world")
}
})

BeforeStep(function() {
if (!this.isWorld()) {
throw Error("Expected this to be world")
}
})

After(function() {
if (!this.isWorld()) {
throw Error("Expected this to be world")
Expand Down
5 changes: 3 additions & 2 deletions src/formatter/rerun_formatter_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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`
Copy link
Author

Choose a reason for hiding this comment

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

I was getting a linting error here.

)
})
})
Expand Down
1 change: 1 addition & 0 deletions src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ const { methods } = supportCodeLibraryBuilder
export const After = methods.After
export const AfterAll = methods.AfterAll
export const Before = methods.Before
export const BeforeStep = methods.BeforeStep
export const BeforeAll = methods.BeforeAll
export const defineParameterType = methods.defineParameterType
export const defineStep = methods.defineStep
Expand Down
27 changes: 27 additions & 0 deletions src/models/test_step_hook_definition.js
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]
}
Copy link
Member

Choose a reason for hiding this comment

The 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]
}
}
19 changes: 19 additions & 0 deletions src/runtime/test_case_runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ export default class TestCaseRunner {
parameters: worldParameters,
})
this.beforeHookDefinitions = this.getBeforeHookDefinitions()
this.beforeStepHookDefinitions = this.getBeforeStepHookDefinitions()
this.afterHookDefinitions = this.getAfterHookDefinitions()
this.testStepIndex = 0
this.result = {
Expand Down Expand Up @@ -91,6 +92,12 @@ export default class TestCaseRunner {
)
}

getBeforeStepHookDefinitions() {
return this.supportCodeLibrary.beforeTestStepHookDefinitions.filter(
stepHookDefinition => stepHookDefinition.appliesToTestCase(this.testCase)
)
}

getStepDefinitions(step) {
return this.supportCodeLibrary.stepDefinitions.filter(stepDefinition =>
stepDefinition.matchesStepName({
Expand Down Expand Up @@ -182,6 +189,12 @@ export default class TestCaseRunner {
})
}

async runStepHooks(hookDefinitions, hookParameter) {
await Promise.each(hookDefinitions, async hookDefinition => {
await this.runHook(hookDefinition, hookParameter)
})
}
Copy link
Member

Choose a reason for hiding this comment

The 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) {
Copy link
Author

Choose a reason for hiding this comment

The 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.
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 .

const stepDefinitions = this.getStepDefinitions(step)
if (stepDefinitions.length === 0) {
Expand All @@ -199,7 +212,13 @@ export default class TestCaseRunner {

async runSteps() {
await Promise.each(this.testCase.pickle.steps, async step => {
// TODO handle failure
await this.runStepHooks(this.beforeStepHookDefinitions, {
sourceLocation: this.testCaseSourceLocation,
pickle: this.testCase.pickle,
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

will fix

})
Copy link
Member

@charlierudolph charlierudolph Apr 3, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

await this.aroundTestStep(() => this.runStep(step))
// TODO run after step hooks
})
}
}
83 changes: 83 additions & 0 deletions src/runtime/test_case_runner_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { afterEach, beforeEach, describe, it } from 'mocha'
import { expect } from 'chai'
import sinon from 'sinon'
import TestCaseHookDefinition from '../models/test_case_hook_definition'
import TestStepHookDefinition from '../models/test_step_hook_definition'
import TestCaseRunner from './test_case_runner'
import Status from '../status'
import StepRunner from './step_runner'
Expand Down Expand Up @@ -29,6 +30,7 @@ describe('TestCaseRunner', () => {
}
this.supportCodeLibrary = {
afterTestCaseHookDefinitions: [],
beforeTestStepHookDefinitions: [],
beforeTestCaseHookDefinitions: [],
defaultTimeout: 5000,
stepDefinitions: [],
Expand Down Expand Up @@ -148,6 +150,87 @@ describe('TestCaseRunner', () => {
})
})

describe('with a passing step and a before step hook', () => {
beforeEach(async function() {
const testStepHookDefinition = new TestStepHookDefinition({
code() {
throw new Error('error')
},
line: 4,
options: {},
uri: 'path/to/hooks',
})
this.supportCodeLibrary.beforeTestStepHookDefinitions = [
testStepHookDefinition,
]
this.step = { uri: 'path/to/feature', locations: [{ line: 2 }] }
this.stepResult = {
duration: 1,
status: Status.PASSED,
}
const stepDefinition = {
uri: 'path/to/steps',
line: 3,
matchesStepName: sinon.stub().returns(true),
}
StepRunner.run.resolves(this.stepResult)
this.supportCodeLibrary.stepDefinitions = [stepDefinition]
this.testCase.pickle.steps = [this.step]
const scenarioRunner = new TestCaseRunner({
eventBroadcaster: this.eventBroadcaster,
skip: false,
testCase: this.testCase,
supportCodeLibrary: this.supportCodeLibrary,
})
await scenarioRunner.run()
})

it('emits test-case-prepared', function() {
expect(this.onTestCasePrepared).to.have.callCount(1)
expect(this.onTestCasePrepared).to.have.been.calledWith({
steps: [
{
actionLocation: { line: 3, uri: 'path/to/steps' },
sourceLocation: { line: 2, uri: 'path/to/feature' },
},
],
sourceLocation: { line: 1, uri: 'path/to/feature' },
})
})

it('emits test-case-started', function() {
expect(this.onTestCaseStarted).to.have.callCount(1)
expect(this.onTestCaseStarted).to.have.been.calledWith({
sourceLocation: { line: 1, uri: 'path/to/feature' },
})
})

it('emits test-step-started', function() {
expect(this.onTestStepStarted).to.have.callCount(1)
expect(this.onTestStepStarted).to.have.been.calledWith({
index: 0,
testCase: { sourceLocation: { line: 1, uri: 'path/to/feature' } },
})
})

it('emits test-step-finished', function() {
expect(this.onTestStepFinished).to.have.callCount(1)
expect(this.onTestStepFinished).to.have.been.calledWith({
index: 0,
testCase: { sourceLocation: { line: 1, uri: 'path/to/feature' } },
result: { duration: 1, status: Status.PASSED },
})
})

it('emits test-case-finished', function() {
expect(this.onTestCaseFinished).to.have.callCount(1)
expect(this.onTestCaseFinished).to.have.been.calledWith({
result: { duration: 1, status: Status.PASSED },
sourceLocation: { line: 1, uri: 'path/to/feature' },
})
})
})

describe('with a failing step', () => {
beforeEach(async function() {
this.step = { uri: 'path/to/feature', locations: [{ line: 2 }] }
Expand Down
25 changes: 25 additions & 0 deletions src/support_code_library_builder/define_helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,35 @@ import { ParameterType } from 'cucumber-expressions'
import path from 'path'
import StackTrace from 'stacktrace-js'
import StepDefinition from '../models/step_definition'
import TestStepHookDefinition from '../models/test_step_hook_definition'
import TestCaseHookDefinition from '../models/test_case_hook_definition'
import TestRunHookDefinition from '../models/test_run_hook_definition'
import validateArguments from './validate_arguments'

export function defineTestStepHook(builder, collectionName) {
return (options, code) => {
if (typeof options === 'string') {
options = { tags: options }
} else if (typeof options === 'function') {
code = options
options = {}
}
const { line, uri } = getDefinitionLineAndUri(builder.cwd)
validateArguments({
args: { code, options },
fnName: 'defineTestStepHook',
location: formatLocation({ line, uri }),
})
const hookDefinition = new TestStepHookDefinition({
code,
line,
options,
uri,
})
builder.options[collectionName].push(hookDefinition)
}
}

export function defineTestCaseHook(builder, collectionName) {
return (options, code) => {
if (typeof options === 'string') {
Expand Down
4 changes: 4 additions & 0 deletions src/support_code_library_builder/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {
defineTestRunHook,
defineParameterType,
defineTestCaseHook,
defineTestStepHook,
defineStep,
} from './define_helpers'
import { wrapDefinitions } from './finalize_helpers'
Expand All @@ -15,6 +16,7 @@ export class SupportCodeLibraryBuilder {
defineParameterType: defineParameterType(this),
After: defineTestCaseHook(this, 'afterTestCaseHookDefinitions'),
AfterAll: defineTestRunHook(this, 'afterTestRunHookDefinitions'),
BeforeStep: defineTestStepHook(this, 'beforeTestStepHookDefinitions'),
Before: defineTestCaseHook(this, 'beforeTestCaseHookDefinitions'),
BeforeAll: defineTestRunHook(this, 'beforeTestRunHookDefinitions'),
defineStep: defineStep(this),
Expand All @@ -41,6 +43,7 @@ export class SupportCodeLibraryBuilder {
definitions: _.chain([
'afterTestCaseHook',
'afterTestRunHook',
'beforeTestStepHook',
'beforeTestCaseHook',
'beforeTestRunHook',
'step',
Expand All @@ -59,6 +62,7 @@ export class SupportCodeLibraryBuilder {
this.options = _.cloneDeep({
afterTestCaseHookDefinitions: [],
afterTestRunHookDefinitions: [],
beforeTestStepHookDefinitions: [],
beforeTestCaseHookDefinitions: [],
beforeTestRunHookDefinitions: [],
defaultTimeout: 5000,
Expand Down
79 changes: 79 additions & 0 deletions src/support_code_library_builder/index_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -184,4 +184,83 @@ describe('supportCodeLibraryBuilder', () => {
})
})
})
describe('this.BeforeStep', () => {
describe('function only', () => {
beforeEach(function() {
this.hook = function() {}
supportCodeLibraryBuilder.reset('path/to/project')
supportCodeLibraryBuilder.methods.BeforeStep(this.hook) // eslint-disable-line babel/new-cap
this.options = supportCodeLibraryBuilder.finalize()
})

it('adds a step hook definition', function() {
expect(this.options.beforeTestStepHookDefinitions).to.have.lengthOf(1)
expect(this.options.beforeTestStepHookDefinitions[0].code).to.eql(
this.hook
)
})
})

describe('tag and function', () => {
beforeEach(function() {
this.hook = function() {}
supportCodeLibraryBuilder.reset('path/to/project')
supportCodeLibraryBuilder.methods.BeforeStep('@tagA', this.hook) // eslint-disable-line babel/new-cap
this.options = supportCodeLibraryBuilder.finalize()
})

it('adds a step hook definition', function() {
expect(this.options.beforeTestStepHookDefinitions).to.have.lengthOf(1)
expect(
this.options.beforeTestStepHookDefinitions[0].options.tags
).to.eql('@tagA')
expect(this.options.beforeTestStepHookDefinitions[0].code).to.eql(
this.hook
)
})
})

describe('options and function', () => {
beforeEach(function() {
this.hook = function() {}
supportCodeLibraryBuilder.reset('path/to/project')
supportCodeLibraryBuilder.methods.BeforeStep(
{ tags: '@tagA' },
this.hook
) // eslint-disable-line babel/new-cap
this.options = supportCodeLibraryBuilder.finalize()
})

it('adds a step hook definition', function() {
expect(this.options.beforeTestStepHookDefinitions).to.have.lengthOf(1)
expect(
this.options.beforeTestStepHookDefinitions[0].options.tags
).to.eql('@tagA')
expect(this.options.beforeTestStepHookDefinitions[0].code).to.eql(
this.hook
)
})
})

describe('multiple', () => {
beforeEach(function() {
this.hook1 = function hook1() {}
this.hook2 = function hook2() {}
supportCodeLibraryBuilder.reset('path/to/project')
supportCodeLibraryBuilder.methods.BeforeStep(this.hook1) // eslint-disable-line babel/new-cap
supportCodeLibraryBuilder.methods.BeforeStep(this.hook2) // eslint-disable-line babel/new-cap
this.options = supportCodeLibraryBuilder.finalize()
})

it('adds the step hook definitions in the order of definition', function() {
expect(this.options.beforeTestStepHookDefinitions).to.have.lengthOf(2)
expect(this.options.beforeTestStepHookDefinitions[0].code).to.eql(
this.hook1
)
expect(this.options.beforeTestStepHookDefinitions[1].code).to.eql(
this.hook2
)
})
})
})
})
Loading