Issue 4223 - Migrate some more of adblockplustests

test/stub-modules/common.js

+let path = require("path");

[Wladimir Palant, 2016/09/30 09:37:54]

I realized that this module doesn't belong into stub-modules - why should it
override lib/common.js if we ever add one? If you move it to test/common.js,
does nodeunit complain?

[kzar, 2016/10/03 13:46:16]

You're right but nodeunit considers all JS files in the test directory to
contain tests. For those it considers every export a test, I guess except for
special names like "setUp". When I tried to move this file to test/common.js
nodeunit attempted to run createSandbox as a test and thought something had gone
wrong when test.done() (extraModules.done()) didn't get called.

I can't see a way to exclude a specific file. Possible solutions:
 - We reimplement the logic which decides which files contain tests and pass
that array of file names to nodeunit ourselves. That would require adding a
test_runner.js or similar script, which calls / replaces the
nodeunit/bin/nodeunit script.
 - We hack common.js to simply call extraModules.done()!
 - We just leave the file in stub-modules/common.js.

Or perhaps there's a cleaner solution I didn't think of?

[kzar, 2016/10/03 16:43:14]

Well I've had a go at this, I moved common.js and then wrote a basic test
runner. It has the advantage of allowing us to run specific test files too,
stuff like `npm test test/cssRules.js` should work.

+let SandboxedModule = require("sandboxed-module");
+
+let globals = {
+  atob: data => new Buffer(data, "base64").toString("binary"),
+  btoa: data => new Buffer(data, "binary").toString("base64"),
+  Ci: {},
+  Cu: {import: () => { }},

[Wladimir Palant, 2016/09/30 09:37:54]

I don't think that eslint will accept this. According to our coding style the
bracket needs to go onto the next line:

  Cu: {
    import: () =>
    {
    }
  },

Similarly for the other empty functions below.

[kzar, 2016/10/03 13:46:17]

Done.

+  navigator: {},
+  onShutdown: {add: () => { }},
+  Services: {obs: {addObserver: () => { }}},
+  XPCOMUtils: {generateQI: () => { }}
+};
+
+function addExports(exports)
+{
+  // Unfortunately there's not a way for a source transformer to know the name
+  // of the module being transformed. This means we must add all extra exports

[Wladimir Palant, 2016/09/30 09:37:54]

Well, source transformers are being bound to the SandboxedModule instance and
can use this.filename as I indicated before. But this won't work of course if
you return an arrow function ;). Change this in to a regular function and you
won't need to inject exports into all modules.

[kzar, 2016/10/03 13:46:15]

Oh cool, Done.

+  // to all modules, not ideal!
+  let code = "if (!('EXPORT' in exports))\n" +
+             "  Object.defineProperty(exports, 'EXPORT', {get: () => EXPORT});";

[Wladimir Palant, 2016/09/30 09:37:55]

Template literals allow multi-line input and are easier to read anyway:

  for (let name of exports)
  {
    src += `
      if (!("${name}" in exports))
        Object.defineProperty(exports, "${name}", {get: () => ${name}});
    `;
  }

[kzar, 2016/10/03 13:46:16]

Done.

+
+  return src => src + "\n" + exports.
+    map(name => code.replace(/EXPORT/g, name)).join("\n");
+}
+
+function rewriteRequires(source)
+{
+  // Taken from EasyPasswords
+  // https://github.com/palant/easypasswords/blob/master/gulp-utils.js

[Wladimir Palant, 2016/09/30 09:37:55]

I'm donating this code to the Adblock Plus project, it's ok not to mention the
source :)

Side-note: when mentioning the source you better link to the specific revision
rather than master.

[kzar, 2016/10/03 13:46:15]

Acknowledged.

+  let modules = new Set(["events", "io"]);

[Wladimir Palant, 2016/09/30 09:37:55]

Why only these two modules? I'd suggest that you get rid of the NODE_PATH hack
altogether - our modules should really require via relative paths, and currently
they merely don't do it because of the limitations of our module loader (that we
should fix eventually). Something along these lines:

  let modules = new Map();
  function readModules(dir)
  {
    for (let file of fs.readdirSync(dir))
      if (path.extname(file) == ".js")
        modules[path.basename(file, ".js")] = path.resolve(dir, file);
  }

  readModules(path.resolve(__dirname, "..", "..", "lib"));
  readModules(__dirname);

  ...

  function escape_string(str)
  {
    return str.replace(/(["'\\])/g, "\\$1");
  }

  return prefix + escape_string(modules.get(request));

So anything like require("io") will be rewritten into
require("/home/foo/adblockpluscore/test/stub-modules/io.js") - absolute path, no
messing with NODE_PATH required any more. Note that Easy Passwords currently
doesn't have that escape_string() code which means that the tests probably don't
work on Windows (backslashes in resolved path).

Note that unit tests themselves run outside the sandbox, so they should use
proper relative paths to require modules.

[kzar, 2016/10/03 13:46:17]

Still thinking about this one. (There's also the problem that we're now
transforming common.js and any modules it requires.)

[kzar, 2016/10/03 17:47:43]

Done.

+  return source.replace(/(\brequire\(["'])([^"']+)/g, (match, prefix, request) =>
+  {
+    if (modules.has(request))
+      return prefix + request + ".js";
+    return match;
+  });
+}
+
+function libPath(moduleId)
+{
+  return path.resolve(__dirname, "..", "..", "lib", moduleId + ".js");
+}
+
+exports.createSandbox = function(extraExports)
+{
+  // Shared require cache for this sandbox
+  let cache = {};
+
+  let options = {
+    cache: cache,
+    globals: globals,
+    sourceTransformers: [rewriteRequires]
+  };
+  if (extraExports)
+    options.sourceTransformers.push(addExports(extraExports));
+
+  return (moduleId, exports) =>

[Wladimir Palant, 2016/09/30 09:37:55]

exports parameter is unused.

[kzar, 2016/10/03 13:46:16]

Done.

+  {
+    let key = libPath(moduleId);
+
+    // Only load modules which aren't already cached for this sandbox
+    if (!(key in cache))
+      cache[key] = SandboxedModule.require(moduleId, options);
+
+    return cache[key];

[Wladimir Palant, 2016/09/30 09:37:54]

Ok, if you need to cache modules manually then this is just wrong. How about a
different approach, one that doesn't require passing a shared cache via options:

  let options = {...};
  return SandboxedModule.require("./common", options).require;

  ...

  exports.require = require;

So common.js will load itself into the sandbox and export the require() function
from inside the sandbox. Calling this require() function should load additional
modules into the same environment then. And if you move common.js to
test/common.js then relative paths should work correctly without any hacks.

[kzar, 2016/10/03 13:46:16]

Ah really cool idea. We don't even need that pull request to land now. Done.

+  };
+};