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

test_runner.js

Index: test_runner.js
diff --git a/test_runner.js b/test_runner.js
index 094c7b93296276b1d24cc9d5e73ecfecd3b248d9..4099c4c1b2c74bddf82fa87f2252443af171812c 100644
--- a/test_runner.js
+++ b/test_runner.js
@@ -21,9 +21,10 @@
 
 const fs = require("fs");
 const path = require("path");
-const url = require("url");
 
 const nodeunit = require("nodeunit");
+const webpack = require("webpack");
+const MemoryFS = require("memory-fs");

[Wladimir Palant, 2017/08/17 13:06:10]

Nit: sort the imports alphabetically?

[kzar, 2017/08/17 13:26:42]

Well I did but case sensitively, I considered A to fall after z. But I don't
care too much, if you'd rather I can move it too.

[Wladimir Palant, 2017/08/18 20:49:56]

Actually, people usually sort by module name, not by the names of imported
symbols ;)

[kzar, 2017/08/21 09:07:01]

Done.

 
 const chromiumProcess = require("./chromium_process");
 
@@ -49,19 +50,49 @@ function addTestPaths(testPaths, recurse)
     if (path.extname(testPath) == ".js")
     {
       if (testPath.split(path.sep).includes("browser"))
-        browserFiles.push(testPath);
+      {
+        browserFiles.push(
+          path.relative(path.join(__dirname, "test", "browser"),
+                        testPath).replace(/\.js$/, "")
+        );

[Wladimir Palant, 2017/08/17 13:06:10]

I'd prefer to have this "path to module name" conversion logic out of this
function. It's very specific to WebPack so it rather belongs in
runBrowserTests().

[kzar, 2017/08/17 13:26:42]

Done.

+      }
       else
         unitFiles.push(testPath);
     }
   }
 }
 
-function getFileURL(filePath)
+function webpackInMemory(bundleFilename, options)

[Wladimir Palant, 2017/08/17 13:06:10]

Do we need the bundleFilename parameter? It seems irrelevant here, we can just
use "/output" instead and will save us some relative-to-absolute conversions and
output.path option.

[kzar, 2017/08/17 13:26:42]

Well it just means that when things go wrong the error messages are a bit more
friendly, since we refer to bundle.js consistently. I know what you mean though,
it's kind of annoying passing that name around.

 {
-  return url.format({
-    protocol: "file",
-    slashes: "true",
-    pathname: path.resolve(process.cwd(), filePath).split(path.sep).join("/")
+  return new Promise((resolve, reject) =>
+  {
+    // Based on this example https://webpack.js.org/api/node/#custom-file-systems

[Wladimir Palant, 2017/08/17 13:06:10]

Nit: overlong line, move URL to next line?

[kzar, 2017/08/17 13:26:42]

Done.

+    let memoryFS = new MemoryFS();
+
+    options.output = {filename: bundleFilename, path: "/"};
+    let webpackCompiler = webpack(options);
+    webpackCompiler.outputFileSystem = memoryFS;
+
+    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.unlinkSync("/" + bundleFilename);
+        resolve(bundle);
+      }
+    });
   });
 }
 
@@ -70,20 +101,33 @@ function runBrowserTests()
   if (!browserFiles.length)
     return;
 
-  // Navigate to this directory because about:blank won't be allowed to load
-  // file:/// URLs.
-  let initialPage = getFileURL(__dirname);
-  let bootstrapPath = path.join(__dirname, "test", "browser",
-                                "_bootstrap.js");
-  let nodeunitPath = path.join(
-    path.dirname(require.resolve("nodeunit")),
-    "examples", "browser", "nodeunit.js"
-  );
-  let args = [
-    getFileURL(nodeunitPath),
-    ...browserFiles.map(getFileURL)
-  ];
-  return chromiumProcess(initialPage, bootstrapPath, args);
+  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 rathered used exports-loader here, to avoid treating

[Wladimir Palant, 2017/08/17 13:06:10]

"rathered" => "rather"?

[kzar, 2017/08/17 13:26:42]

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")]

[Wladimir Palant, 2017/08/17 13:06:10]

We don't need this any more, do we?

[kzar, 2017/08/17 13:26:43]

I think we do, otherwise we'll look for common.js in node_modules instead of
lib/ for example.

+    }
+  }).then(bundle =>
+  {
+    return chromiumProcess(bundle, bundleFilename, browserFiles);
+  });
 }
 
 if (process.argv.length > 2)
@@ -96,7 +140,7 @@ else
   );
 }
 
-Promise.resolve(runBrowserTests()).catch(error =>
+runBrowserTests().catch(error =>
 {
   console.error("Failed running browser tests");
   console.error(error);