Issue 6391 - Allow running the browser unit tests with Firefox

Issue 6391 - Allow running the browser unit tests with Firefox

This is a working implementation. For Firefox it uses WebDriver. 

[DONE] We could use the same implementation for Chromium, but I chose to not change this for now. Ideally we would only use WebDriver. What do you think?

[DONE needs testing on Win] It doesn't download binaries like it is done for Chromium.

[DONE] Also I'm pondering moving the *_process.js in a test/ subdirectory.

I have tested it on macOS and Linux.

[hub, 2018-03-12 20:34:44]

On 2018/03/12 20:23:55, hub wrote:

With these test, I get failures on Firefox. I will investigate this first.

[Sebastian Noack, 2018-03-12 23:17:31]

> Ideally we would only use WebDriver. What do you think?

That sounds sensible.

> It doesn't download binaries like it is done for Chromium.

IMO, it would be great if we could not only download one version of each
browser, but run the tests on multiple versions (e.g. oldest supported version,
current stable, and perhaps even some pre-releases).

> Also I'm pondering moving the *_process.js in a test/ subdirectory.

I was about to leave a comment about this, before I've seen you already
considered it yourself. IMO, it's not great how all that test runner logic ends
up spamming the project root.

https://codereview.adblockplus.org/29720661/diff/29720662/firefox_process.js
File firefox_process.js (right):

https://codereview.adblockplus.org/29720661/diff/29720662/firefox_process.js#...
firefox_process.js:75: runScript(script, scriptName, scriptArgs)
Isn't Promise.race([p]) equivalent to p?

[hub, 2018-03-13 01:59:25]

On 2018/03/12 23:17:31, Sebastian Noack wrote:
 
> > It doesn't download binaries like it is done for Chromium.
> 
> IMO, it would be great if we could not only download one version of each
> browser, but run the tests on multiple versions (e.g. oldest supported
version,
> current stable, and perhaps even some pre-releases).

Filed https://issues.adblockplus.org/ticket/6472 for that.

[hub, 2018-03-13 01:59:37]

https://codereview.adblockplus.org/29720661/diff/29720662/firefox_process.js
File firefox_process.js (right):

https://codereview.adblockplus.org/29720661/diff/29720662/firefox_process.js#...
firefox_process.js:75: runScript(script, scriptName, scriptArgs)
On 2018/03/12 23:17:31, Sebastian Noack wrote:
> Isn't Promise.race([p]) equivalent to p?

indeed. will fix that.

[hub, 2018-03-16 20:59:21]

now WebDriver is used buy default, but only for chromium. The old way is still
present to allow testing version < 63. There is still an important bug in
Chromium WebDriver as it seems executeScript() return before the end of the
script, preventing proper reporting.

[hub, 2018-03-19 20:40:11]

We download for Linux and macOS. The problem with Win32 is that Firefox releases
for Windows are only available as an installer.

[hub, 2018-03-21 20:54:59]

This should be feature complete now. I don't have a way to test on Windows yet
(I don't have Windows)

[hub, 2018-03-21 23:20:40]

I have tested this on Linux and macOS.

[kzar, 2018-04-18 17:14:44]

On 2018/03/21 20:54:59, hub wrote:
> This should be feature complete now. I don't have a way to test on Windows yet
> (I don't have Windows)

We have test machines for that which you can connect to remotely, see the
getting started guide
https://docs.google.com/document/d/1WnXJQDYwUBPdzLsQEZEi_nuYaibrtkR8ovuKIaG14...

[kzar, 2018-04-20 15:39:06]

> now WebDriver is used buy default, but only for chromium.
> The old way is still present to allow testing version < 63

Please could you put a comment by the old code to mention that it can be removed
once we no longer support versions of Chrome below 63?

https://codereview.adblockplus.org/29720661/diff/29729665/.hgignore
File .hgignore (right):

https://codereview.adblockplus.org/29720661/diff/29729665/.hgignore#newcode3
.hgignore:3: ^firefox-snapshots$
I guess we'll need to add this to the .gitignore as well.

https://codereview.adblockplus.org/29720661/diff/29729665/test/runners/chromi...
File test/runners/chromium_download.js (right):

https://codereview.adblockplus.org/29720661/diff/29729665/test/runners/chromi...
test/runners/chromium_download.js:18: /* eslint-env node */
Perhaps put these ESLint rules in a file test/runners/.eslintrc.json instead?
That's what we do in adblockpluscore for example[1].

[1] - https://hg.adblockplus.org/adblockpluscore/file/tip/lib/.eslintrc.json

[hub, 2018-04-24 14:05:56]

https://codereview.adblockplus.org/29720661/diff/29729665/.hgignore
File .hgignore (right):

https://codereview.adblockplus.org/29720661/diff/29729665/.hgignore#newcode3
.hgignore:3: ^firefox-snapshots$
On 2018/04/20 15:39:05, kzar wrote:
> I guess we'll need to add this to the .gitignore as well.

Done.

https://codereview.adblockplus.org/29720661/diff/29729665/test/runners/chromi...
File test/runners/chromium_download.js (right):

https://codereview.adblockplus.org/29720661/diff/29729665/test/runners/chromi...
test/runners/chromium_download.js:18: /* eslint-env node */
On 2018/04/20 15:39:05, kzar wrote:
> Perhaps put these ESLint rules in a file test/runners/.eslintrc.json instead?
> That's what we do in adblockpluscore for example[1].
> 
> [1] - https://hg.adblockplus.org/adblockpluscore/file/tip/lib/.eslintrc.json

Done.

[kzar, 2018-05-04 10:52:44]

When I tried your changes out I cleared node_modules, then ran `npm
install` before running `npm test`. Firefox and Chrome seemed to
download, the browser tests ran with Firefox and then everything hung
for some time. Eventually this exception was displayed:

>
/home/kzar/adblockpluschrome/adblockpluscore/node_modules/selenium-webdriver/lib/promise.js:2626
>         throw error;
>         ^
>
> WebDriverError: unknown error: Chrome failed to start: exited abnormally
>   (Driver info: chromedriver=2.36.540471
(9c759b81a907e70363c6312294d30b6ccccc2752),platform=Linux 4.15.0-3-amd64 x86_64)
>     at Object.checkLegacyResponse
(/home/kzar/adblockpluschrome/adblockpluscore/node_modules/selenium-webdriver/lib/error.js:546:15)
>     at parseHttpResponse
(/home/kzar/adblockpluschrome/adblockpluscore/node_modules/selenium-webdriver/lib/http.js:509:13)
>     at doSend.then.response
(/home/kzar/adblockpluschrome/adblockpluscore/node_modules/selenium-webdriver/lib/http.js:441:30)
>     at <anonymous>
>     at process._tickCallback (internal/process/next_tick.js:188:7)
> From: Task: WebDriver.createSession()
>     at Function.createSession
(/home/kzar/adblockpluschrome/adblockpluscore/node_modules/selenium-webdriver/lib/webdriver.js:769:24)
>     at Function.createSession
(/home/kzar/adblockpluschrome/adblockpluscore/node_modules/selenium-webdriver/chrome.js:761:15)
>     at createDriver
(/home/kzar/adblockpluschrome/adblockpluscore/node_modules/selenium-webdriver/index.js:170:33)
>     at Builder.build
(/home/kzar/adblockpluschrome/adblockpluscore/node_modules/selenium-webdriver/index.js:642:16)
>     at runScript
(/home/kzar/adblockpluschrome/adblockpluscore/test/runners/chromiumwd_process.js:40:10)
>     at ensureChromium.then.chromiumPath
(/home/kzar/adblockpluschrome/adblockpluscore/test/runners/chromiumwd_process.js:50:12)
>     at <anonymous>
> npm ERR! Test failed.  See above for more details.

I'm running Debian Buster, Node.js 8.11.1 and I applied your changes
on top of b3311140557e. Let me know if there are more details about my
environment that you need.

Also, it's already a problem that the test output was kind of
confusing. If the browser tests failed for example but the unit tests
passed you might mistakenly believe (or Travis might!) that everything
passed. I guess this is even worse now with 3(?) separate "Tests
Passed OK" messages showing at different times. You'll need to stare
at the output as it's being printed to make sure to spot a failure,
even if you're in the know about how the tests work. I wonder if we should
somehow
combine the overall Tests Passed / Failed messages until the very end instead.

On 2018/04/18 17:14:44, kzar wrote:
> On 2018/03/21 20:54:59, hub wrote:
> > ...I don't have a way to test on Windows yet (I don't have Windows)
>
> We have test machines for that...

So what about this one, did you get a chance to test your changes with
Windows yet?

https://codereview.adblockplus.org/29720661/diff/29760645/test/runners/chromi...
File test/runners/chromium_process.js (right):

https://codereview.adblockplus.org/29720661/diff/29760645/test/runners/chromi...
test/runners/chromium_process.js:140: console.log("\nTests in Chromium (Remote
Interface)\n");
Perhaps this message should specify if the tests running are the browser tests
or the unit tests? I found the output kinda unclear so far.

https://codereview.adblockplus.org/29720661/diff/29760645/test/runners/chromi...
File test/runners/chromiumwd_process.js (right):

https://codereview.adblockplus.org/29720661/diff/29760645/test/runners/chromi...
test/runners/chromiumwd_process.js:1: /*
Since this file (the Chromium WebDriver runner) is pretty similar to the Firefox
WebDriver runner I wonder if they can be combined. Or did you keep them separate
for a reason?

https://codereview.adblockplus.org/29720661/diff/29760645/test/runners/chromi...
test/runners/chromiumwd_process.js:27: // Chromium 63.0.3239.x
Seems like one of these comments can go?

https://codereview.adblockplus.org/29720661/diff/29760645/test/runners/firefo...
File test/runners/firefox_process.js (right):

https://codereview.adblockplus.org/29720661/diff/29760645/test/runners/firefo...
test/runners/firefox_process.js:52: return result;
Nit: I guess you could omit the curly braces and the return?

https://codereview.adblockplus.org/29720661/diff/29760645/test_runner.js
File test_runner.js (right):

https://codereview.adblockplus.org/29720661/diff/29760645/test_runner.js#newc...
test_runner.js:38: chromium_remote: chromiumProcess,
So do I have this right that by default the browser tests run under both Firefox
and Chromium via WebDriver. But if the BROWSER_TEST_RUNNERS environment variable
is specified that can be changed. The value of chromium_remote would mean that
the browser tests run with Chromium via chrome-remote-interface instead?

If so please could you add a comment here to explain the this logic? Also please
could you update the README to document how it works? Please could you make the
comment about WebDriver being available >= 63... clearer?

Also perhaps we should rename chromiumwd_process to chromium_process and
chromium_process to chromium_remote_interface_process? Then the names would be
more consistent between Firefox and Chromium.

Also I wonder if we could automatically decide between using WebDriver and
chrome-remote-interface depending on the version of Chromium present?

https://codereview.adblockplus.org/29720661/diff/29760645/test_runner.js#newc...
test_runner.js:44: function configureRunners()
I wonder what the point of this function is, since we immediately call it and
don't call it anywhere else? Unless I'm missing something we could just remove
it and put the logic inline here?

https://codereview.adblockplus.org/29720661/diff/29760645/test_runner.js#newc...
test_runner.js:52: runners.filter(runner =>
runnerDefinitions.hasOwnProperty(runner));
Shouldn't this instead be `return runners.filter...`? IIRC
Array.prototype.filter doesn't mutate the Array but rather returns a new one.

[hub, 2018-05-18 00:04:23]

Updated patch to address most of the issues.

I haven't figured out what's failing on your system though.

And yes, I agree that the messages about failure in the browser are confusing.

https://codereview.adblockplus.org/29720661/diff/29760645/test/runners/chromi...
File test/runners/chromium_process.js (right):

https://codereview.adblockplus.org/29720661/diff/29760645/test/runners/chromi...
test/runners/chromium_process.js:140: console.log("\nTests in Chromium (Remote
Interface)\n");
On 2018/05/04 10:52:43, kzar wrote:
> Perhaps this message should specify if the tests running are the browser tests
> or the unit tests? I found the output kinda unclear so far.

Done.

https://codereview.adblockplus.org/29720661/diff/29760645/test/runners/chromi...
File test/runners/chromiumwd_process.js (right):

https://codereview.adblockplus.org/29720661/diff/29760645/test/runners/chromi...
test/runners/chromiumwd_process.js:1: /*
On 2018/05/04 10:52:43, kzar wrote:
> Since this file (the Chromium WebDriver runner) is pretty similar to the
Firefox
> WebDriver runner I wonder if they can be combined. Or did you keep them
separate
> for a reason?

There are trivial enough that merging them would make them more complicated.

https://codereview.adblockplus.org/29720661/diff/29760645/test/runners/chromi...
test/runners/chromiumwd_process.js:27: // Chromium 63.0.3239.x
On 2018/05/04 10:52:43, kzar wrote:
> Seems like one of these comments can go?

Yes and no. It is kinda difficult to figure out which is what so I indicate what
version number to use for Chrome 65. I'll clarify.

https://codereview.adblockplus.org/29720661/diff/29760645/test/runners/firefo...
File test/runners/firefox_process.js (right):

https://codereview.adblockplus.org/29720661/diff/29760645/test/runners/firefo...
test/runners/firefox_process.js:52: return result;
On 2018/05/04 10:52:43, kzar wrote:
> Nit: I guess you could omit the curly braces and the return?

Done.

https://codereview.adblockplus.org/29720661/diff/29760645/test_runner.js
File test_runner.js (right):

https://codereview.adblockplus.org/29720661/diff/29760645/test_runner.js#newc...
test_runner.js:38: chromium_remote: chromiumProcess,
On 2018/05/04 10:52:44, kzar wrote:
> So do I have this right that by default the browser tests run under both
Firefox
> and Chromium via WebDriver. But if the BROWSER_TEST_RUNNERS environment
variable
> is specified that can be changed. 

Yes

> The value of chromium_remote would mean that
> the browser tests run with Chromium via chrome-remote-interface instead?

Yes

> 
> If so please could you add a comment here to explain the this logic? Also
please
> could you update the README to document how it works? Please could you make
the
> comment about WebDriver being available >= 63... clearer?

Will do

> Also perhaps we should rename chromiumwd_process to chromium_process and
> chromium_process to chromium_remote_interface_process? Then the names would be
> more consistent between Firefox and Chromium.

Ok.
 
> Also I wonder if we could automatically decide between using WebDriver and
> chrome-remote-interface depending on the version of Chromium present?

Currently we download a specific version, so that's the runner we want to use
that decide which version of the browsers to use. The same way a prior to this
patch.

https://codereview.adblockplus.org/29720661/diff/29760645/test_runner.js#newc...
test_runner.js:44: function configureRunners()
On 2018/05/04 10:52:44, kzar wrote:
> I wonder what the point of this function is, since we immediately call it and
> don't call it anywhere else? Unless I'm missing something we could just remove
> it and put the logic inline here?

I think it is nice to have this in its own function.

https://codereview.adblockplus.org/29720661/diff/29760645/test_runner.js#newc...
test_runner.js:52: runners.filter(runner =>
runnerDefinitions.hasOwnProperty(runner));
On 2018/05/04 10:52:43, kzar wrote:
> Shouldn't this instead be `return runners.filter...`? IIRC
> Array.prototype.filter doesn't mutate the Array but rather returns a new one.

good catch.

fixed.

[kzar, 2018-05-25 12:16:10]

> I haven't figured out what's failing on your system though.

Did you try using a Node.js more similar to mine? IIRC you're using 10
but I'm on 8.11.1. Have you tried clearing out node_modules and
reinstalling them as well?

> Also, it's already a problem that the test output was kind of
> confusing. If the browser tests failed for example but the unit tests
> passed you might mistakenly believe (or Travis might!) that everything
> passed. I guess this is even worse now with 3(?) separate "Tests
> Passed OK" messages showing at different times. You'll need to stare
> at the output as it's being printed to make sure to spot a failure,
> even if you're in the know about how the tests work. I wonder if we
> should somehow combine the overall Tests Passed / Failed messages
> until the very end instead.

What about this? What do you think about combining the output of browser and
unit tests so that the total pass / failed counts are combined and the overall
Success / Failure conclusion is too?

[hub, 2018-06-01 20:47:37]

On 2018/05/25 12:16:10, kzar wrote:
> > I haven't figured out what's failing on your system though.
> 
> Did you try using a Node.js more similar to mine? IIRC you're using 10
> but I'm on 8.11.1. Have you tried clearing out node_modules and
> reinstalling them as well?

Works with that setup too. But I'm on macOS. I'll give it a try on a Linux
setup. The error looks like it can't connect to Chrome.

> What about this? What do you think about combining the output of browser and
> unit tests so that the total pass / failed counts are combined and the overall
> Success / Failure conclusion is too?

I'll see what can be done. It wasn't done before either, so it isn't a
regression.

[kzar, 2018-06-04 17:50:03]

On 2018/06/01 20:47:37, hub wrote:
> On 2018/05/25 12:16:10, kzar wrote:
> > > I haven't figured out what's failing on your system though.
> > 
> > Did you try using a Node.js more similar to mine? IIRC you're using 10
> > but I'm on 8.11.1. Have you tried clearing out node_modules and
> > reinstalling them as well?
> 
> Works with that setup too. But I'm on macOS. I'll give it a try on a Linux
> setup. The error looks like it can't connect to Chrome.

Weird OK. Well let me know if I can do something to help test / track down the
cause.

> > What about this? What do you think about combining the output of browser and
> > unit tests so that the total pass / failed counts are combined and the
overall
> > Success / Failure conclusion is too?
> 
> I'll see what can be done. It wasn't done before either, so it isn't a
> regression.

It's not a regression as such, but this change does exacerbate the problem.

[hub, 2018-07-31 16:59:40]

Given the issues, I changed the default to be chrome remote interface (we have
been using so far) and firefox.

So we can get wider use of this.

I'll file an issue to deal with the failure in the browser test (since this is
not a regression)

[hub, 2018-07-31 17:11:35]

On 2018/07/31 16:59:40, hub wrote:

> I'll file an issue to deal with the failure in the browser test (since this is
> not a regression)

https://issues.adblockplus.org/ticket/6822

[kzar, 2018-08-03 15:22:51]

I've looked through the changes again quickly, and except for my comments I
can't see anything obvious. My main thought is that, where possible we should
use the minimum supported browser versions. But, I don't know if that is
possible.

My other concern is that I think you should test that this works on Windows.

Otherwise, LGTM.

> Given the issues, I changed the default to be chrome remote
> interface (we have been using so far) and firefox.

Nice, I've tested and it worked first time for me.

> I'll file an issue to deal with the failure in the browser test
> (since this is not a regression)
> https://issues.adblockplus.org/ticket/6822

Ace, thanks. I've triaged that.

https://codereview.adblockplus.org/29720661/diff/29843570/README.md
File README.md (right):

https://codereview.adblockplus.org/29720661/diff/29843570/README.md#newcode40
README.md:40: - "firefox": Firefox 60
Nit: Mind specifying that the Firefox tests run with WebDriver too?

https://codereview.adblockplus.org/29720661/diff/29843570/test/runners/chromi...
File test/runners/chromium_download.js (right):

https://codereview.adblockplus.org/29720661/diff/29843570/test/runners/chromi...
test/runners/chromium_download.js:79: let url =
`https://www.googleapis.com/download/storage/v1/b/chromium-browser-snapshots/o/${dir}%2F${chromiumRevision}%2F${fileName}?alt=media`;
Nit: Seems like the `url` variable is kind of pointless.

https://codereview.adblockplus.org/29720661/diff/29843570/test/runners/chromi...
File test/runners/chromium_process.js (right):

https://codereview.adblockplus.org/29720661/diff/29843570/test/runners/chromi...
test/runners/chromium_process.js:32: const CHROMIUM_REVISION = 508578;
Perhaps we should default to equivalent of Chrome 49 unless WebDriver is being
used? Since that's the lowest version we support, it might catch problems such
as the Unicode regular expression flag problem that bit us on the arse recently.

https://codereview.adblockplus.org/29720661/diff/29843570/test/runners/firefo...
File test/runners/firefox_download.js (right):

https://codereview.adblockplus.org/29720661/diff/29843570/test/runners/firefo...
test/runners/firefox_download.js:138: "win32-ia32": ["win32-EME-free", `Firefox
Setup ${firefoxVersion}.exe`],
Any particular reason we're going for the DRM free versions here?

https://codereview.adblockplus.org/29720661/diff/29843570/test/runners/firefo...
File test/runners/firefox_process.js (right):

https://codereview.adblockplus.org/29720661/diff/29843570/test/runners/firefo...
test/runners/firefox_process.js:27: const FIREFOX_VERSION = "60.0b4";
Shouldn't we use Firefox 51 here, since it's the earliest version we support?

https://codereview.adblockplus.org/29720661/diff/29843570/test/runners/firefo...
test/runners/firefox_process.js:51: .catch(error =>
How come you catch the exception, only to re-throw it?

[hub, 2018-08-03 16:13:54]

updated patch

https://codereview.adblockplus.org/29720661/diff/29843570/README.md
File README.md (right):

https://codereview.adblockplus.org/29720661/diff/29843570/README.md#newcode40
README.md:40: - "firefox": Firefox 60
On 2018/08/03 15:22:49, kzar wrote:
> Nit: Mind specifying that the Firefox tests run with WebDriver too?

Done.

https://codereview.adblockplus.org/29720661/diff/29843570/test/runners/chromi...
File test/runners/chromium_download.js (right):

https://codereview.adblockplus.org/29720661/diff/29843570/test/runners/chromi...
test/runners/chromium_download.js:79: let url =
`https://www.googleapis.com/download/storage/v1/b/chromium-browser-snapshots/o/${dir}%2F${chromiumRevision}%2F${fileName}?alt=media`;
On 2018/08/03 15:22:49, kzar wrote:
> Nit: Seems like the `url` variable is kind of pointless.

Done.

https://codereview.adblockplus.org/29720661/diff/29843570/test/runners/chromi...
File test/runners/chromium_process.js (right):

https://codereview.adblockplus.org/29720661/diff/29843570/test/runners/chromi...
test/runners/chromium_process.js:32: const CHROMIUM_REVISION = 508578;
On 2018/08/03 15:22:50, kzar wrote:
> Perhaps we should default to equivalent of Chrome 49 unless WebDriver is being
> used? Since that's the lowest version we support, it might catch problems such
> as the Unicode regular expression flag problem that bit us on the arse
recently.

Here it is webdriver, and as I mentionned, we can't get lower than that.
chromium_remote_process.js will download 467222 which is the version Wladimir
had set on. I'll see if I can lower it, but finding the right version is a lot
of work: Google doesn't make it easy at all.

https://codereview.adblockplus.org/29720661/diff/29843570/test/runners/firefo...
File test/runners/firefox_download.js (right):

https://codereview.adblockplus.org/29720661/diff/29843570/test/runners/firefo...
test/runners/firefox_download.js:138: "win32-ia32": ["win32-EME-free", `Firefox
Setup ${firefoxVersion}.exe`],
On 2018/08/03 15:22:50, kzar wrote:
> Any particular reason we're going for the DRM free versions here?

it is not encumbered with technologies that falls into the perview of DMCA and
similar laws (Canada has one) that makes it a legal trouble.

IANAL, but I don't see a reason why we would need it, so let's go without.

https://codereview.adblockplus.org/29720661/diff/29843570/test/runners/firefo...
File test/runners/firefox_process.js (right):

https://codereview.adblockplus.org/29720661/diff/29843570/test/runners/firefo...
test/runners/firefox_process.js:27: const FIREFOX_VERSION = "60.0b4";
On 2018/08/03 15:22:50, kzar wrote:
> Shouldn't we use Firefox 51 here, since it's the earliest version we support?

57 is the recommended (supported) minimum version of Firefox for the
`geckodriver`. 51 clearly doesn't work. I could get 56 to work, so I'll go with
56 for now.

https://codereview.adblockplus.org/29720661/diff/29843570/test/runners/firefo...
test/runners/firefox_process.js:51: .catch(error =>
On 2018/08/03 15:22:50, kzar wrote:
> How come you catch the exception, only to re-throw it?

catch() also deal with rejected promises.

[kzar, 2018-08-03 18:30:13]

LGTM

https://codereview.adblockplus.org/29720661/diff/29843570/test/runners/chromi...
File test/runners/chromium_process.js (right):

https://codereview.adblockplus.org/29720661/diff/29843570/test/runners/chromi...
test/runners/chromium_process.js:32: const CHROMIUM_REVISION = 508578;
On 2018/08/03 16:13:53, hub wrote:
> On 2018/08/03 15:22:50, kzar wrote:
> > Perhaps we should default to equivalent of Chrome 49 unless WebDriver is
being
> > used? Since that's the lowest version we support, it might catch problems
such
> > as the Unicode regular expression flag problem that bit us on the arse
> recently.
> 
> Here it is webdriver, and as I mentionned, we can't get lower than that.
> chromium_remote_process.js will download 467222 which is the version Wladimir
> had set on. I'll see if I can lower it, but finding the right version is a lot
> of work: Google doesn't make it easy at all.

If we can use Chrome 49 without WebDriver it would be cool, I think that could
reduce regressions that Ross keeps catching. But totally fair enough if that's
too tricky or if it's impossible.

https://codereview.adblockplus.org/29720661/diff/29843570/test/runners/firefo...
File test/runners/firefox_download.js (right):

https://codereview.adblockplus.org/29720661/diff/29843570/test/runners/firefo...
test/runners/firefox_download.js:138: "win32-ia32": ["win32-EME-free", `Firefox
Setup ${firefoxVersion}.exe`],
On 2018/08/03 16:13:53, hub wrote:
> On 2018/08/03 15:22:50, kzar wrote:
> > Any particular reason we're going for the DRM free versions here?
> 
> it is not encumbered with technologies that falls into the perview of DMCA and
> similar laws (Canada has one) that makes it a legal trouble.
> 
> IANAL, but I don't see a reason why we would need it, so let's go without.

Fair enough, seems reasonable.

https://codereview.adblockplus.org/29720661/diff/29843570/test/runners/firefo...
File test/runners/firefox_process.js (right):

https://codereview.adblockplus.org/29720661/diff/29843570/test/runners/firefo...
test/runners/firefox_process.js:27: const FIREFOX_VERSION = "60.0b4";
On 2018/08/03 16:13:53, hub wrote:
> On 2018/08/03 15:22:50, kzar wrote:
> > Shouldn't we use Firefox 51 here, since it's the earliest version we
support?
> 
> 57 is the recommended (supported) minimum version of Firefox for the
> `geckodriver`. 51 clearly doesn't work. I could get 56 to work, so I'll go
with
> 56 for now.

Fair enough.

https://codereview.adblockplus.org/29720661/diff/29843570/test/runners/firefo...
test/runners/firefox_process.js:51: .catch(error =>
On 2018/08/03 16:13:53, hub wrote:
> On 2018/08/03 15:22:50, kzar wrote:
> > How come you catch the exception, only to re-throw it?
> 
> catch() also deal with rejected promises.

Acknowledged.

[hub, 2018-08-03 18:55:45]

https://codereview.adblockplus.org/29720661/diff/29843570/test/runners/chromi...
File test/runners/chromium_process.js (right):

https://codereview.adblockplus.org/29720661/diff/29843570/test/runners/chromi...
test/runners/chromium_process.js:32: const CHROMIUM_REVISION = 508578;
On 2018/08/03 18:30:11, kzar wrote:
> On 2018/08/03 16:13:53, hub wrote:
> > On 2018/08/03 15:22:50, kzar wrote:
> > > Perhaps we should default to equivalent of Chrome 49 unless WebDriver is
> being
> > > used? Since that's the lowest version we support, it might catch problems
> such
> > > as the Unicode regular expression flag problem that bit us on the arse
> > recently.
> > 
> > Here it is webdriver, and as I mentionned, we can't get lower than that.
> > chromium_remote_process.js will download 467222 which is the version
Wladimir
> > had set on. I'll see if I can lower it, but finding the right version is a
lot
> > of work: Google doesn't make it easy at all.
> 
> If we can use Chrome 49 without WebDriver it would be cool, I think that could
> reduce regressions that Ross keeps catching. But totally fair enough if that's
> too tricky or if it's impossible.

I managed to find the build, I tried (using the chrome remote interface), and it
doesn't work. There is an error, and then Chromium crashes. I can spend some
time later, but right now it is definitely not a simple build number change.