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

Step definitions do not support es6 generators as steps #9

Closed
InvictusMB opened this Issue Dec 2, 2016 · 4 comments

Comments

2 participants
@InvictusMB
Contributor

InvictusMB commented Dec 2, 2016

Cucumber also accepts es6 generator functions as steps and wraps them with co
In order to be fully compatible with cucumber all three options should be supported.

Current support:

  • promises
  • callbacks (assuming #8 is closed)
  • generators

InvictusMB pushed a commit to InvictusMB/serenity-js that referenced this issue Dec 2, 2016

Fialkovskyi, Ievgen (ITCDEC) - KLM Fialkovskyi, Ievgen (ITCDEC) - KLM
fix(cucumber): Support callback style step definitions (fixes #8)
Match syncronized steps with the contract cucumber expects. es6 generators accepted by cucumber are still not supported (#9)
@jan-molak

This comment has been minimized.

Show comment
Hide comment
@jan-molak

jan-molak Dec 6, 2016

Owner

#8 merged.

I agree that the webdriver synchroniser should not break any existing step definitions people might have in their test suites. Therefore, if a step definition uses generators, it should keep working even with the synchroniser activated.

I'd be more than happy to review and merge PRs providing this functionality.

Jan

Owner

jan-molak commented Dec 6, 2016

#8 merged.

I agree that the webdriver synchroniser should not break any existing step definitions people might have in their test suites. Therefore, if a step definition uses generators, it should keep working even with the synchroniser activated.

I'd be more than happy to review and merge PRs providing this functionality.

Jan

@InvictusMB

This comment has been minimized.

Show comment
Hide comment
@InvictusMB

InvictusMB Dec 7, 2016

Contributor

@jan-molak
Is it ok to add a dependency on co to wrap generator steps?

One way then would be simply returning thenable wrapper.
I'm not totally sure about this solution as it is not fully transparent and has a side effect of transforming an interface of a step.

Other way to be 100% transparent would be more complex.
Something like:

    function synchronisedStep (originalStep: SomeFunction) {

        return mimicInterface(originalStep, function (...args: any[]) {

            let context = this;

            return executeStepInControlFlow(originalStep, context, args);
        });
    }

    function executeStep(step, context, args) {
        if (isGeneratorFn(step)) {
            return co.wrap(step).apply(context, args);
        }
        return step.apply(context, args);
    }

    function executeStepInControlFlow(step, context, args) {
        let deferred = new Deferred<void>();

        controlFlow
            .execute(executeStep(step, context, args))
            .then(deferred.resolve, deferred.reject);

        return deferred.promise;
    }

    function mimicInterface (original, pretender) {
        if (isGeneratorFn(original)) {
            return function* (...args: any[]) {

                let context  = this;

                return pretender.apply(context, args);
            };
        }

        return function (...args: any[]) {

            let context  = this,
                result = pretender.apply(context, args);

            if (!hasCallbackInterface(original, args)) {
                return result;
            }
        };
    }

Also in second case TS complains about using a generator with compilation target ES5.

Contributor

InvictusMB commented Dec 7, 2016

@jan-molak
Is it ok to add a dependency on co to wrap generator steps?

One way then would be simply returning thenable wrapper.
I'm not totally sure about this solution as it is not fully transparent and has a side effect of transforming an interface of a step.

Other way to be 100% transparent would be more complex.
Something like:

    function synchronisedStep (originalStep: SomeFunction) {

        return mimicInterface(originalStep, function (...args: any[]) {

            let context = this;

            return executeStepInControlFlow(originalStep, context, args);
        });
    }

    function executeStep(step, context, args) {
        if (isGeneratorFn(step)) {
            return co.wrap(step).apply(context, args);
        }
        return step.apply(context, args);
    }

    function executeStepInControlFlow(step, context, args) {
        let deferred = new Deferred<void>();

        controlFlow
            .execute(executeStep(step, context, args))
            .then(deferred.resolve, deferred.reject);

        return deferred.promise;
    }

    function mimicInterface (original, pretender) {
        if (isGeneratorFn(original)) {
            return function* (...args: any[]) {

                let context  = this;

                return pretender.apply(context, args);
            };
        }

        return function (...args: any[]) {

            let context  = this,
                result = pretender.apply(context, args);

            if (!hasCallbackInterface(original, args)) {
                return result;
            }
        };
    }

Also in second case TS complains about using a generator with compilation target ES5.

@jan-molak

This comment has been minimized.

Show comment
Hide comment
@jan-molak

jan-molak Dec 7, 2016

Owner

One way then would be simply returning thenable wrapper.
I'm not totally sure about this solution as it is not fully transparent and has a side effect of transforming an interface of a step.

I agree that a side effect here is not ideal, but I wonder how much of a problem it would be? As in: could this break anything? Make debugging harder? What are the practical downsides of doing it this way? (It's not a rhetorical question, I'm honestly not sure :) )

co doesn't have any dependencies, so it seems pretty harmless? Would there be any downside to using it? Also, it looks like co has a capability of detecting generator functions, which means that perhaps we could us it instead of is-generator we currently have?

Owner

jan-molak commented Dec 7, 2016

One way then would be simply returning thenable wrapper.
I'm not totally sure about this solution as it is not fully transparent and has a side effect of transforming an interface of a step.

I agree that a side effect here is not ideal, but I wonder how much of a problem it would be? As in: could this break anything? Make debugging harder? What are the practical downsides of doing it this way? (It's not a rhetorical question, I'm honestly not sure :) )

co doesn't have any dependencies, so it seems pretty harmless? Would there be any downside to using it? Also, it looks like co has a capability of detecting generator functions, which means that perhaps we could us it instead of is-generator we currently have?

InvictusMB added a commit to InvictusMB/serenity-js that referenced this issue Jan 16, 2017

@InvictusMB

This comment has been minimized.

Show comment
Hide comment
@InvictusMB

InvictusMB Jan 16, 2017

Contributor

@jan-molak
Unfortunately co doesn't expose neither isGeneratorFunction nor toPromisehelpers. So there is still a need for is-generator.
I would go for an easy solution with co for now. Especially considering that cucumber@2 provides an API for wrapping steps and it is quite different so there is no need to monkey patch and one can do with steps whatever he wants.

Another alternative to co is using bluebird promise library that provides Promise.coroutine helper to consume generators. But that would be an overkill if bluebird promises do not provide any additional value to the project compared to native ones.

Contributor

InvictusMB commented Jan 16, 2017

@jan-molak
Unfortunately co doesn't expose neither isGeneratorFunction nor toPromisehelpers. So there is still a need for is-generator.
I would go for an easy solution with co for now. Especially considering that cucumber@2 provides an API for wrapping steps and it is quite different so there is no need to monkey patch and one can do with steps whatever he wants.

Another alternative to co is using bluebird promise library that provides Promise.coroutine helper to consume generators. But that would be an overkill if bluebird promises do not provide any additional value to the project compared to native ones.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment