Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Supporting step definitions assertions in callbacks #38

Closed
kareemk opened this Issue · 6 comments

3 participants

@kareemk

Given step definitions currently rely on failures in step definitions to be synchronous:

support_code/step_definitions.js:19

      try {
        code.apply(world, parameters);
      } catch (exception) {
        if (exception)
          Cucumber.Debug.warn(exception.stack || exception, 'exception inside feature', 3);
        var stepResult;
        if (exception instanceof Cucumber.Runtime.PendingStepException)
          stepResult = Cucumber.Runtime.PendingStepResult()
        else
          stepResult = Cucumber.Runtime.FailedStepResult(exception);
        callback(stepResult);
      }

It's not possible to handle assertions that need to be made in callbacks, which is the case when using phantomjs as the bridge is asynchronous. So:

this.browser.evaluate(function(){ "$('#id').length"}, function(result) {
  result.should.not.eql(0)
});

Won't fail the scenario if the assertion fails.

Seems like the best approach is to support firing an error event in the case of doing assertions in callbacks (as well as directly throwing exceptions in step definitions. Thoughts? Happy to work on this once we flush out the ideal approach.

@jbpros
Owner

Thanks for reporting this one, Kareem. Async error handling is something that needs to be addressed.

I had several ideas in mind:

  • Add an error() method on the callback object, that would fire an error event, just like you suggested:

    Given(/^life is sweet$/, function(callback) {
      callback.error("Dude, I #fail");
    });
    
  • Regarding direct synchronous exceptions: I've been considering completely dropping support for them. I read several articles suggesting to avoid exceptions at all in JS.

    Practically, exceptions inside stepdefs would go uncaught and fail the whole process.

    I'm actually not keen to go down that path. Interrupting a whole feature suite because of a single step error would defeat Cucumber's goal.

@kareemk

Thinking through this I think that synchronous exceptions support should be dropped as it can still be supported through the error callback if required, that's a much more elegant approach resulting in a simpler API.

@kareemk

Furthermore, I find that the should error messages useless as I'm mainly comparing counts of matched elements resulting in cryptic exception traces like:

.AssertionError: expected 0 to not equal 0
    at Object.eql (/Users/KareemK/Source/crowdtap.member-home/node_modules/should/lib/should.js:291:10)
    at /Users/KareemK/Source/crowdtap.member-home/features/step_definitions/shared_steps.coffee:24:34
    at Object.<anonymous> (/Users/KareemK/Source/crowdtap.member-home/features/support/world.js:39:28)
    at Object.<anonymous> (/Users/KareemK/Source/crowdtap.member-home/node_modules/phantom/node_modules/dnode-protocol/index.js:274:16)
    at apply (/Users/KareemK/Source/crowdtap.member-home/node_modules/phantom/node_modules/dnode-protocol/index.js:143:17)
    at EventEmitter.handle (/Users/KareemK/Source/crowdtap.member-home/node_modules/phantom/node_modules/dnode-protocol/index.js:120:13)
    at /Users/KareemK/Source/crowdtap.member-home/node_modules/phantom/node_modules/dnode-protocol/index.js:81:20
    at EventEmitter.<anonymous> (/Users/KareemK/Source/crowdtap.member-home/node_modules/phantom/node_modules/dnode/node_modules/lazy/lazy.js:62:13)
    at EventEmitter.<anonymous> (/Users/KareemK/Source/crowdtap.member-home/node_modules/phantom/node_modules/dnode/node_modules/lazy/lazy.js:46:19)
    at EventEmitter.emit (events.js:67:17)
@paulbjensen

Hi Chaps,

I just ran into this issue when trying to use cucumber.js with zombie.js.

I like jbpros' suggestion of having a callback.error function. If no one minds I'd be happy to have a pop at implementing this in a fork and send a pull request when its ready.

@jbpros
Owner

@paulbjensen Thanks for proposing your help. Please fork then!

@jbpros
Owner

This was addressed months ago with the addition of callback.fail() in 2b921d0. Closing.

@jbpros jbpros closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.