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

test_runner.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 */
 
 "use strict";
 
 const fs = require("fs");
 const path = require("path");
 
 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");
 
 let unitFiles = [];
 let browserFiles = [];
 
 function addTestPaths(testPaths, recurse)
 {
   for (let testPath of testPaths)
   {
     let stat = fs.statSync(testPath);
     if (stat.isDirectory())
     {
       if (recurse)
       {
         addTestPaths(fs.readdirSync(testPath).map(
           file => path.join(testPath, file)));
       }
       continue;
     }
     if (path.basename(testPath).startsWith("_"))
       continue;
     if (path.extname(testPath) == ".js")
     {
       if (testPath.split(path.sep).includes("browser"))
-      {
-        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.

-      }
+        browserFiles.push(testPath);
       else
         unitFiles.push(testPath);
     }
   }
 }
 
 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 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.

+    // 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;
 
     webpackCompiler.run((err, stats) =>
     {
       // Error handling is based on this example
       // https://webpack.js.org/api/node/#error-handling
@@ skipping 23 matching lines @@
 
   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.

+        // I would have rather used exports-loader here, to avoid treating
         // 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);
+    return chromiumProcess(
+      bundle, bundleFilename,
+      browserFiles.map(
+        file => path.relative(path.join(__dirname, "test", "browser"),
+                              file).replace(/\.js$/, "")
+      )
+    );
   });
 }
 
 if (process.argv.length > 2)
   addTestPaths(process.argv.slice(2), true);
 else
 {
   addTestPaths(
     [path.join(__dirname, "test"), path.join(__dirname, "test", "browser")],
     true
   );
 }
 
-runBrowserTests().catch(error =>
+Promise.resolve(runBrowserTests()).catch(error =>
 {
   console.error("Failed running browser tests");
   console.error(error);
 }).then(() =>
 {
   if (unitFiles.length)
     nodeunit.reporters.default.run(unitFiles);
 });