Issue 5079, 5516 - Use webpack for browser tests, modules for content scripts

chromium_process.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2017 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 */
 /* eslint no-console: "off" */
 
 "use strict";
 
 const childProcess = require("child_process");
 const fs = require("fs");
 const https = require("https");
 const os = require("os");
 const path = require("path");
 
+const extractZip = require("extract-zip");
 const remoteInterface = require("chrome-remote-interface");
-const extractZip = require("extract-zip");
+const webpack = require("webpack");
+const MemoryFS = require("memory-fs");

[Wladimir Palant, 2017/08/17 10:05:38]

I assume that this module exists because WebPack is using it? Still, if we are
using it we should add it as our dependency explicitly.

Also a nit: I'm using the Python style for importing modules here, for better
overview. So please sort the imports of third-party modules alphabetically.

[kzar, 2017/08/17 12:40:21]

Done.

 
 const CHROMIUM_REVISION = 467222;
 
 function rmdir(dirPath)
 {
   for (let file of fs.readdirSync(dirPath))
   {
     let filePath = path.join(dirPath, file);
     try
     {
@@ skipping 200 matching lines @@
     return new Promise((resolve, reject) =>
     {
       setTimeout(() =>
       {
         connectRemoteInterface(attempt + 1).then(resolve).catch(reject);
       }, 200);
     });
   });
 }
 
-function runBootstrap(initialPage, bootstrapPath, bootstrapArgs)
+function webpackInMemory(bundleFilename, options)
 {
+  // Based on this example https://webpack.js.org/api/node/#custom-file-systems
+  let memoryFS = new MemoryFS();
+
+  options.output = {filename: bundleFilename, path: "/"};
+  let webpackCompiler = webpack(options);
+  webpackCompiler.outputFileSystem = memoryFS;

[Wladimir Palant, 2017/08/17 10:05:37]

Nit: It's better to put all the statements above inside the promise as well.
This way, we can rely on the function not throwing but only rejecting the
promise on error.

[kzar, 2017/08/17 12:40:21]

Done.

+  return new Promise((resolve, reject) =>
+  {
+    webpackCompiler.run((err, stats) =>
+    {
+      // Error handling is based on this example
+      // https://webpack.js.org/api/node/#error-handling
+      if (err)
+      {
+        let reason = err.stack || err;
+        if (err.details)
+          reason += "\n" + err.details;
+        reject(reason);
+      }
+      else if (stats.hasErrors())
+        reject(stats.toJson().errors);
+      else
+      {
+        let bundle = memoryFS.readFileSync("/" + bundleFilename, "utf-8");
+        memoryFS.unlink("/" + bundleFilename, () => { resolve(bundle); });

[Wladimir Palant, 2017/08/17 10:05:38]

Nit: Use memoryFS.unlinkSync() here to make this more straightforward? Besides,
this callback doesn't look like it would be accepted by ESLint.

[kzar, 2017/08/17 12:40:21]

Done.

+      }
+    });
+  });
+}
+
+function runBrowserTests(browserTestModules)
+{
+  // We need to navigate to this directory because about:blank won't be allowed
+  // to load file:/// URLs.
+  let initialPage = require("url").format({
+    protocol: "file",
+    slashes: "true",
+    pathname: path.resolve(process.cwd(), __dirname).split(path.sep).join("/")
+  });

[Wladimir Palant, 2017/08/17 10:07:30]

Actually, this shouldn't be necessary any more. Running the tests on about:blank
should be fine, we are no longer loading any file:/// URLs.

[kzar, 2017/08/17 12:40:21]

Done.

+
+  let bootstrapPath = path.join(__dirname, "test", "browser", "_bootstrap.js");
+  let nodeunitPath = path.join(__dirname, "node_modules", "nodeunit",
+                               "examples", "browser", "nodeunit.js");
+
   return connectRemoteInterface().then(async client =>
   {
     try
     {
       let {Runtime, Log, Console, Page} = client;
 
       await Log.enable();
       Log.entryAdded(({entry}) =>
       {
         reportMessage(entry.text, entry.level);
       });
 
       await Console.enable();
       Console.messageAdded(({message}) =>
       {
         reportMessage(message.text, message.level);
       });
 
       await Page.navigate({url: initialPage});
 
       await Runtime.enable();
+
+      let bundleFilename = "bundle.js";
+      let bundle = await webpackInMemory(bundleFilename, {
+        entry: bootstrapPath,
+        module: {
+          rules: [{
+            resource: nodeunitPath,
+            // I would have rathered use exports-loader here, to avoid treating

[Wladimir Palant, 2017/08/17 10:05:37]

"rather used"?

[kzar, 2017/08/17 12:40:21]

Done.

+            // 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")]
+        }
+      });
+
       let compileResult = await Runtime.compileScript({
-        expression: fs.readFileSync(bootstrapPath, "utf-8"),
-        sourceURL: bootstrapPath,
+        expression: bundle,
+        sourceURL: bundleFilename,
         persistScript: true
       });
       if (compileResult.exceptionDetails)
         throwException(compileResult.exceptionDetails, bootstrapPath);
 
       let runResult = await Runtime.runScript({
         scriptId: compileResult.scriptId
       });
       if (runResult.exceptionDetails)
         throwException(runResult.exceptionDetails, bootstrapPath);
 
       let callResult = await Runtime.callFunctionOn({
         objectId: runResult.result.objectId,
-        functionDeclaration: "function(...args) {return this(...args);}",
-        arguments: bootstrapArgs.map(url => ({value: url}))
+        functionDeclaration: "function(...modules) {return this(modules);}",
+        arguments: browserTestModules.map(module => ({value: module}))
       });
       if (callResult.exceptionDetails)
         throwException(callResult.exceptionDetails, bootstrapPath);
 
       let promiseResult = await Runtime.awaitPromise({
         promiseObjectId: callResult.result.objectId
       });
       if (promiseResult.exceptionDetails)
         throwException(promiseResult.exceptionDetails, bootstrapPath);
     }
     finally
     {
       client.close();
     }
   });
 }
 
-module.exports = function(initialPage, bootstrapPath, bootstrapArgs)
+module.exports = function(browserTestFiles)
 {
   return ensureChromium().then(chromiumPath =>
   {
     let child = startChromium(chromiumPath);
     return Promise.race([
       child.done,
-      runBootstrap(initialPage, bootstrapPath, bootstrapArgs)
+      runBrowserTests(
+        browserTestFiles.map(
+          m => "." + path.sep + path.relative(path.join("test", "browser"), m)

[Wladimir Palant, 2017/08/17 10:05:37]

The point here is generating a list of module names suitable for require(). This
means that path.sep is wrong - it will be "\" on Windows, yet path separator in
module names is always "/".

Also, you are building paths relative to test/browser - this is assuming that
the current directory is pointing to repository root. This should be
path.join(__dirname, "test", "browser").

Besides, it seems that you should actually return 
path.relative(path.join(__dirname, "test", "browser"), m).replace(/\.js$/, "")
here. This way you can use require("./" + module + ".js") in _bootstrap.js
without having to create an explicit context.

[kzar, 2017/08/17 12:40:21]

Done.

+        )
+      )
     ]).then(result =>
     {
       child.kill();
       return result;
     }).catch(error =>
     {
       child.kill();
       throw error;
     });
   });
 };