Issue 6717 - Part 2: run qunit in headless firefox

test/firefox.js

 /*

[Sebastian Noack, 2018/08/22 18:16:38]

Why not calling it test_runner.js and put it under the top level, and later put
stuff for the browsers in here as well, just like you did in adblockpluscore?

Also, I wonder whether there is any code we could potentially share between
test_runner.js in adblockpluscore and this script (maybe with some abstraction
maybe)? Note that I didn't compare the code in both test runners yet, so it
might as well not be worth it. I just want to put it up for discussion.

[tlucas, 2018/08/22 20:24:01]

From what i saw, there's virtually no code from adblockpluscore/test_runner.js
that could be reused, except

    nodeunit.reporters.default.run(files);

and the generation of the "files"-array from the command line arguments.

That's now all together done by "nodeunit test/*" from a script in package.json.
I wouldn't mind adding an abstraction layer for future browsers thought, if
anyone insists.

[Sebastian Noack, 2018/08/22 20:54:35]

Fair enough. Still what is the advantage of the directory structure
test/<browser> over a single script in the top-level? It seems even when we add
support for Chrome, it's mostly the same code we want to run.

[tlucas, 2018/08/23 08:47:27]

With the current implementation of the packagers, a .js file on the top-level
would be added to builds. A not explicitly listed folder is skipped (and that's
what we want).

[Sebastian Noack, 2018/08/23 16:38:44]

Well changing buildtools to not include test_runner.js in the builds would be
trivial. The question rather is how will the test runner code look like once we
add more stuff (e.g. other browsers). Is there a point in splitting up things
into different test scripts? It seems adblockpluscore gets away with a combined
test runner for Chrome and Firefox.

[tlucas, 2018/08/24 10:25:08]

For the buildtools, same argument as somewhere else in this mail, i'd rather not
initiate a buildtools change / dependency update.

For the future, we'll also get away with an abstraction (and maybe even without
a test_runner), i.e. outsourcing every "controlling" functionality from
firefox.js to something like _common.js and leaving the setups to firefox.js /
chrome.js / edge.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");
-const webdriver = require("selenium-webdriver");
-const By = webdriver.By;

[Sebastian Noack, 2018/08/22 18:16:38]

Nit: Use desctrucuring.

[tlucas, 2018/08/22 20:24:02]

I assume you expected something like (with the comment below in mind)

const webdriver = require("selenium-webdriver");
const {By:By, until:until} = webdriver;

Or am i mislead by google?

[Sebastian Noack, 2018/08/22 20:54:35]

Better:

  const {By, until} = webdriver;

[tlucas, 2018/08/23 08:47:28]

Done.

-const until = webdriver.until;

[Sebastian Noack, 2018/08/22 18:16:38]

Nit: This seems to be unused.

[tlucas, 2018/08/22 20:24:01]

It's used in line 86;

...
       driver.wait(until. ...

[Sebastian Noack, 2018/08/22 20:54:35]

My bad.

 
-const FIREFOX_VERSION = "57.0";
-
-const Command = require("selenium-webdriver/lib/command").Command;
-const path = require("path");
-const firefox = require("selenium-webdriver/firefox");
+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))
+  )));
+}
 
 exports.runFirefox = function(test)
 {
-  test.expect(14);

[Sebastian Noack, 2018/08/22 18:16:38]

Where is this number coming from?

[tlucas, 2018/08/22 20:24:01]

It's the number of test-groups for qunit, when run inside Firefox (i.e. the
number of assertions you get from this script in total, from the lines 108/120),
e.g.:


firefox.js
✔ runFirefox

OK: 14 assertions (17553ms)

[Sebastian Noack, 2018/08/22 20:54:35]

So that means whenever the number of qunit tests change we have to change the
hard-coded number here as well? That sounds quite unpractical. Can we figure out
how many tests there are and the call expect() asynchronously? What if we not
call expect() at all? Any other ideas?

[tlucas, 2018/08/23 08:47:27]

The only drawback of not using expect() is, we would (in this script) have no
idea if all of the test have been run - but i guess qunit is taking care of
that. Removing it won't have any other side effect - so let's just remove it. 

   // https://stackoverflow.com/a/45045036
   function installWebExt(driver, extension)
   {
     let cmd = new Command("moz-install-web-ext")
       .setParameter("path", path.resolve(extension))
       .setParameter("temporary", true);
 
     driver.getExecutor()
       .defineCommand(cmd.getName(), "POST",
                      "/session/:sessionId/moz/addon/install");
 
-    return driver.schedule(cmd, "installWebExt(" + extension + ")");
+    return driver.schedule(cmd, `installWebExt(${extension})`);
   }
-
-  // Spawn the Firefox process in headless mode

[Sebastian Noack, 2018/08/22 18:16:38]

Nit: This comment seems redundant to me.

[tlucas, 2018/08/22 20:24:01]

Done.

 
   ensureFirefox(FIREFOX_VERSION).then(firefoxPath =>
   {
     let binary = new firefox.Binary(firefoxPath);
 
     binary.addArguments("-headless");
 
     let options = new firefox.Options()
-          .setBinary(binary);
+      .setBinary(binary);
 
     let driver = new webdriver.Builder()
-            .forBrowser("firefox")
-            .setFirefoxOptions(options)
-            .build();
+      .forBrowser("firefox")
+      .setFirefoxOptions(options)
+      .build();
 
     installWebExt(driver, "./devenv.gecko");
 
     driver.wait(() =>
-    {
       // Wait for the firstrun-page to be loaded
-      return driver.getAllWindowHandles().then(handles =>
+      driver.getAllWindowHandles().then(handles =>
       {
         if (handles.length > 1)
         {
           driver.switchTo().window(handles[1]);
           return true;
         }
         return false;
-      });
-    }).then(() =>
-    {
+      })
+    ).then(() =>
       // Navigate to the qunit index
-      driver.executeScript("location.href = \"qunit/index.html\";");

[Sebastian Noack, 2018/08/22 18:16:38]

Wouldn't driver.navigate().to() be more appropriate?

[tlucas, 2018/08/22 20:24:02]

driver.navigate().to() expects absolute URLs, so we'd have to figure that out
first - whereas the script above also accepts relative urls.

[Sebastian Noack, 2018/08/22 20:54:35]

Acknowledged.

-    }).then(() =>
-    {
+      driver.executeScript("location.href = \"qunit/index.html\";")
+    ).then(() =>
       // Wait for qunit-results to be present
-      driver.wait(until.elementLocated(By.id("qunit-testresult")));
-    }).then(() =>
-    {
+      driver.wait(until.elementLocated(By.id("qunit-testresult")))
+    ).then(() =>
       // Wait for tests to finish
       driver.wait(() =>
-      {
-        return driver.findElement(By.id("qunit-testresult"))

[Sebastian Noack, 2018/08/22 18:16:38]

Nit: return + braces are redundant.

[tlucas, 2018/08/22 20:24:01]

Could you help me out here, what should it look like?

[Sebastian Noack, 2018/08/22 20:54:35]

Sorry, I thought for some reason Hubert was the author. The following are
equivalent:

  () => x
  () => { return x; }

[tlucas, 2018/08/23 08:47:27]

Done. (also for the braces / return around "Wait for the firstrun-page to be
loaded")

-        .getAttribute("innerHTML").then(data =>
-        {
-          return data.search("Tests completed") >= 0;
-        });
-      });
-    }).then(() =>
-    {
-      // Find passed tests
-      driver.findElements(By.css("#qunit-tests .pass .test-name"))
-        .then(elements =>
-        {
-          elements.forEach(elem =>

[Sebastian Noack, 2018/08/22 18:16:38]

Nit: We prefer for..of loops over forEach().

[tlucas, 2018/08/22 20:24:01]

Done.

-          {
-            elem.getAttribute("innerHTML").then(data =>
-            {
-              test.ok(true, data);
-            });
-          });
-        });
-      // Find failed tests
-      driver.findElements(By.css("#qunit-tests .fail .test-name"))
-        .then(elements =>
-        {
-          elements.forEach(elem =>

[Sebastian Noack, 2018/08/22 18:16:38]

Nit: See above.

[tlucas, 2018/08/22 20:24:01]

Done.

-          {
-            elem.getAttribute("innerHTML").then(data =>
-            {
-              test.ok(false, "Undefined error in " + data);
-            });
-          });
-        });
-    }).then(() =>
+        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();
-    });
+    }, err =>
+      driver.quit().then(() =>
+      {
+        throw err;
+      })
+    );
   });
 };