Issue 6887 - add Chrome to "npm test"

test/run.js

+/*

[Sebastian Noack, 2018/08/27 19:56:16]

FWIW, I think all.js is a slightly better name for this module.

[tlucas, 2018/08/28 08:18:14]

Done.

+ * 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 mocha */
+
+"use strict";
+
+const glob = require("glob");
+const path = require("path");
+const {exec} = require("child_process");
+
+let testFiles = glob.sync("./test/cases/*.js");

[Sebastian Noack, 2018/08/27 19:56:16]

Nit: IMO, the code reads better if we inline this call below. Yes, then it's
evaluated twice, but I doubt that causes any measurable overhead.

[tlucas, 2018/08/28 08:18:14]

Done.

+
+for (let browser of glob.sync("./test/browser/*.js"))
+{
+  let module = require(path.resolve(browser));

[Sebastian Noack, 2018/08/27 19:56:16]

Is path.resolve() necessary here?

[tlucas, 2018/08/28 08:18:14]

Well, "./test/browsers/firefox.js" can't be implicitly resolved from there.
What's the alternative?

[Sebastian Noack, 2018/08/28 14:44:57]

Right, require() expects a path, relative to the current script.

+  let buildCommand = `python build.py devenv -t ${module.platform}`;

[Sebastian Noack, 2018/08/27 19:56:16]

Nit: Since this variable is only used once, inline it below?

[tlucas, 2018/08/28 08:18:14]

Done.

+
+
+  describe(module.platform, function()
+  {
+    this.timeout(0);
+
+    let driver;
+    let origin;
+
+    let buildDevenv = new Promise((resolve, reject) =>

[Sebastian Noack, 2018/08/27 19:56:16]

This should happen within before().

[tlucas, 2018/08/28 08:18:14]

Moved it.
To clarify, fmi - doesn't the line below:

"    before(() => buildDevenv.then(() =>"

Cause this to only be executed when before() is called? Or is the Promise
started on construction?

[Sebastian Noack, 2018/08/28 14:44:58]

Nope. This is not a function (that is called). You were essentially calling
exec() right away.

+    {
+      exec(buildCommand, (error, stdout, stderr) =>
+      {
+        if (error) reject(stderr);

[Sebastian Noack, 2018/08/27 19:56:15]

In case of an error, we might rather want to forward the output to stderr of the
current process and then reject the promise with the provided error.

(This would equivalent to not catching STDERR in the first place when using the
subprocess module in Python.)

Also that way we make sure, that the error is printed an readable format, and a
proper error object is propagated.

[tlucas, 2018/08/28 08:18:14]

Done.

+        else resolve(stdout);
+      });
+    });
+
+    before(() => buildDevenv.then(() =>
+      module.getDriver().then(d =>
+      {
+        driver = d;
+        return driver.wait(() =>
+          driver.getAllWindowHandles().then(handles => handles[1])
+        );
+      }).then(handle =>
+        driver.switchTo().window(handle)
+      ).then(() =>
+        driver.executeScript("return location.origin;")
+      ).then(o =>
+      {
+        origin = o;
+      })
+    ));
+
+    for (let file of testFiles)
+    {
+      let testModule = require(path.resolve(file));
+
+      it(testModule.title, () =>

[Sebastian Noack, 2018/08/27 19:56:15]

I wonder, would it work if we call it() on the top-level inside the module so
that we just need to call require() here and don't need this wrapper?

[tlucas, 2018/08/28 08:18:14]

In theory yes - How would you pass "driver" and "origin"?

[Sebastian Noack, 2018/08/28 14:44:58]

From looking at other examples using Mocha, it seems instead of using a variable
in the shared closure, you can leverage a shared context (i.e. "this"):

  before(function()
  {
    return somePromise.then(result =>
    {
      this.result = result;
    });
  });

  it(function()
  {
    somePromise.then(() =>
    {
      doSomethingWith(this.result);
    }
  });

Note that it's important to use the function statement for the callback passed
to before() and it(), while using arrow functions for any nested callback. That
is because arrow functions ignore any context provided, but bind "this" to the
context of the parent scope.

[tlucas, 2018/08/29 09:16:53]

Thanks for clarifying. But i don't see, how we'd currently benefit from that.

[Sebastian Noack, 2018/08/29 15:22:12]

Well, it would be less code here, less boilerplate in test/wrappers/*, the
scripts could contain more than one test case (which may, or may not, be desired
in the future), and the code will be easier to understand if you know Mocha.

[tlucas, 2018/09/01 09:38:30]

This actually works pretty nicely. After finding out about require.cache at
least, done.

+      {
+        return testModule.it(driver, origin);
+      });
+    }
+
+    after(() => driver.quit());
+  });
+}