Issue 6717 - Part 2: run qunit in headless firefox

test/firefox.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/>.
  */
 
 "use strict";
 
 const FIREFOX_VERSION = "57.0";
 
 const path = require("path");
 const webdriver = require("selenium-webdriver");
 const {By, until} = webdriver;
 const firefox = require("selenium-webdriver/firefox");
 const {Command} = require("selenium-webdriver/lib/command");
 const {ensureFirefox} = require("../adblockpluscore/test/runners/" +
                                 "firefox_download");
 

[Sebastian Noack, 2018/08/24 13:27:59]

Nit: We usually only put one blank line after the imports.

[tlucas, 2018/08/24 15:26:00]

Done.

-
 function reportElements(test, driver, success)
 {
   return driver.findElements(
     By.css(`#qunit-tests ${success ? ".pass" : ".fail"} .test-name`)
   ).then(elements => Promise.all(elements.map(elem =>
-        elem.getAttribute("innerHTML").then(data => test.ok(success, data))
+    elem.getAttribute("innerHTML").then(data => test.ok(success, data))
   )));
 }
 
 exports.runFirefox = function(test)
 {
   // https://stackoverflow.com/a/45045036
   function installWebExt(driver, extension)
   {
     let cmd = new Command("moz-install-web-ext")
       .setParameter("path", path.resolve(extension))
@@ skipping 45 matching lines @@
         driver.findElement(By.id("qunit-testresult"))
           .getAttribute("innerHTML").then(data =>
              data.includes("Tests completed")))
     ).then(() => Promise.all([
       reportElements(test, driver, true),
       reportElements(test, driver, false)
     ])).then(() =>
     {
       driver.quit();
       test.done();
-    }).catch((err) =>

[Sebastian Noack, 2018/08/24 13:27:59]

I'd suggest to change the error handling like that:

  .then(
    () =>
    {
      driver.quit();
      test.done();
    },
    err =>
    {
      driver.quit();
      throw err;
    }
  )

1. There is no reason to call driver.quit() again in case driver.quit()
   or test.done() (after calling driver.quit()) failed for some reason.
2. We don't need to wait for driver.quit() to complete before emitting
   the error.
3. Nit: The parenthesis are redundant (and we usually omit them) in
   arrow functions with exactly one argument.

Even better would be to call the Promise's finally() method, but that would
require Node.js >=10, and it doesn't seem to be worthwile to bump up the minimum
version just for that.

[tlucas, 2018/08/24 15:26:00]

Done. (Still waiting for driver.quit() to finish before throwing the error, as
discussed)

-    {
+    }, err =>
       driver.quit().then(() =>
       {
         throw err;
-      });
-    });
+      })
+    );
   });
 };