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

Issue 29720661: Issue 6391 - Allow running the browser unit tests with Firefox (Closed)

Created:
March 12, 2018, 8:23 p.m. by hub
Modified:
Aug. 8, 2018, 2:19 p.m.
Reviewers:
Sebastian Noack, kzar
CC:
sergei
Base URL:
https://hg.adblockplus.org/adblockpluscore/
Visibility:
Public.

Description

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.

Patch Set 1 #

Total comments: 2

Patch Set 2 : Move the _process.js files. Remove the need for index.html #

Patch Set 3 : Support using chromium with webdriver. #

Patch Set 4 : Added firefox download #

Patch Set 5 : Download the Windows installer (albeit untested) #

Patch Set 6 : Fix the truncated output in Chromium. #

Total comments: 4

Patch Set 7 : eslint options and gitignore #

Total comments: 14

Patch Set 8 : Review comments addressed. #

Patch Set 9 : README fixups #

Patch Set 10 : Revert to default to the chrome remote interface (instead of webdriver) #

Total comments: 17

Patch Set 11 : Lower Firefox version. Nit in the README #

Patch Set 12 : Update Firefox version in README #

Unified diffs Side-by-side diffs Delta from patch set Stats (+548 lines, -427 lines) Patch
M .gitignore View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M .hgignore View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M README.md View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +11 lines, -0 lines 0 comments Download
M package.json View 1 2 3 1 chunk +5 lines, -0 lines 0 comments Download
A test/runners/.eslintrc.json View 1 2 3 4 5 6 1 chunk +8 lines, -0 lines 0 comments Download
A test/runners/chromium_download.js View 1 2 3 4 5 6 7 8 9 10 1 chunk +91 lines, -0 lines 0 comments Download
M test/runners/chromium_process.js View 1 2 3 4 5 6 7 1 chunk +28 lines, -293 lines 0 comments Download
M test/runners/chromium_remote_process.js View 1 2 3 4 5 6 7 4 chunks +6 lines, -123 lines 0 comments Download
A test/runners/download.js View 1 2 3 4 5 6 1 chunk +80 lines, -0 lines 0 comments Download
A test/runners/firefox_download.js View 1 2 3 4 5 6 1 chunk +180 lines, -0 lines 0 comments Download
A test/runners/firefox_process.js View 1 2 3 4 5 6 7 8 9 10 1 chunk +56 lines, -0 lines 0 comments Download
A test/runners/webdriver.js View 1 2 3 4 5 6 7 1 chunk +44 lines, -0 lines 0 comments Download
M test_runner.js View 1 2 3 4 5 6 7 8 9 3 chunks +37 lines, -11 lines 0 comments Download

Messages

Total messages: 23
hub
March 12, 2018, 8:23 p.m. (2018-03-12 20:23:55 UTC) #1
hub
On 2018/03/12 20:23:55, hub wrote: With these test, I get failures on Firefox. I will ...
March 12, 2018, 8:34 p.m. (2018-03-12 20:34:44 UTC) #2
Sebastian Noack
> Ideally we would only use WebDriver. What do you think? That sounds sensible. > ...
March 12, 2018, 11:17 p.m. (2018-03-12 23:17:31 UTC) #3
hub
On 2018/03/12 23:17:31, Sebastian Noack wrote: > > It doesn't download binaries like it is ...
March 13, 2018, 1:59 a.m. (2018-03-13 01:59:25 UTC) #4
hub
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#newcode75 firefox_process.js:75: runScript(script, scriptName, scriptArgs) On 2018/03/12 23:17:31, Sebastian Noack wrote: ...
March 13, 2018, 1:59 a.m. (2018-03-13 01:59:37 UTC) #5
hub
now WebDriver is used buy default, but only for chromium. The old way is still ...
March 16, 2018, 8:59 p.m. (2018-03-16 20:59:21 UTC) #6
hub
We download for Linux and macOS. The problem with Win32 is that Firefox releases for ...
March 19, 2018, 8:40 p.m. (2018-03-19 20:40:11 UTC) #7
hub
This should be feature complete now. I don't have a way to test on Windows ...
March 21, 2018, 8:54 p.m. (2018-03-21 20:54:59 UTC) #8
hub
I have tested this on Linux and macOS.
March 21, 2018, 11:20 p.m. (2018-03-21 23:20:40 UTC) #9
kzar
On 2018/03/21 20:54:59, hub wrote: > This should be feature complete now. I don't have ...
April 18, 2018, 5:14 p.m. (2018-04-18 17:14:44 UTC) #10
kzar
> now WebDriver is used buy default, but only for chromium. > The old way ...
April 20, 2018, 3:39 p.m. (2018-04-20 15:39:06 UTC) #11
hub
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 ...
April 24, 2018, 2:05 p.m. (2018-04-24 14:05:56 UTC) #12
kzar
When I tried your changes out I cleared node_modules, then ran `npm install` before running ...
May 4, 2018, 10:52 a.m. (2018-05-04 10:52:44 UTC) #13
hub
Updated patch to address most of the issues. I haven't figured out what's failing on ...
May 18, 2018, 12:04 a.m. (2018-05-18 00:04:23 UTC) #14
kzar
> I haven't figured out what's failing on your system though. Did you try using ...
May 25, 2018, 12:16 p.m. (2018-05-25 12:16:10 UTC) #15
hub
On 2018/05/25 12:16:10, kzar wrote: > > I haven't figured out what's failing on your ...
June 1, 2018, 8:47 p.m. (2018-06-01 20:47:37 UTC) #16
kzar
On 2018/06/01 20:47:37, hub wrote: > On 2018/05/25 12:16:10, kzar wrote: > > > I ...
June 4, 2018, 5:50 p.m. (2018-06-04 17:50:03 UTC) #17
hub
Given the issues, I changed the default to be chrome remote interface (we have been ...
July 31, 2018, 4:59 p.m. (2018-07-31 16:59:40 UTC) #18
hub
On 2018/07/31 16:59:40, hub wrote: > I'll file an issue to deal with the failure ...
July 31, 2018, 5:11 p.m. (2018-07-31 17:11:35 UTC) #19
kzar
I've looked through the changes again quickly, and except for my comments I can't see ...
Aug. 3, 2018, 3:22 p.m. (2018-08-03 15:22:51 UTC) #20
hub
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, ...
Aug. 3, 2018, 4:13 p.m. (2018-08-03 16:13:54 UTC) #21
kzar
LGTM https://codereview.adblockplus.org/29720661/diff/29843570/test/runners/chromium_process.js File test/runners/chromium_process.js (right): https://codereview.adblockplus.org/29720661/diff/29843570/test/runners/chromium_process.js#newcode32 test/runners/chromium_process.js:32: const CHROMIUM_REVISION = 508578; On 2018/08/03 16:13:53, hub ...
Aug. 3, 2018, 6:30 p.m. (2018-08-03 18:30:13 UTC) #22
hub
Aug. 3, 2018, 6:55 p.m. (2018-08-03 18:55:45 UTC) #23
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.

Powered by Google App Engine
This is Rietveld