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

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

(This includes Hubert's earlier work https://codereview.adblockplus.org/29460576/ )

[kzar, 2017-08-16 15:55:54]

Patch Set 1

Got there in the end!

[hub, 2017-08-16 16:14:47]

https://codereview.adblockplus.org/29517687/diff/29517688/lib/content/elemHid...
File lib/content/elemHideEmulation.js (right):

https://codereview.adblockplus.org/29517687/diff/29517688/lib/content/elemHid...
lib/content/elemHideEmulation.js:479: exports.splitSelector = splitSelector;
do we still need this since it is exported from common.js ?

[Wladimir Palant, 2017-08-17 10:05:37]

The initial idea was that chromium_process.js would be a generic helper whereas
test_runner.js would contain all the logic required specifically for unit tests.
The thought was also that we might move this functionality to npm at some point.
So maybe it would be better to run WebPack in test_runner.js and merely pass in
a page, resulting source code and parameters to chromium_process.js. Then it
could still execute the source code on the given page in a generic fashion.

https://codereview.adblockplus.org/29517687/diff/29517688/chromium_process.js
File chromium_process.js (right):

https://codereview.adblockplus.org/29517687/diff/29517688/chromium_process.js...
chromium_process.js:32: const MemoryFS = require("memory-fs");
I assume that this module exists because WebPack is using it? Still, if we are
using it we should add it as our dependency explicitly.

Also a nit: I'm using the Python style for importing modules here, for better
overview. So please sort the imports of third-party modules alphabetically.

https://codereview.adblockplus.org/29517687/diff/29517688/chromium_process.js...
chromium_process.js:260: webpackCompiler.outputFileSystem = memoryFS;
Nit: It's better to put all the statements above inside the promise as well.
This way, we can rely on the function not throwing but only rejecting the
promise on error.

https://codereview.adblockplus.org/29517687/diff/29517688/chromium_process.js...
chromium_process.js:279: memoryFS.unlink("/" + bundleFilename, () => {
resolve(bundle); });
Nit: Use memoryFS.unlinkSync() here to make this more straightforward? Besides,
this callback doesn't look like it would be accepted by ESLint.

https://codereview.adblockplus.org/29517687/diff/29517688/chromium_process.js...
chromium_process.js:327: // I would have rathered use exports-loader here, to
avoid treating
"rather used"?

https://codereview.adblockplus.org/29517687/diff/29517688/chromium_process.js...
chromium_process.js:387: m => "." + path.sep + path.relative(path.join("test",
"browser"), m)
The point here is generating a list of module names suitable for require(). This
means that path.sep is wrong - it will be "\" on Windows, yet path separator in
module names is always "/".

Also, you are building paths relative to test/browser - this is assuming that
the current directory is pointing to repository root. This should be
path.join(__dirname, "test", "browser").

Besides, it seems that you should actually return 
path.relative(path.join(__dirname, "test", "browser"), m).replace(/\.js$/, "")
here. This way you can use require("./" + module + ".js") in _bootstrap.js
without having to create an explicit context.

https://codereview.adblockplus.org/29517687/diff/29517688/test/browser/_boots...
File test/browser/_bootstrap.js (right):

https://codereview.adblockplus.org/29517687/diff/29517688/test/browser/_boots...
test/browser/_bootstrap.js:25: function runTests(browserTestModules)
Nit: Frankly, I'm a bit annoyed by the frequent repetition of "browserTest" -
this is redundant, the directory name already says so. moduleNames maybe?

https://codereview.adblockplus.org/29517687/diff/29517688/test/browser/_boots...
test/browser/_bootstrap.js:43: browserTestModules =
browserTestModuleContext.keys();
Why do we need this? test_runner.js is where we are compiling the list of
modules, here we should just run what we get.

https://codereview.adblockplus.org/29517687/diff/29517688/test/browser/_boots...
test/browser/_bootstrap.js:47: // only use the necessary ones here.
Why is this comment here? That's not where the bundling happens.

https://codereview.adblockplus.org/29517687/diff/29517688/test/browser/_boots...
test/browser/_bootstrap.js:48: let testModules = {};
Nit: tests should be unambiguous enough as a name for this variable?

https://codereview.adblockplus.org/29517687/diff/29517688/test/browser/_boots...
test/browser/_bootstrap.js:50: testModules[module] =
nodeunit.testCase(browserTestModuleContext(module));
As mentioned in comment on chrome_process.js, require("./" + module + ".js")
should do here without an explicit context.

https://codereview.adblockplus.org/29517687/diff/29517688/test/browser/elemHi...
File test/browser/elemHideEmulation.js (right):

https://codereview.adblockplus.org/29517687/diff/29517688/test/browser/elemHi...
test/browser/elemHideEmulation.js:20: const {ElemHideEmulation} =
require("content/elemHideEmulation");
require("../../lib/content/elemHideEmulation")? Sure, this is quite a bit
longer, but it also doesn't rely on implicit module paths.

https://codereview.adblockplus.org/29517687/diff/29517688/test/browser/elemHi...
test/browser/elemHideEmulation.js:111: return elemHideEmulation.apply();
elemHideEmulation.apply() doesn't return any result, you shouldn't make it look
like it does. As it is, `return` here is unnecessary.

Anyway, you shouldn't change this function towards being synchronous. Hubert is
currently implementing a change that will make element hiding emulation apply
asynchronously. So rather than change all tests to run synchronously, just make
this function return a promise. The following will do:

  return Promise.resolve().then(() =>
  {
    // actual function body here
  });

This way any errors thrown here will result in the promise being rejected and
handled by the same code path as all the other errors.

[Wladimir Palant, 2017-08-17 10:07:31]

https://codereview.adblockplus.org/29517687/diff/29517688/chromium_process.js
File chromium_process.js (right):

https://codereview.adblockplus.org/29517687/diff/29517688/chromium_process.js...
chromium_process.js:293: });
Actually, this shouldn't be necessary any more. Running the tests on about:blank
should be fine, we are no longer loading any file:/// URLs.

[kzar, 2017-08-17 12:40:22]

Patch Set 2 : Addressed Wladimir's and Hubert's initial feedback

https://codereview.adblockplus.org/29517687/diff/29517688/chromium_process.js
File chromium_process.js (right):

https://codereview.adblockplus.org/29517687/diff/29517688/chromium_process.js...
chromium_process.js:32: const MemoryFS = require("memory-fs");
On 2017/08/17 10:05:38, Wladimir Palant wrote:
> I assume that this module exists because WebPack is using it? Still, if we are
> using it we should add it as our dependency explicitly.
> 
> Also a nit: I'm using the Python style for importing modules here, for better
> overview. So please sort the imports of third-party modules alphabetically.

Done.

https://codereview.adblockplus.org/29517687/diff/29517688/chromium_process.js...
chromium_process.js:260: webpackCompiler.outputFileSystem = memoryFS;
On 2017/08/17 10:05:37, Wladimir Palant wrote:
> Nit: It's better to put all the statements above inside the promise as well.
> This way, we can rely on the function not throwing but only rejecting the
> promise on error.

Done.

https://codereview.adblockplus.org/29517687/diff/29517688/chromium_process.js...
chromium_process.js:279: memoryFS.unlink("/" + bundleFilename, () => {
resolve(bundle); });
On 2017/08/17 10:05:38, Wladimir Palant wrote:
> Nit: Use memoryFS.unlinkSync() here to make this more straightforward?
Besides,
> this callback doesn't look like it would be accepted by ESLint.

Done.

https://codereview.adblockplus.org/29517687/diff/29517688/chromium_process.js...
chromium_process.js:293: });
On 2017/08/17 10:07:30, Wladimir Palant wrote:
> Actually, this shouldn't be necessary any more. Running the tests on
about:blank
> should be fine, we are no longer loading any file:/// URLs.

Done.

https://codereview.adblockplus.org/29517687/diff/29517688/chromium_process.js...
chromium_process.js:327: // I would have rathered use exports-loader here, to
avoid treating
On 2017/08/17 10:05:37, Wladimir Palant wrote:
> "rather used"?

Done.

https://codereview.adblockplus.org/29517687/diff/29517688/chromium_process.js...
chromium_process.js:387: m => "." + path.sep + path.relative(path.join("test",
"browser"), m)
On 2017/08/17 10:05:37, Wladimir Palant wrote:
> The point here is generating a list of module names suitable for require().
This
> means that path.sep is wrong - it will be "\" on Windows, yet path separator
in
> module names is always "/".
> 
> Also, you are building paths relative to test/browser - this is assuming that
> the current directory is pointing to repository root. This should be
> path.join(__dirname, "test", "browser").
> 
> Besides, it seems that you should actually return 
> path.relative(path.join(__dirname, "test", "browser"), m).replace(/\.js$/, "")
> here. This way you can use require("./" + module + ".js") in _bootstrap.js
> without having to create an explicit context.

Done.

https://codereview.adblockplus.org/29517687/diff/29517688/lib/content/elemHid...
File lib/content/elemHideEmulation.js (right):

https://codereview.adblockplus.org/29517687/diff/29517688/lib/content/elemHid...
lib/content/elemHideEmulation.js:479: exports.splitSelector = splitSelector;
On 2017/08/16 16:14:46, hub wrote:
> do we still need this since it is exported from common.js ?

Done.

https://codereview.adblockplus.org/29517687/diff/29517688/test/browser/_boots...
File test/browser/_bootstrap.js (right):

https://codereview.adblockplus.org/29517687/diff/29517688/test/browser/_boots...
test/browser/_bootstrap.js:25: function runTests(browserTestModules)
On 2017/08/17 10:05:38, Wladimir Palant wrote:
> Nit: Frankly, I'm a bit annoyed by the frequent repetition of "browserTest" -
> this is redundant, the directory name already says so. moduleNames maybe?

Done.

https://codereview.adblockplus.org/29517687/diff/29517688/test/browser/_boots...
test/browser/_bootstrap.js:43: browserTestModules =
browserTestModuleContext.keys();
On 2017/08/17 10:05:38, Wladimir Palant wrote:
> Why do we need this? test_runner.js is where we are compiling the list of
> modules, here we should just run what we get.

Done.

https://codereview.adblockplus.org/29517687/diff/29517688/test/browser/_boots...
test/browser/_bootstrap.js:47: // only use the necessary ones here.
On 2017/08/17 10:05:38, Wladimir Palant wrote:
> Why is this comment here? That's not where the bundling happens.

Done.

https://codereview.adblockplus.org/29517687/diff/29517688/test/browser/_boots...
test/browser/_bootstrap.js:48: let testModules = {};
On 2017/08/17 10:05:38, Wladimir Palant wrote:
> Nit: tests should be unambiguous enough as a name for this variable?

Done.

https://codereview.adblockplus.org/29517687/diff/29517688/test/browser/_boots...
test/browser/_bootstrap.js:50: testModules[module] =
nodeunit.testCase(browserTestModuleContext(module));
On 2017/08/17 10:05:38, Wladimir Palant wrote:
> As mentioned in comment on chrome_process.js, require("./" + module + ".js")
> should do here without an explicit context.

Done.

https://codereview.adblockplus.org/29517687/diff/29517688/test/browser/elemHi...
File test/browser/elemHideEmulation.js (right):

https://codereview.adblockplus.org/29517687/diff/29517688/test/browser/elemHi...
test/browser/elemHideEmulation.js:20: const {ElemHideEmulation} =
require("content/elemHideEmulation");
On 2017/08/17 10:05:38, Wladimir Palant wrote:
> require("../../lib/content/elemHideEmulation")? Sure, this is quite a bit
> longer, but it also doesn't rely on implicit module paths.

Done.

https://codereview.adblockplus.org/29517687/diff/29517688/test/browser/elemHi...
test/browser/elemHideEmulation.js:111: return elemHideEmulation.apply();
On 2017/08/17 10:05:39, Wladimir Palant wrote:
> elemHideEmulation.apply() doesn't return any result, you shouldn't make it
look
> like it does. As it is, `return` here is unnecessary.
> 
> Anyway, you shouldn't change this function towards being synchronous. Hubert
is
> currently implementing a change that will make element hiding emulation apply
> asynchronously. So rather than change all tests to run synchronously, just
make
> this function return a promise. The following will do:
> 
>   return Promise.resolve().then(() =>
>   {
>     // actual function body here
>   });
> 
> This way any errors thrown here will result in the promise being rejected and
> handled by the same code path as all the other errors.

Done.

[kzar, 2017-08-17 12:59:50]

Patch Set 3 : runBrowserTests should always return a Promise

[Wladimir Palant, 2017-08-17 13:06:11]

https://codereview.adblockplus.org/29517687/diff/29518555/test_runner.js
File test_runner.js (right):

https://codereview.adblockplus.org/29517687/diff/29518555/test_runner.js#newc...
test_runner.js:27: const MemoryFS = require("memory-fs");
Nit: sort the imports alphabetically?

https://codereview.adblockplus.org/29517687/diff/29518555/test_runner.js#newc...
test_runner.js:57: );
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().

https://codereview.adblockplus.org/29517687/diff/29518555/test_runner.js#newc...
test_runner.js:65: function webpackInMemory(bundleFilename, options)
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.

https://codereview.adblockplus.org/29517687/diff/29518555/test_runner.js#newc...
test_runner.js:69: // Based on this example
https://webpack.js.org/api/node/#custom-file-systems
Nit: overlong line, move URL to next line?

https://codereview.adblockplus.org/29517687/diff/29518555/test_runner.js#newc...
test_runner.js:113: // I would have rathered used exports-loader here, to avoid
treating
"rathered" => "rather"?

https://codereview.adblockplus.org/29517687/diff/29518555/test_runner.js#newc...
test_runner.js:125: modules: [path.resolve(__dirname, "lib")]
We don't need this any more, do we?

https://codereview.adblockplus.org/29517687/diff/29518566/test_runner.js
File test_runner.js (right):

https://codereview.adblockplus.org/29517687/diff/29518566/test_runner.js#newc...
test_runner.js:102: return Promise.resolve();
Or maybe just change the call below from runBrowserTests() to
Promise.resolve(runBrowserTests()) again?

[kzar, 2017-08-17 13:26:44]

Patch Set 4 : Addressed nits

https://codereview.adblockplus.org/29517687/diff/29518555/test_runner.js
File test_runner.js (right):

https://codereview.adblockplus.org/29517687/diff/29518555/test_runner.js#newc...
test_runner.js:27: const MemoryFS = require("memory-fs");
On 2017/08/17 13:06:10, Wladimir Palant wrote:
> Nit: sort the imports alphabetically?

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.

https://codereview.adblockplus.org/29517687/diff/29518555/test_runner.js#newc...
test_runner.js:57: );
On 2017/08/17 13:06:10, Wladimir Palant wrote:
> 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().

Done.

https://codereview.adblockplus.org/29517687/diff/29518555/test_runner.js#newc...
test_runner.js:65: function webpackInMemory(bundleFilename, options)
On 2017/08/17 13:06:10, Wladimir Palant wrote:
> 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.

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.

https://codereview.adblockplus.org/29517687/diff/29518555/test_runner.js#newc...
test_runner.js:69: // Based on this example
https://webpack.js.org/api/node/#custom-file-systems
On 2017/08/17 13:06:10, Wladimir Palant wrote:
> Nit: overlong line, move URL to next line?

Done.

https://codereview.adblockplus.org/29517687/diff/29518555/test_runner.js#newc...
test_runner.js:113: // I would have rathered used exports-loader here, to avoid
treating
On 2017/08/17 13:06:10, Wladimir Palant wrote:
> "rathered" => "rather"?

Done.

https://codereview.adblockplus.org/29517687/diff/29518555/test_runner.js#newc...
test_runner.js:125: modules: [path.resolve(__dirname, "lib")]
On 2017/08/17 13:06:10, Wladimir Palant wrote:
> We don't need this any more, do we?

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

https://codereview.adblockplus.org/29517687/diff/29518566/test_runner.js
File test_runner.js (right):

https://codereview.adblockplus.org/29517687/diff/29518566/test_runner.js#newc...
test_runner.js:102: return Promise.resolve();
On 2017/08/17 13:06:11, Wladimir Palant wrote:
> Or maybe just change the call below from runBrowserTests() to
> Promise.resolve(runBrowserTests()) again?

Done.

[Wladimir Palant, 2017-08-18 20:49:57]

LGTM with the nit below addressed.

https://codereview.adblockplus.org/29517687/diff/29518555/test_runner.js
File test_runner.js (right):

https://codereview.adblockplus.org/29517687/diff/29518555/test_runner.js#newc...
test_runner.js:27: const MemoryFS = require("memory-fs");
On 2017/08/17 13:26:42, kzar wrote:
> Well I did but case sensitively, I considered A to fall after z.

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

[hub, 2017-08-18 20:52:36]

LGTM (with the same nit)

I'll let you land this first so I can rebase issue #5438

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

https://codereview.adblockplus.org/29517687/diff/29518555/test_runner.js
File test_runner.js (right):

https://codereview.adblockplus.org/29517687/diff/29518555/test_runner.js#newc...
test_runner.js:27: const MemoryFS = require("memory-fs");
On 2017/08/18 20:49:56, Wladimir Palant wrote:
> On 2017/08/17 13:26:42, kzar wrote:
> > Well I did but case sensitively, I considered A to fall after z.
> 
> Actually, people usually sort by module name, not by the names of imported
> symbols ;)

Done.