Adding runtime to the worldConstructor

[devpaul]

Tested using jasmine and works running cucumber against SauceLabs using wd

[devpaul]

See usage of SauceLabs + wd + cucumber-js here: https://github.com/RageMaion/node-bdd-example

PR should resolve issue #97

[devpaul]

Here's a fully working example using SauceLabs + wd + cucumber-js: https://github.com/devpaul/node-bdd-example

[jbpros]

Thank you for this PR. We definitely need to be able to attach listeners from inside support code and probably hooks as well. However, I'm not convinced we should expose the runtime instance to the end users; this is internal stuff. What about exposing another method on the support code helper (i.e. the object holding defineStep(), Given(), World, etc.)?

this.registerListener(myListener);
this.Before(...

It would be more consistent with the rest of the API, imo. WDYT?

[devpaul]

Thanks for your feedback. I understand your argument for appropriately restricting the API. If I understand your suggestion, support code with this.registerListener(myListener) would have a listener registered. As a user would I add this to a step definition?

It may be a bit odd if users commingle their registerListener functionality with step definitions. cucumber-js may want to encourage the use of a common.js, listeners.js, or similar pattern. And If I understand the World constructor correctly (that the last this.World assignment wins) it may be useful to put the World constructor there as well.

I'd also like to add a list of event name strings to Listeners.Event (e.g. Listeners.Event.BeforeScenerio = 'BeforeScenerio') to assist in code completion and prevent typo frustration.

Let me know what you think. I'm ready to move forward w/ the discussed code changes.

[devpaul]

Removed the old runtime passing method.

Now supports adding listeners and handlers in step definitions via this.registerListener(listener); this.registerHandler(eventName, handler); and adding by event name (e.g. this.BeforeFeatures(handler))

Updated example usage https://github.com/devpaul/node-bdd-example

[jbpros]

Thank you @devpaul for the update. This is very nice work, I like it!

The exhaustive list of event names is a very neat idea. The listener registration method is good and the coding style is very well aligned with the rest of the project, thank you for taking care of that ;)

Your idea of exposing methods to register specific event handlers is a good idea. I didn't think of that before, probably because the other Cucumber implementations don't expose such methods. However, in the JavaScript world, this kind of API is frequent and makes sense.

@aslakhellesoy @mattwynne do you have any opinions on this? As it's not standard Cucumber stuff, I'd be glad to know what you think of it. Basically, this PR introduces (in addition to registerListener()) several methods like BeforeFeatures, AfterFeatures, BeforeScenario, AfterStep, etc. Those methods register event handlers that, in turn, will be passed the event object. There's also a more generic registerHandler (which should be renamed to registerEventHandler). My concerns are:

1. It's exposing internal stuff: event objects were not meant to be public. You might argue that events are effectively used within the public API when passed to (custom) listeners but those are self-contained instances that encapsulate some third-party logic and people building them should explicitly know what they are doing with the events. Having things like a BeforeStep method for example available in the step definitions/support code make the event objects much more ubiquitous to the regular users and could simply confuse them.
2. As long as we expose registerListener, a method that allows one to tell cucumber to pass all events to a function, do we really need finer-grained register methods for specific events? The advantage of registerListener is that it makes it clearer the user should understand things like a Cucumber listener and the events it receives.
3. Following those thoughts, registerHandler is not needed neither.
4. Future internal changes to Cucumber.js (think Gherkin3) will impact a wider part of its public API with all those methods.
5. BeforeX and AfterX methods names are too close to the existing Before and After hook methods, imo. It'd be ok if they were similar beasts, the former ones will pass events to the handlers while the latter are executed in the context of the World instance. Close names, distant behaviour.

@devpaul Do you have real use cases for these methods? Before merging this, it'd be great for us all to see concrete usages of these additional API methods, especially since it can be achieved through the registerListener method.

[devpaul]

@jbpros I'm glad you like the changes to the PR. I'm looking forward to getting this functionality in a release. When I started working with cucumber-js one of the things I noticed immediately is how difficult it was to write plug-ins for the framework. Adding registerListener should help to make things better by providing a way to hook into the eventing system. The named event methods BeforeX and AfterX provide a more declarative approach to adding behavior and suits the cucumber syntax.

Here is how I'm using BeforeFeatures to create a world definition: https://github.com/devpaul/node-bdd-example/blob/master/test/features/step_definitions/worldDefinition.js -- it reads very well. Better than if I had split construction between a listener and the worldDefinition. I could imagine custom formatters being another use case for this API.

I understand your concerns. In regards to your first point, I would be careful about trying to be overprotective about access. Cucumber has already let the cat out of the bag by returning a runtime with an attachListener method. Formatters already use the listeners to output data. If there is a specific use case for protectionism let's work with that instead.

Taking your concerns into account, what if we moved the named BeforeX and AfterX declarative methods to Cucumber.Listener and removed registerHandler from the helper. This would leave the only registerListener on the helper, move APIs you feel are more sensitive to the more internal Listener object and still keep a declarative syntax in one place (now on Listener rather than on the helper).

In regard to hook vs event behavior, what if we added the world and scenario to the event payload? I haven't looked at the implications for doing this, but it may allow you to make the Hooker into a special-case wrapper of a Listener and reduce complexity.

Let me know what you think. I don't want to make a change that hasn't been fully fleshed out, but my time may run short after this week so I'd like to keep the momentum going on this PR :).

[Bochenski]

@devpaul This is fantastic - Really hope this gets accepted soon, as it's enabled me to hook cucumber into protractor and SauceLabs all from a grunt task (critically the AfterFeatures event allows me to gracefully close my connection to SauceLabs). I've got another working example here based on your example and this other from @DSKang
@jbpros - Any news on whether/when this might be accepted?

[revington]

@devpaul I can not get it working. My code looks like this:

//features/support/world.js

var World = function World(callback){
    // some set up here
    var world = this;
   wdBrowser.init(conf, function(){
       world.BeforeFeatures(function(event, callback){
          console.log('### YES! ####');
       });
      callback(world);
   });

}

exports.World = World;

BeforeFeatures is not defined.

[devpaul]

@revington Take a look at the example usage I posted above. BeforeFeatures is not present on the World, it's part of the step definitions the same way Before and After hooks are used.

see: https://github.com/devpaul/node-bdd-example/blob/master/test/features/step_definitions/worldDefinition.js

[revington]

@devpaul Thank you very much!

So now I've got a file in my step definitions: ./features/step_definitions/after-scenario-hook.js

function afterEachHook() {                                                           
    this.AfterScenario(function (event, callback) {                                  
        console.log('AfterScenario: %s', event.getPayloadItem('scenario').getName());
        callback();                                                                  
    });                                                                              
}                                                                                    
module.exports = afterEachHook;                                                      

And it works perfectly. Do you know hot can I read scenario results? So I can check if it failed or was successful.
I am exploring the payload and the Cucumber.Runtime.StepResult with no success.

[devpaul]

@revington I would take a look at the formatters. They use the same functional hooks being exposed in this PR. https://github.com/cucumber/cucumber-js/blob/master/lib/cucumber/listener/json_formatter.js#L116

[revington]

@devpaul I finally got it working! Thank you very much:

function afterEachHook() {                                       
    var context = this,                                          
        featureName,                                             
        result = {                                               
            passed: true                                                                         
        };                                                                           
    this.BeforeFeature(function (event, callback) {              
        var feature = event.getPayloadItem('feature');           
        featureName = feature.getName();                         
        callback();                                              
    });                                                          
    this.StepResult(function (event, callback) {                 
        var stepResult = event.getPayloadItem('stepResult');     
        result.passed = result.passed & !stepResult.isFailed();  
        callback();                                              
    });                                                          
    this.BeforeScenario(function (event, callback) {             
        var scenario = event.getPayloadItem('scenario');         
        result.name = featureName + ':' + scenario.getName();    
        callback();                                              
    });                                                          
    this.AfterScenario(function (event, callback) {   
        // context.sessionId is the browser's sessionId   
        notifySauce(context.sessionId, result, function () {     
            return callback();                                   
        });                                                      
    });                                                          
}                                                                

[devpaul]

I've moved the BeforeX and AfterX methods to the listener on another branch: https://github.com/devpaul/cucumber-js/tree/cucumbersauce. For the most part, I like the change as it helps to package plugins as listeners and improves reusability. I updated cucumber-wd-plugin (https://github.com/devpaul/cucumber-wd-plugin) to use the more listener-centric approach for writing plugins.

[jbpros]

Thanks again for the hard work Paul. This is very good! I'm merging this now.

I'd like to see end-to-end tests added at some point. What I suggest is we consider these two new methods unstable API at the moment. I'm ok if the build passes without this tested deeply as well as the rest. We'll also need to document them in the README at some point.

Taking your concerns into account, what if we moved the named BeforeX and AfterX declarative methods to Cucumber.Listener and removed registerHandler from the helper. This would leave the only registerListener on the helper, move APIs you feel are more sensitive to the more internal Listener object and still keep a declarative syntax in one place (now on Listener rather than on the helper).

I'll merge with registerHandler() in but we can keep on discussing alternative solutions. registerListener() feels about right to me, though.

In regard to hook vs event behavior, what if we added the world and scenario to the event payload? I haven't looked at the implications for doing this, but it may allow you to make the Hooker into a special-case wrapper of a Listener and reduce complexity.

That's interesting. Would you like/have time to make a spike on that?

[jbpros]

Closed by 4be20a5

[lock]

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.