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

pass the step definition options to function wrapper #838

Merged

Conversation

lgandecki
Copy link
Contributor

It would be helpful to have the options available in the wrapper.
Then I can do something like this:

defineSupportCode(function ({setDefinitionFunctionWrapper}) {
    setDefinitionFunctionWrapper(function (fn, options) {
        let retryTest = isFinite(options.retry) ? parseInt(options.retry, 10) : 0
        let wrappedFunction = isAsync(fn) || config.sync === false
            ? wrapStepAsync(fn, retryTest) : wrapStepSync(fn, retryTest)
        return wrappedFunction
    })
})

I'm currently writing a "wdio-cucumber2-framework" (which is a wrapper around cucumber 2 to allow webdriverio runner run cucumber tests), and this is the cleanest way for me to implement retrying (this is how it was done in wdio-cucumber-framework as well , with the options)

Please let me know what you think, I hope you don't feel like this is too much of a feature creep. :-)

Thanks!

@charlierudolph
Copy link
Member

Can you please add a feature test for this where the options are made use of? Maybe something similar to this feature?

@lgandecki
Copy link
Contributor Author

@charlierudolph Thanks for the feedback!
I added the feature test. It's a bit tricky because I can't think of a good way of using the options inside the wrapper if not with an external runner - which would require quite a complex test here.
It does check whether the options are passed and can be used.
Let me know if you think of a better way to test this, or better way to name the steps/scenarios. :) I always find it hard to write human-readable specification for such a low level functionality

import {defineSupportCode} from 'cucumber'

defineSupportCode(({Then, When}) => {
When(/^I run a step with options$/, {retry: 2}, function () {
Copy link
Member

Choose a reason for hiding this comment

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

I'm hesitant about this as it means options can now be filled with non-standard options. Thoughts on putting the the wrapper options in a specific child wrapperOptions. Thus this would become

When(/^I run a step with options$/, {wrapperOptions: {retry: 2}}, function () {

And in setDefinitionFunctionWrapper it is passed the fn and options.wrapperOptions

Copy link
Contributor Author

@lgandecki lgandecki Jun 1, 2017

Choose a reason for hiding this comment

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

I don't have a strong preference, besides the fact that {wrapperOptions: {}} introduces noise to otherwise such a nice an readable API :) .. it will also differ from the current implementation of retry in the wdio-cucumber-framework - although it doesn't have to be backward compatible.

Maybe @christian-bromann would have something more to say?
Would you be happy with the {wrapperOptions: {retry: 2}}?

Choose a reason for hiding this comment

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

I agree with @lgandecki here. I don't see any harm in allowing non-standard options.

Copy link
Member

Choose a reason for hiding this comment

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

The problem with non-standard options is if somewhere down the line cucumber adds a retry option and now that is used for multiple things. By having wrapperOptions, the content is specifically for the setDefinitionFunctionWrapper function and can never clash with future cucumber-js options

Choose a reason for hiding this comment

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

@charlierudolph if you guys add a retry function and it would break stuff in wdio-cucumber-framework that would be ok. You guys should not care about compatibility with 3rd party integrations if you bump versions correctly.

Copy link
Member

Choose a reason for hiding this comment

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

I'm still pressing for using wrapperOptions so users are not confused as to what is a built in cucumber-js option and one that is being used by a step definitions wrapper (which the user or a framework can define).

Choose a reason for hiding this comment

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

If this is a blocker for you I would be fine with introducing wrapperOptions

Copy link
Member

Choose a reason for hiding this comment

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

I would consider this a blocker for me. I am good merging this in if we use wrapperOptions though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like we have a decision. Not the best for us, but hey, can't have everything ;)
tests seem to be passing, looking forward to the new release so we can move forward with the wdio framework :)
Thanks!

@charlierudolph
Copy link
Member

Cool. I added a couple small cleanup commits. I should be able to merge and release this today.

@charlierudolph charlierudolph merged commit ac33ce6 into cucumber:master Jun 10, 2017
@christian-bromann
Copy link

Awesome, thanks so much @charlierudolph

@lock
Copy link

lock bot commented Oct 25, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Oct 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants