Issue 6391 - Allow running the browser unit tests with Firefox

test_runner.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 /* eslint-env node */
 
 "use strict";
 
 const fs = require("fs");
 const path = require("path");
 
 const MemoryFS = require("memory-fs");
 const nodeunit = require("nodeunit");
 const webpack = require("webpack");
 
-const chromiumProcess = require("./chromium_process");
+const chromiumProcess = require("./test/runners/chromium_process");
+const chromiumWdProcess = require("./test/runners/chromiumwd_process");
+const firefoxProcess = require("./test/runners/firefox_process");
 
 let unitFiles = [];
 let browserFiles = [];
 
+let runnerDefinitions = {
+  // Chromium with chrome-remote-interface
+  chromium_remote: chromiumProcess,

[kzar, 2018/05/04 10:52:44]

So do I have this right that by default the browser tests run under both Firefox
and Chromium via WebDriver. But if the BROWSER_TEST_RUNNERS environment variable
is specified that can be changed. The value of chromium_remote would mean that
the browser tests run with Chromium via chrome-remote-interface instead?

If so please could you add a comment here to explain the this logic? Also please
could you update the README to document how it works? Please could you make the
comment about WebDriver being available >= 63... clearer?

Also perhaps we should rename chromiumwd_process to chromium_process and
chromium_process to chromium_remote_interface_process? Then the names would be
more consistent between Firefox and Chromium.

Also I wonder if we could automatically decide between using WebDriver and
chrome-remote-interface depending on the version of Chromium present?

[hub, 2018/05/18 00:04:22]

Yes
Yes
Will do
Ok.
 
Currently we download a specific version, so that's the runner we want to use
that decide which version of the browsers to use. The same way a prior to this
patch.

+  // Chromium with WebDriver ( >= 63.0.3239)
+  chromium: chromiumWdProcess,
+  firefox: firefoxProcess
+};
+
+function configureRunners()

[kzar, 2018/05/04 10:52:44]

I wonder what the point of this function is, since we immediately call it and
don't call it anywhere else? Unless I'm missing something we could just remove
it and put the logic inline here?

[hub, 2018/05/18 00:04:23]

I think it is nice to have this in its own function.

+{
+  let runners = "BROWSER_TEST_RUNNERS" in process.env ?
+      process.env.BROWSER_TEST_RUNNERS.split(",") : [];
+
+  if (runners.length == 0)
+    return ["chromium", "firefox"];
+
+  runners.filter(runner => runnerDefinitions.hasOwnProperty(runner));

[kzar, 2018/05/04 10:52:43]

Shouldn't this instead be `return runners.filter...`? IIRC
Array.prototype.filter doesn't mutate the Array but rather returns a new one.

[hub, 2018/05/18 00:04:22]

good catch.

fixed.

+
+  return runners;
+}
+
+let runnerProcesses = configureRunners();
+
 function addTestPaths(testPaths, recurse)
 {
   for (let testPath of testPaths)
   {
     let stat = fs.statSync(testPath);
     if (stat.isDirectory())
     {
       if (recurse)
       {
         addTestPaths(fs.readdirSync(testPath).map(
@@ skipping 42 matching lines @@
       else
       {
         let bundle = memoryFS.readFileSync("/" + bundleFilename, "utf-8");
         memoryFS.unlinkSync("/" + bundleFilename);
         resolve(bundle);
       }
     });
   });
 }
 
-function runBrowserTests()
+function runBrowserTests(processes)
 {
   if (!browserFiles.length)
     return;
 
   let nodeunitPath = path.join(__dirname, "node_modules", "nodeunit",
                                "examples", "browser", "nodeunit.js");
   let bundleFilename = "bundle.js";
 
   return webpackInMemory(bundleFilename, {
     entry: path.join(__dirname, "test", "browser", "_bootstrap.js"),
     module: {
       rules: [{
         resource: nodeunitPath,
         // I would have rather used exports-loader here, to avoid treating
         // nodeunit as a global. Unfortunately the nodeunit browser example
         // script is quite slopily put together, if exports isn't falsey it
         // breaks! As a workaround we need to use script-loader, which means
         // that exports is falsey for that script as a side-effect.
         use: ["script-loader"]
       }]
     },
     resolve: {
       alias: {
         nodeunit$: nodeunitPath
       },
       modules: [path.resolve(__dirname, "lib")]
     }
   }).then(bundle =>
-  {
-    return chromiumProcess(
-      bundle, bundleFilename,
-      browserFiles.map(
-        file => path.relative(path.join(__dirname, "test", "browser"),
-                              file).replace(/\.js$/, "")
+    Promise.all(
+      processes.map(currentProcess =>
+        runnerDefinitions[currentProcess](
+          bundle, bundleFilename,
+          browserFiles.map(
+            file => path.relative(path.join(__dirname, "test", "browser"),
+                                  file).replace(/\.js$/, "")
+          )
+        )
       )
-    );
-  });
+    )
+  );
 }
 
 if (process.argv.length > 2)
   addTestPaths(process.argv.slice(2), true);
 else
 {
   addTestPaths(
     [path.join(__dirname, "test"), path.join(__dirname, "test", "browser")],
     true
   );
 }
 
-Promise.resolve(runBrowserTests()).catch(error =>
+Promise.resolve(runBrowserTests(runnerProcesses)).catch(error =>
 {
   console.error("Failed running browser tests");
   console.error(error);
 }).then(() =>
 {
   if (unitFiles.length)
     nodeunit.reporters.default.run(unitFiles);
 });