Issue 4223 - Migrate some more of adblockplustests

Issue 4223 - Migrate some more of adblockplustests

[kzar, 2016-09-25 13:49:39]

Patch Set 1

https://codereview.adblockplus.org/29354864/diff/29354865/lib/cssRules.js
File lib/cssRules.js (right):

https://codereview.adblockplus.org/29354864/diff/29354865/lib/cssRules.js#new...
lib/cssRules.js:77: _getAllRules: function()
(Not to happy with this, or the similar changes to elemHide. Open to suggestions
if you can see a cleaner way.)

https://codereview.adblockplus.org/29354864/diff/29354865/test/filterListener.js
File test/filterListener.js (right):

https://codereview.adblockplus.org/29354864/diff/29354865/test/filterListener...
test/filterListener.js:199: checkKnownFilters(test, "add subscription with
filter1, @@filter2, #filter3, !filter4, #@#filter5,
example.com##[-abp-properties='filter6'],
example.com#@#[-abp-properties='filter7']", {blacklist: [filter1.text],
elemhide: [filter3.text], elemhideexception: [filter5.text, filter7.text],
cssrule: [filter6.text]});
This test and the two below it fail occasionally. I'm not sure why, but there
seems to be a race condition.

[Wladimir Palant, 2016-09-26 09:40:58]

https://codereview.adblockplus.org/29354864/diff/29354865/lib/cssRules.js
File lib/cssRules.js (right):

https://codereview.adblockplus.org/29354864/diff/29354865/lib/cssRules.js#new...
lib/cssRules.js:77: _getAllRules: function()
On 2016/09/25 13:49:39, kzar wrote:
> (Not to happy with this, or the similar changes to elemHide. Open to
suggestions
> if you can see a cleaner way.)

How about you use sandboxed-module ;). The following configuration should do to
get internal variable exported, for unit tests only:

  sourceTransformers: {
    exportInternalVars: function(source)
    {
      let needExport = {
        "cssRules": ["filters"],
        "elemHide": ["filterByKey", "exceptions"]
      };

      let basename = path.basename(this.filename, ".js");
      if (needExport.hasOwnProperty(basename))
        for (let varName of needExport[basename])
          source += `Object.defineProperty(exports, '${varName}', {get: () =>
${varName}});`;
      return source;
    }    
  }

https://codereview.adblockplus.org/29354864/diff/29354865/test/cssRules.js
File test/cssRules.js (right):

https://codereview.adblockplus.org/29354864/diff/29354865/test/cssRules.js#ne...
test/cssRules.js:29: common.prepareFilterComponents.call(this, true);
Saving and restoring data is a hack that was necessary because these tests used
to run against live extension. IMHO here we only need a helper that will create
a new virtual environment:

  let requireSandboxed = require("common").createSandbox();

With createSandbox being something like:

  function createSandbox()
  {
    // Shared require cache for this sandbox
    let requires = {};

    let SandboxedModule = require("sandboxed-module");
    return moduleId =>
    {
      return SandboxedModule.require(moduleId, {
        requires,
        sourceTransformers: ...,
        globals: ...
      });
    };
  }

You can then create a new sandbox for each test and load the modules - no need
for any cleanup. If you want to be extra fancy, you could generalize
createSandbox() somewhat by passing in internal variables to be exported rather
than hardcoding them in source transformers.

https://codereview.adblockplus.org/29354864/diff/29354865/test_wrapper.js
File test_wrapper.js (right):

https://codereview.adblockplus.org/29354864/diff/29354865/test_wrapper.js#new...
test_wrapper.js:30: // modules beforehand, otherwise they will might break when
given our modules!)
Messing with the global require function is a bad idea. As indicated before, it
should be a good idea for various reasons to run core code sandboxed. Fixing
requires within the sandbox isn't as easy as the documentation suggests, for
Easy Passwords I ended up using a source transformer rerouting all "known"
modules - see
https://github.com/palant/easypasswords/blob/1a419a05bddd2145f2ece1ca7838147a....

https://codereview.adblockplus.org/29354864/diff/29354865/test_wrapper.js#new...
test_wrapper.js:41: nodeBabel();
This should no longer be necessary if we require at least Node 6, our code can
run without transpiling now.

[kzar, 2016-09-29 15:53:11]

Patch Set 2 : Use sandboxed-module

https://codereview.adblockplus.org/29354864/diff/29354865/test/cssRules.js
File test/cssRules.js (right):

https://codereview.adblockplus.org/29354864/diff/29354865/test/cssRules.js#ne...
test/cssRules.js:29: common.prepareFilterComponents.call(this, true);
On 2016/09/26 09:40:58, Wladimir Palant wrote:
> Saving and restoring data is a hack that was necessary because these tests
used
> to run against live extension. IMHO here we only need a helper that will
create
> a new virtual environment:
> 
>   let requireSandboxed = require("common").createSandbox();
> 
> With createSandbox being something like:
> 
>   function createSandbox()
>   {
>     // Shared require cache for this sandbox
>     let requires = {};
> 
>     let SandboxedModule = require("sandboxed-module");
>     return moduleId =>
>     {
>       return SandboxedModule.require(moduleId, {
>         requires,
>         sourceTransformers: ...,
>         globals: ...
>       });
>     };
>   }
> 
> You can then create a new sandbox for each test and load the modules - no need
> for any cleanup. If you want to be extra fancy, you could generalize
> createSandbox() somewhat by passing in internal variables to be exported
rather
> than hardcoding them in source transformers.

Cool idea, it works great as you suggested except the requires Object. I found I
had to pass in a cache Object instead.

I also needed to make a change to sandboxed-module so that it was possible to
pass in a cache Object. I'm hoping he will accept my pull request... let's see.
https://github.com/felixge/node-sandboxed-module/pull/62

https://codereview.adblockplus.org/29354864/diff/29354865/test/filterListener.js
File test/filterListener.js (right):

https://codereview.adblockplus.org/29354864/diff/29354865/test/filterListener...
test/filterListener.js:199: checkKnownFilters(test, "add subscription with
filter1, @@filter2, #filter3, !filter4, #@#filter5,
example.com##[-abp-properties='filter6'],
example.com#@#[-abp-properties='filter7']", {blacklist: [filter1.text],
elemhide: [filter3.text], elemhideexception: [filter5.text, filter7.text],
cssrule: [filter6.text]});
On 2016/09/25 13:49:39, kzar wrote:
> This test and the two below it fail occasionally. I'm not sure why, but there
> seems to be a race condition.

Still not got to the bottom of this.

https://codereview.adblockplus.org/29354864/diff/29354865/test_wrapper.js
File test_wrapper.js (right):

https://codereview.adblockplus.org/29354864/diff/29354865/test_wrapper.js#new...
test_wrapper.js:30: // modules beforehand, otherwise they will might break when
given our modules!)
On 2016/09/26 09:40:58, Wladimir Palant wrote:
> Messing with the global require function is a bad idea. As indicated before,
it
> should be a good idea for various reasons to run core code sandboxed. Fixing
> requires within the sandbox isn't as easy as the documentation suggests, for
> Easy Passwords I ended up using a source transformer rerouting all "known"
> modules - see
>
https://github.com/palant/easypasswords/blob/1a419a05bddd2145f2ece1ca7838147a....

Done.

https://codereview.adblockplus.org/29354864/diff/29354865/test_wrapper.js#new...
test_wrapper.js:41: nodeBabel();
On 2016/09/26 09:40:58, Wladimir Palant wrote:
> This should no longer be necessary if we require at least Node 6, our code can
> run without transpiling now.

Done.

https://codereview.adblockplus.org/29354864/diff/29355282/test/elemHide.js
File test/elemHide.js (right):

https://codereview.adblockplus.org/29354864/diff/29355282/test/elemHide.js#ne...
test/elemHide.js:40: function testResult(domain, expectedSelectors, criteria)
(These changes are due to rebasing.)

[kzar, 2016-09-29 15:58:57]

Patch Set 3 : Get filterListener tests passing reliably

https://codereview.adblockplus.org/29354864/diff/29354865/test/filterListener.js
File test/filterListener.js (right):

https://codereview.adblockplus.org/29354864/diff/29354865/test/filterListener...
test/filterListener.js:199: checkKnownFilters(test, "add subscription with
filter1, @@filter2, #filter3, !filter4, #@#filter5,
example.com##[-abp-properties='filter6'],
example.com#@#[-abp-properties='filter7']", {blacklist: [filter1.text],
elemhide: [filter3.text], elemhideexception: [filter5.text, filter7.text],
cssrule: [filter6.text]});
On 2016/09/29 15:53:09, kzar wrote:
> On 2016/09/25 13:49:39, kzar wrote:
> > This test and the two below it fail occasionally. I'm not sure why, but
there
> > seems to be a race condition.
> 
> Still not got to the bottom of this.

Ah, was simply that the order varied! Fixed.

[kzar, 2016-09-29 18:19:38]

Patch Set 4 : Added filterNotifier tests, improved sourceTransformer logic

https://codereview.adblockplus.org/29354864/diff/29355314/test/stub-modules/c...
File test/stub-modules/common.js (right):

https://codereview.adblockplus.org/29354864/diff/29355314/test/stub-modules/c...
test/stub-modules/common.js:29: // Taken from EasyPasswords
I modified this logic quite a bit since I took it from EasyPasswords. Should I
keep this comment or not?

[kzar, 2016-09-29 19:24:57]

Patch Set 5 : Migrate filterStorage tests

[Wladimir Palant, 2016-09-30 09:37:56]

https://codereview.adblockplus.org/29354864/diff/29355316/package.json
File package.json (right):

https://codereview.adblockplus.org/29354864/diff/29355316/package.json#newcode11
package.json:11: "test": "NODE_PATH=lib:test/stub-modules node -e
'require(\".bin/nodeunit\");'"
Why not just:

  NODE_PATH=lib:test/stub-modules node_modules/.bin/nodeunit

https://codereview.adblockplus.org/29354864/diff/29355316/test/cssRules.js
File test/cssRules.js (right):

https://codereview.adblockplus.org/29354864/diff/29355316/test/cssRules.js#ne...
test/cssRules.js:21: let sandboxedRequire = null;
This doesn't need to be a global variable.

https://codereview.adblockplus.org/29354864/diff/29355316/test/cssRules.js#ne...
test/cssRules.js:34: ElemHide = sandboxedRequire("elemHide").ElemHide;
How about:

  (
    {Filter, CSSPropertyFilter} = sandboxedRequire("filterClasses"),
    {CSSRules} = sandboxedRequire("cssRules"),
    {ElemHide} = sandboxedRequire("elemHide")
  );

https://codereview.adblockplus.org/29354864/diff/29355316/test/domainRestrict...
File test/domainRestrictions.js (right):

https://codereview.adblockplus.org/29354864/diff/29355316/test/domainRestrict...
test/domainRestrictions.js:23: let {Filter} = sandboxedRequire("filterClasses");
Please put it in setUp function here as well - one sandbox per test, even if no
data modification is expected. In fact, filter.fromText() caches filters, you
wouldn't want that to mess with the tests. Same for all the other files with a
global sandbox, I'm not commenting there.

https://codereview.adblockplus.org/29354864/diff/29355316/test/filterClasses.js
File test/filterClasses.js (right):

https://codereview.adblockplus.org/29354864/diff/29355316/test/filterClasses....
test/filterClasses.js:251: let defaultTypes = 0x7FFFFFFF & ~(t.ELEMHIDE |
t.DOCUMENT | t.POPUP | t.GENERICHIDE | t.GENERICBLOCK);
This needs to go into a function, called from the unit tests explicitly - just
to make sure it uses data from the current sandbox. Something like:

  let [t, defaultTypes] = getRegExpFilterTypes();

https://codereview.adblockplus.org/29354864/diff/29355316/test/filterListener.js
File test/filterListener.js (right):

https://codereview.adblockplus.org/29354864/diff/29355316/test/filterListener...
test/filterListener.js:40: defaultMatcher =
sandboxedRequire("matcher").defaultMatcher;
As in the other test, it's probably cleaner to have all requires in a single
statement so that you can use destructuring assignment consistently.

https://codereview.adblockplus.org/29354864/diff/29355316/test/filterListener...
test/filterListener.js:100: result[type] = result[type].sort();
sort() modifies an array in place, the assignments here are pointless. It should
just be:

    expected[type].sort();
  result[type].sort();

https://codereview.adblockplus.org/29354864/diff/29355316/test/stub-modules/c...
File test/stub-modules/common.js (right):

https://codereview.adblockplus.org/29354864/diff/29355316/test/stub-modules/c...
test/stub-modules/common.js:1: let path = require("path");
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?

https://codereview.adblockplus.org/29354864/diff/29355316/test/stub-modules/c...
test/stub-modules/common.js:8: Cu: {import: () => { }},
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.

https://codereview.adblockplus.org/29354864/diff/29355316/test/stub-modules/c...
test/stub-modules/common.js:18: // of the module being transformed. This means
we must add all extra exports
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.

https://codereview.adblockplus.org/29354864/diff/29355316/test/stub-modules/c...
test/stub-modules/common.js:21: "  Object.defineProperty(exports, 'EXPORT',
{get: () => EXPORT});";
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}});
    `;
  }

https://codereview.adblockplus.org/29354864/diff/29355316/test/stub-modules/c...
test/stub-modules/common.js:30: //
https://github.com/palant/easypasswords/blob/master/gulp-utils.js
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.

https://codereview.adblockplus.org/29354864/diff/29355316/test/stub-modules/c...
test/stub-modules/common.js:31: let modules = new Set(["events", "io"]);
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.

https://codereview.adblockplus.org/29354864/diff/29355316/test/stub-modules/c...
test/stub-modules/common.js:58: return (moduleId, exports) =>
exports parameter is unused.

https://codereview.adblockplus.org/29354864/diff/29355316/test/stub-modules/c...
test/stub-modules/common.js:66: return cache[key];
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:17]

Patch Set 6 : Addressed feedback

https://codereview.adblockplus.org/29354864/diff/29355316/package.json
File package.json (right):

https://codereview.adblockplus.org/29354864/diff/29355316/package.json#newcode11
package.json:11: "test": "NODE_PATH=lib:test/stub-modules node -e
'require(\".bin/nodeunit\");'"
On 2016/09/30 09:37:53, Wladimir Palant wrote:
> Why not just:
> 
>   NODE_PATH=lib:test/stub-modules node_modules/.bin/nodeunit

Done.

https://codereview.adblockplus.org/29354864/diff/29355316/test/cssRules.js
File test/cssRules.js (right):

https://codereview.adblockplus.org/29354864/diff/29355316/test/cssRules.js#ne...
test/cssRules.js:21: let sandboxedRequire = null;
On 2016/09/30 09:37:53, Wladimir Palant wrote:
> This doesn't need to be a global variable.

Done.

https://codereview.adblockplus.org/29354864/diff/29355316/test/cssRules.js#ne...
test/cssRules.js:34: ElemHide = sandboxedRequire("elemHide").ElemHide;
On 2016/09/30 09:37:53, Wladimir Palant wrote:
> How about:
> 
>   (
>     {Filter, CSSPropertyFilter} = sandboxedRequire("filterClasses"),
>     {CSSRules} = sandboxedRequire("cssRules"),
>     {ElemHide} = sandboxedRequire("elemHide")
>   );

Done.

https://codereview.adblockplus.org/29354864/diff/29355316/test/domainRestrict...
File test/domainRestrictions.js (right):

https://codereview.adblockplus.org/29354864/diff/29355316/test/domainRestrict...
test/domainRestrictions.js:23: let {Filter} = sandboxedRequire("filterClasses");
On 2016/09/30 09:37:53, Wladimir Palant wrote:
> Please put it in setUp function here as well - one sandbox per test, even if
no
> data modification is expected. In fact, filter.fromText() caches filters, you
> wouldn't want that to mess with the tests. Same for all the other files with a
> global sandbox, I'm not commenting there.

Done.

https://codereview.adblockplus.org/29354864/diff/29355316/test/filterClasses.js
File test/filterClasses.js (right):

https://codereview.adblockplus.org/29354864/diff/29355316/test/filterClasses....
test/filterClasses.js:251: let defaultTypes = 0x7FFFFFFF & ~(t.ELEMHIDE |
t.DOCUMENT | t.POPUP | t.GENERICHIDE | t.GENERICBLOCK);
On 2016/09/30 09:37:54, Wladimir Palant wrote:
> This needs to go into a function, called from the unit tests explicitly - just
> to make sure it uses data from the current sandbox. Something like:
> 
>   let [t, defaultTypes] = getRegExpFilterTypes();

Done, well kinda. I simply moved this into setUp.

https://codereview.adblockplus.org/29354864/diff/29355316/test/filterListener.js
File test/filterListener.js (right):

https://codereview.adblockplus.org/29354864/diff/29355316/test/filterListener...
test/filterListener.js:40: defaultMatcher =
sandboxedRequire("matcher").defaultMatcher;
On 2016/09/30 09:37:54, Wladimir Palant wrote:
> As in the other test, it's probably cleaner to have all requires in a single
> statement so that you can use destructuring assignment consistently.

Done.

https://codereview.adblockplus.org/29354864/diff/29355316/test/filterListener...
test/filterListener.js:100: result[type] = result[type].sort();
On 2016/09/30 09:37:54, Wladimir Palant wrote:
> sort() modifies an array in place, the assignments here are pointless. It
should
> just be:
> 
>     expected[type].sort();
>   result[type].sort();

Done.

https://codereview.adblockplus.org/29354864/diff/29355316/test/stub-modules/c...
File test/stub-modules/common.js (right):

https://codereview.adblockplus.org/29354864/diff/29355316/test/stub-modules/c...
test/stub-modules/common.js:1: let path = require("path");
On 2016/09/30 09:37:54, Wladimir Palant wrote:
> 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?

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?

https://codereview.adblockplus.org/29354864/diff/29355316/test/stub-modules/c...
test/stub-modules/common.js:8: Cu: {import: () => { }},
On 2016/09/30 09:37:54, Wladimir Palant wrote:
> 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.

Done.

https://codereview.adblockplus.org/29354864/diff/29355316/test/stub-modules/c...
test/stub-modules/common.js:18: // of the module being transformed. This means
we must add all extra exports
On 2016/09/30 09:37:54, Wladimir Palant wrote:
> 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.

Oh cool, Done.

https://codereview.adblockplus.org/29354864/diff/29355316/test/stub-modules/c...
test/stub-modules/common.js:21: "  Object.defineProperty(exports, 'EXPORT',
{get: () => EXPORT});";
On 2016/09/30 09:37:55, Wladimir Palant wrote:
> 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}});
>     `;
>   }

Done.

https://codereview.adblockplus.org/29354864/diff/29355316/test/stub-modules/c...
test/stub-modules/common.js:30: //
https://github.com/palant/easypasswords/blob/master/gulp-utils.js
On 2016/09/30 09:37:55, Wladimir Palant wrote:
> 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.

Acknowledged.

https://codereview.adblockplus.org/29354864/diff/29355316/test/stub-modules/c...
test/stub-modules/common.js:31: let modules = new Set(["events", "io"]);
On 2016/09/30 09:37:55, Wladimir Palant wrote:
> 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.

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

https://codereview.adblockplus.org/29354864/diff/29355316/test/stub-modules/c...
test/stub-modules/common.js:58: return (moduleId, exports) =>
On 2016/09/30 09:37:55, Wladimir Palant wrote:
> exports parameter is unused.

Done.

https://codereview.adblockplus.org/29354864/diff/29355316/test/stub-modules/c...
test/stub-modules/common.js:66: return cache[key];
On 2016/09/30 09:37:54, Wladimir Palant wrote:
> 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.

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

[kzar, 2016-10-03 16:43:15]

Patch Set 7 : Moved test/stub-modules/common.js to test/common.js

https://codereview.adblockplus.org/29354864/diff/29355316/test/stub-modules/c...
File test/stub-modules/common.js (right):

https://codereview.adblockplus.org/29354864/diff/29355316/test/stub-modules/c...
test/stub-modules/common.js:1: let path = require("path");
On 2016/10/03 13:46:16, kzar wrote:
> On 2016/09/30 09:37:54, Wladimir Palant wrote:
> > 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?
> 
> 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?

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.

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

Patch Set 8 : Removed NODE_PATH env variable

https://codereview.adblockplus.org/29354864/diff/29355316/test/stub-modules/c...
File test/stub-modules/common.js (right):

https://codereview.adblockplus.org/29354864/diff/29355316/test/stub-modules/c...
test/stub-modules/common.js:31: let modules = new Set(["events", "io"]);
On 2016/09/30 09:37:55, Wladimir Palant wrote:
> 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.

Done.

[kzar, 2016-10-03 18:02:11]

Patch Set 9 : Allow tests to be run from different directory

[kzar, 2016-10-04 09:44:07]

Patch Set 10 : Fixed typo in test_runner.js

[Wladimir Palant, 2016-10-04 11:03:00]

https://codereview.adblockplus.org/29354864/diff/29355623/test/common.js
File test/common.js (right):

https://codereview.adblockplus.org/29354864/diff/29355623/test/common.js#newc...
test/common.js:68: for (let name of extraExports)
Wrong indentation here, single space instead of two.

https://codereview.adblockplus.org/29354864/diff/29355623/test/common.js#newc...
test/common.js:99: //  modules to be omitted for convenience.)
I don't consider this a good idea. As I mentioned before, the correct way to
require CommonJS modules is by their relative path - and that's what we should
really be doing. The core libraries currently don't use relative paths but
that's a hack and we'll likely fix this in future.

https://codereview.adblockplus.org/29354864/diff/29355623/test/cssRules.js
File test/cssRules.js (right):

https://codereview.adblockplus.org/29354864/diff/29355623/test/cssRules.js#ne...
test/cssRules.js:31: {Filter, CSSPropertyFilter} =
sandboxedRequire("filterClasses"),
This should be ../lib/filterClasses, same with other requires.

https://codereview.adblockplus.org/29354864/diff/29355623/test_runner.js
File test_runner.js (right):

https://codereview.adblockplus.org/29354864/diff/29355623/test_runner.js#newc...
test_runner.js:18: "use strict";
Gotta love Git, it considers your new test_wrapper.js a copy of lib/coreUtils.js
("similarity index 50%"). I suggest using -M100 -C100 when exporting your patch.

https://codereview.adblockplus.org/29354864/diff/29355623/test_runner.js#newc...
test_runner.js:26: function addTestPaths(testPaths, depth)
I wonder whether depth really needs to be a number. How about having a boolean
called recursive that will always be set to false on the recursive call? This
will also make sure that we only recurse once but be more obvious.

https://codereview.adblockplus.org/29354864/diff/29355623/test_runner.js#newc...
test_runner.js:35: addTestPaths(fs.readdirSync(testPath).map(file =>
path.join(testPath, file)),
1) This is still longer than 80 characters. Maybe get directory entries in a
separate statement, with a temporary variable?

2) Please always put brackets around a block if it spans multiple lines - even
if it is a single statement.

https://codereview.adblockplus.org/29354864/diff/29355623/test_runner.js#newc...
test_runner.js:37: else if (path.extname(testPath) == ".js" &&
path.basename(testPath) != "common.js")
How about something more generic than ignoring common.js explicitly? Maybe
rename it into _common.js and ignore anything starting with an underscore?

https://codereview.adblockplus.org/29354864/diff/29355623/test_runner.js#newc...
test_runner.js:41: addTestPaths(process.argv.length > 2 ? process.argv.slice(2)
: [path.join(__dirname, "test")], 2);
No point having this overly long line. Please split it to multiple lines, this
will also make it easier to read:

  if (process.argv.length > 2)
    addTestPaths(process.argv.slice(2), 2);
  else
    addTestPaths([path.join(__dirname, "test")], 2);

[kzar, 2016-10-04 12:06:25]

Patch Set 11 : Addressed feedback

https://codereview.adblockplus.org/29354864/diff/29355623/test/common.js
File test/common.js (right):

https://codereview.adblockplus.org/29354864/diff/29355623/test/common.js#newc...
test/common.js:68: for (let name of extraExports)
On 2016/10/04 11:02:57, Wladimir Palant wrote:
> Wrong indentation here, single space instead of two.

Whoops, Done.

https://codereview.adblockplus.org/29354864/diff/29355623/test/common.js#newc...
test/common.js:99: //  modules to be omitted for convenience.)
On 2016/10/04 11:02:57, Wladimir Palant wrote:
> I don't consider this a good idea. As I mentioned before, the correct way to
> require CommonJS modules is by their relative path - and that's what we should
> really be doing. The core libraries currently don't use relative paths but
> that's a hack and we'll likely fix this in future.

Done.

https://codereview.adblockplus.org/29354864/diff/29355623/test/cssRules.js
File test/cssRules.js (right):

https://codereview.adblockplus.org/29354864/diff/29355623/test/cssRules.js#ne...
test/cssRules.js:31: {Filter, CSSPropertyFilter} =
sandboxedRequire("filterClasses"),
On 2016/10/04 11:02:57, Wladimir Palant wrote:
> This should be ../lib/filterClasses, same with other requires.

Done.

https://codereview.adblockplus.org/29354864/diff/29355623/test_runner.js
File test_runner.js (right):

https://codereview.adblockplus.org/29354864/diff/29355623/test_runner.js#newc...
test_runner.js:18: "use strict";
On 2016/10/04 11:02:58, Wladimir Palant wrote:
> Gotta love Git, it considers your new test_wrapper.js a copy of
lib/coreUtils.js
> ("similarity index 50%"). I suggest using -M100 -C100 when exporting your
patch.

Acknowledged.

https://codereview.adblockplus.org/29354864/diff/29355623/test_runner.js#newc...
test_runner.js:26: function addTestPaths(testPaths, depth)
On 2016/10/04 11:02:57, Wladimir Palant wrote:
> I wonder whether depth really needs to be a number. How about having a boolean
> called recursive that will always be set to false on the recursive call? This
> will also make sure that we only recurse once but be more obvious.

Done.

https://codereview.adblockplus.org/29354864/diff/29355623/test_runner.js#newc...
test_runner.js:35: addTestPaths(fs.readdirSync(testPath).map(file =>
path.join(testPath, file)),
On 2016/10/04 11:02:58, Wladimir Palant wrote:
> 1) This is still longer than 80 characters. Maybe get directory entries in a
> separate statement, with a temporary variable?
> 
> 2) Please always put brackets around a block if it spans multiple lines - even
> if it is a single statement.

Done.

https://codereview.adblockplus.org/29354864/diff/29355623/test_runner.js#newc...
test_runner.js:37: else if (path.extname(testPath) == ".js" &&
path.basename(testPath) != "common.js")
On 2016/10/04 11:02:58, Wladimir Palant wrote:
> How about something more generic than ignoring common.js explicitly? Maybe
> rename it into _common.js and ignore anything starting with an underscore?

Done.

https://codereview.adblockplus.org/29354864/diff/29355623/test_runner.js#newc...
test_runner.js:41: addTestPaths(process.argv.length > 2 ? process.argv.slice(2)
: [path.join(__dirname, "test")], 2);
On 2016/10/04 11:02:59, Wladimir Palant wrote:
> No point having this overly long line. Please split it to multiple lines, this
> will also make it easier to read:
> 
>   if (process.argv.length > 2)
>     addTestPaths(process.argv.slice(2), 2);
>   else
>     addTestPaths([path.join(__dirname, "test")], 2);

Done.

[Wladimir Palant, 2016-10-04 12:15:06]

LGTM but feel free to address the nit below before pushing.

Nice, it looks like running tests should be Windows-compatible with these
changes.

https://codereview.adblockplus.org/29354864/diff/29355641/test_runner.js
File test_runner.js (right):

https://codereview.adblockplus.org/29354864/diff/29355641/test_runner.js#newc...
test_runner.js:40: path.basename(testPath)[0] != "_")
Nit: !path.basename(testPath).startsWith("_")

[kzar, 2016-10-04 12:17:27]

Patch Set 12 : Addressed final nit

https://codereview.adblockplus.org/29354864/diff/29355641/test_runner.js
File test_runner.js (right):

https://codereview.adblockplus.org/29354864/diff/29355641/test_runner.js#newc...
test_runner.js:40: path.basename(testPath)[0] != "_")
On 2016/10/04 12:15:05, Wladimir Palant wrote:
> Nit: !path.basename(testPath).startsWith("_")

Done.

[Wladimir Palant, 2016-10-04 12:26:27]

Even more LGTM.