Fix conditional driver loading (#670)

* Revert "Revert "Adjustments to `start` Options (#660)""

This reverts commit f52ad8b.

* Update test suite to detect when Chrome didn't load

The commit with a problem caused no drivers to load but the tests
did not detect this as they were looking to see what args were
specified not what was the result of those args with regard to what
drivers loaded.

This commit updates the test suite to better detect the problem. It
captures the stdout and checks it for Chrome instead of just checking
the args.

With this in place I now have a failing test with my problematic code
from before.

* Make selenium use only the drivers specified

This removes the previous attempt to do this, `--detect-drivers`. That
option would be better named as "--load-drivers" as it won't load any
drivers regardless if specified the path via the CLI.

According to this discussion:

SeleniumHQ/selenium#8928

The `-I` driver is what is needed to specify what drivers should be
loaded. Therefore for each driver we are providing an explicit path we
are also indicating that driver should be loaded. Any without a -I
will not get loaded.

I updated the failing test so it does explicitly load the firefox driver
(to avoid the jar throwing an error about no drivers being loaded) and
then have the test verify that Chrome is not in the output indicating
that other drivers such as Chrome did not load.

The possible values for `-I` doesn't seem well documented (that I could
find). The code seems to compare the CLI value to the "display name"
for each driver info class:

https://github.com/SeleniumHQ/selenium/blob/9546dba7a5aefe45ff8fe2bbc1039819dde10672/java/src/org/openqa/selenium/grid/node/config/NodeOptions.java#L460

Therefore to get a complete list of the right values I just looked at
all the implementations of the "getDisplayName" method.

I have personally manually tested chrome, firefox and chromium edge.
I don't have a win32 system to test IE and the legacy version of
Edge but based on the display name it *should* work just fine.

Author: eric1234

lib/start.js

@@ -27,6 +27,10 @@ async function start(_opts) {
     opts.seleniumArgs = [];
   }
 
+  if (!opts.spawnOptions) {
+    opts.spawnOptions = {};
+  }
+
   if (!opts.version) {
     opts.version = defaultConfig.version;
   }
@@ -84,26 +88,36 @@ async function start(_opts) {
    */
   if (fsPaths.chrome) {
     args.push('-Dwebdriver.chrome.driver=' + fsPaths.chrome.installPath);
+    opts.seleniumArgs.push('-I');
+    opts.seleniumArgs.push('chrome');
   }
 
   if (process.platform === 'win32' && fsPaths.ie) {
     args.push('-Dwebdriver.ie.driver=' + fsPaths.ie.installPath);
+    opts.seleniumArgs.push('-I');
+    opts.seleniumArgs.push('internet explorer');
   } else {
     delete fsPaths.ie;
   }
 
   if (process.platform === 'win32' && fsPaths.edge) {
     args.push('-Dwebdriver.edge.driver=' + fsPaths.edge.installPath);
+    opts.seleniumArgs.push('-I');
+    opts.seleniumArgs.push('edge');
   } else {
     delete fsPaths.edge;
   }
 
   if (fsPaths.firefox) {
     args.push('-Dwebdriver.gecko.driver=' + fsPaths.firefox.installPath);
+    opts.seleniumArgs.push('-I');
+    opts.seleniumArgs.push('firefox');
   }
 
   if (fsPaths.chromiumedge) {
     args.push('-Dwebdriver.edge.driver=' + fsPaths.chromiumedge.installPath);
+    opts.seleniumArgs.push('-I');
+    opts.seleniumArgs.push('edge');
   } else {
     delete fsPaths.chromiumedge;
   }
@@ -117,6 +131,10 @@ async function start(_opts) {
     throw new Error(`Port ${seleniumStatusUrl.port} is already in use.`);
   }
 
+  if (!opts.spawnOptions.stdio) {
+    opts.spawnOptions.stdio = 'ignore';
+  }
+
   debug('Spawning Selenium Server process', opts.javaPath, args);
   const selenium = spawn(opts.javaPath, args, opts.spawnOptions);
   await checkStarted(selenium, seleniumStatusUrl.toString());

test/programmatic.js

@@ -21,16 +21,27 @@ describe('programmatic use', function () {
         }
       });
   };
-  const testStart = function (done, options, callback) {
+  const testStart = function (done, rawOptions, callback) {
     const selenium = require('../');
+    const stdio = [
+      'ignore', // stdin
+      'pipe', // stdout
+      'ignore', // stderr
+    ];
+    const options = Object.assign({}, rawOptions);
+    options.spawnOptions = Object.assign({ stdio }, options.spawnOptions);
     selenium
       .start(options)
       .catch(done)
       .then((cp) => {
         cp.kill();
-        if (callback(cp) !== false) {
-          done();
-        }
+        let stdout = '';
+        cp.stdout.on('data', (chunk) => (stdout += chunk));
+        cp.stdout.on('end', () => {
+          if (callback(stdout) !== false) {
+            done();
+          }
+        });
       });
   };
 
@@ -53,26 +64,26 @@ describe('programmatic use', function () {
   });
 
   it('should start', (done) => {
-    testStart(done, {}, (cp) => {
-      if (cp.spawnargs && !cp.spawnargs.some(containsChrome)) {
+    testStart(done, {}, (log) => {
+      if (!containsChrome(log)) {
         done(new Error('Chrome driver should be loaded'));
         return false;
       }
     });
   });
 
   it('should start with custom seleniumArgs', (done) => {
-    testStart(done, { seleniumArgs: ['--port', '12345'] }, (cp) => {
-      if (cp.spawnargs && !cp.spawnargs.some(containsChrome)) {
+    testStart(done, { seleniumArgs: ['--port', '12345'] }, (log) => {
+      if (!containsChrome(log)) {
         done(new Error('Chrome driver should be loaded'));
         return false;
       }
     });
   });
 
   it('should start with the given drivers', (done) => {
-    testStart(done, { drivers: {} }, (cp) => {
-      if (cp.spawnargs && cp.spawnargs.some(containsChrome)) {
+    testStart(done, { drivers: { firefox: {} } }, (log) => {
+      if (containsChrome(log)) {
         done(new Error('Chrome driver should not be loaded'));
         return false;
       }
@@ -81,8 +92,8 @@ describe('programmatic use', function () {
 
   it('should start and merge drivers', (done) => {
     const options = { seleniumArgs: ['--port', '4445'], drivers: { chrome: {} } };
-    testStart(done, options, (cp) => {
-      if (cp.spawnargs && !cp.spawnargs.some(containsChrome)) {
+    testStart(done, options, (log) => {
+      if (!containsChrome(log)) {
         done(new Error('Chrome driver should be loaded'));
         return false;
       }
@@ -93,8 +104,8 @@ describe('programmatic use', function () {
     testStart(
       done,
       { singleDriverStart: 'firefox', drivers: { chrome: {}, firefox: {} }, seleniumArgs: ['--port', '4446'] },
-      (cp) => {
-        if (cp.spawnargs && cp.spawnargs.some(containsChrome)) {
+      (log) => {
+        if (containsChrome(log)) {
           done(new Error('Chrome driver should not be loaded'));
           return false;
         }