Bug with webdriver's inheritance

[ghost]

The way that the webdriver class/module/whatever inherits from EventEmitter is wrong:

webdriver.prototype = new EventEmitter();

This way each instance of webdriver you create is sharing the same EventEmitter variables. This is not a problem if you have one webdriver, but if you have more, even not at the same time, it causes problems because when you call webdriver.on( 'status'|'command' ), the listeners of all instances (even ones that are not active) will be called whenever there is event. If one is trying to run several drivers in parallel it becomes a mess. In other words if you have two webdrivers each event in each webdriver will trigger two event callbacks.

The correct way to inherit from EventEmitter:

var util = require( 'util' );
function webdriver ( args ) {
  EventEmitter.call( this );
  //the rest of the webdriver constructor
}
util.inherits( webdriver, EventEmitter );
//now each webdriver instance has its own private event listeners

[admc]

This looks totally right to me -- if you were up for making the changes and submitting a pull request, I would be happy merge it in! Thanks.

[ghost]

No, it is wrong. It would be right if you were to write

webdriver.prototype = EventEmitter.prototype;

Then you would still have to call the EventEmitter constructor in the webdriver constructor. new EventEmitter(); creates an instances, you can't inherit instance of object. Well you can obviously, but is wrong and doesn't work as it should except in the case where you use one webdriver instance in the lifetime of the app.

I fixed it myself and as you can see above the fix is three lines long and I tested it. It would take you less time to copy paste it than for me to fork your repository, fix, push, submit pull request, then you need to merge the changes, etc. Just copy the lines above. Or here is the whole begining of the file webdriver.js

var EventEmitter = require('events').EventEmitter;
var async = require("async");
var _ = require("underscore");
var fs = require("fs");
var element = require('./element').element;
var request = require('request');

var __slice = Array.prototype.slice;
var utils = require("./utils");
var JSONWIRE_ERRORS = require('./jsonwire-errors.js');
var MAX_ERROR_LENGTH = 500;
var util = require( 'util' );

// webdriver client main class
// WdConfig is an option object containing the following fields:
// host,port, username, accessKey
var webdriver = module.exports = function(remoteWdConfig) {
  EventEmitter.call( this );
  this.sessionID = null;
  this.username = remoteWdConfig.username || process.env.SAUCE_USERNAME;
  this.accessKey = remoteWdConfig.accessKey  || process.env.SAUCE_ACCESS_KEY;
  this.basePath = (remoteWdConfig.path || '/wd/hub');
  this.https = (remoteWdConfig.https || false);
  // default
  this.options = {
    host: remoteWdConfig.host || '127.0.0.1'
    , port: remoteWdConfig.port || 4444
    , path: (this.basePath + '/session').replace('//', '/')
  };
  this.defaultCapabilities = {
    browserName: 'firefox'
     , version: ''
    , javascriptEnabled: true
    , platform: 'ANY'
  };
  // saucelabs default
  if ((this.username) && (this.accessKey)) {
    this.defaultCapabilities.platform = 'VISTA';
  }
};

//inherit from EventEmitter
util.inherits( webdriver, EventEmitter );

[admc]

I agree that this a better way to do this, however having applied your changes the wd tests are now breaking:

https://travis-ci.org/admc/wd/builds/5824508

I don't have time at the moment to dive in and debug this, any thoughts would be appreciated.

I'm running 'make test' locally with selenium 2.31: http://docs.seleniumhq.org/download/ and 'make test_saucelabs' the way travis is and seeing the same error.

Adam

[ghost]

I tried to spot something but couldn't find anything that doesn't look right, sorry. I'm still new to WebDriver. Maybe something will popup when I start using it soon.

[sebv]

Made the change, @admc, it looks like you had messed up the wd remote configuration (plus there was a missing callback in one of the new test).