Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Unified Diff: test_runner.js

Issue 29517687: Issue 5079, 5516 - Use webpack for browser tests, modules for content scripts (Closed)
Patch Set: Addressed Wladimir's and Hubert's initial feedback Created Aug. 17, 2017, 12:36 p.m.
Use n/p to move between diff chunks; N/P to move between comments.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « test/browser/elemHideEmulation.js ('k') | no next file » | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
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
Wladimir Palant 2017/08/18 20:49:56 Actually, people usually sort by module name, not
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" conv
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
kzar 2017/08/17 13:26:42 Well it just means that when things go wrong the e
{
- 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
+ }
+ }).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);
« no previous file with comments | « test/browser/elemHideEmulation.js ('k') | no next file » | no next file with comments »

Powered by Google App Engine
This is Rietveld