Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(514)

Issue 29860555: Issue 6717 - Part 2: run qunit in headless firefox (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 4 months ago by tlucas
Modified:
1 year, 4 months ago
Reviewers:
hub, kzar, Sebastian Noack
Base URL:
https://hg.adblockplus.org/adblockpluschrome/file/3270e924ba9f
Visibility:
Public.

Description

Issue 6717 - Part 2: run qunit in headless firefox (Prio #2) Since this patch is my first JS-patch ever, please give as many hints on how to improve as possible. @ Hubert -> I raised the minimum Firefox Version to 57, in order to get this running with the webdriver. Maybe that's a solution for core too? (FWIW, the oldest supported Firefox version listed at https://adblockplus.org/en/requirements is v60.0, Although i don't know exactly if version-requirements differ for the ext and for core.)

Patch Set 1 #

Total comments: 6

Patch Set 2 : Removing test_runner.js #

Patch Set 3 : Fix type in README.md #

Total comments: 39

Patch Set 4 : #

Patch Set 5 : #

Total comments: 14

Patch Set 6 : #

Patch Set 7 : #

Total comments: 10

Patch Set 8 : #

Total comments: 2

Patch Set 9 : #

Total comments: 6

Patch Set 10 #

Total comments: 2

Patch Set 11 : #

Total comments: 4

Patch Set 12 : #

Patch Set 13 : No func change, reindent #

Unified diffs Side-by-side diffs Delta from patch set Stats (+131 lines, -4 lines) Patch
M README.md View 1 2 3 4 5 6 7 8 9 2 chunks +8 lines, -1 line 0 comments Download
M dependencies View 1 chunk +1 line, -1 line 0 comments Download
M package.json View 1 2 3 4 5 6 7 8 1 chunk +14 lines, -2 lines 0 comments Download
A test/firefox.js View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +108 lines, -0 lines 0 comments Download

Messages

Total messages: 26
tlucas
Patch Set 1 * introduce "npm test -- gecko" This patch involves a dependency update ...
1 year, 4 months ago (2018-08-21 11:09:04 UTC) #1
hub
If the tests fail, `npm test` doesn't return failure. https://codereview.adblockplus.org/29860555/diff/29860556/README.md File README.md (right): https://codereview.adblockplus.org/29860555/diff/29860556/README.md#newcode109 README.md:109: ...
1 year, 4 months ago (2018-08-21 13:23:21 UTC) #2
tlucas
Patch Set 2: * Removed test_runner.js * Restructured README.md / package.json On 2018/08/21 13:23:21, hub ...
1 year, 4 months ago (2018-08-22 07:01:31 UTC) #3
hub
Also since you bump the revisions from core, it is probably necessary to add the ...
1 year, 4 months ago (2018-08-22 16:13:56 UTC) #4
Sebastian Noack
https://codereview.adblockplus.org/29860555/diff/29861569/.eslintrc.json File .eslintrc.json (right): https://codereview.adblockplus.org/29860555/diff/29861569/.eslintrc.json#newcode14 .eslintrc.json:14: "__dirname": true On 2018/08/22 16:13:56, hub wrote: > also ...
1 year, 4 months ago (2018-08-22 18:16:39 UTC) #5
hub
https://codereview.adblockplus.org/29860555/diff/29861569/package.json File package.json (right): https://codereview.adblockplus.org/29860555/diff/29861569/package.json#newcode13 package.json:13: "nodeunit": "0.9.1", On 2018/08/22 18:16:38, Sebastian Noack wrote: > ...
1 year, 4 months ago (2018-08-22 19:21:08 UTC) #6
Sebastian Noack
https://codereview.adblockplus.org/29860555/diff/29861569/package.json File package.json (right): https://codereview.adblockplus.org/29860555/diff/29861569/package.json#newcode13 package.json:13: "nodeunit": "0.9.1", On 2018/08/22 19:21:08, hub wrote: > On ...
1 year, 4 months ago (2018-08-22 20:06:43 UTC) #7
tlucas
Patch Set 4: * Addressing Sebastian's comments (with further discussions) https://codereview.adblockplus.org/29860555/diff/29861569/.eslintrc.json File .eslintrc.json (right): https://codereview.adblockplus.org/29860555/diff/29861569/.eslintrc.json#newcode14 ...
1 year, 4 months ago (2018-08-22 20:24:02 UTC) #8
Sebastian Noack
https://codereview.adblockplus.org/29860555/diff/29861569/package.json File package.json (right): https://codereview.adblockplus.org/29860555/diff/29861569/package.json#newcode13 package.json:13: "nodeunit": "0.9.1", On 2018/08/22 20:24:00, tlucas wrote: > On ...
1 year, 4 months ago (2018-08-22 20:54:35 UTC) #9
tlucas
Patch Set 5: * Removed redundant { return x; } statements * Added a "comment" ...
1 year, 4 months ago (2018-08-23 08:47:28 UTC) #10
hub
https://codereview.adblockplus.org/29860555/diff/29862555/test/firefox.js File test/firefox.js (right): https://codereview.adblockplus.org/29860555/diff/29862555/test/firefox.js#newcode27 test/firefox.js:27: const Command = require("selenium-webdriver/lib/command").Command; you could use destructuring here ...
1 year, 4 months ago (2018-08-23 12:54:00 UTC) #11
tlucas
Patch Set 6: * Removed even more {} * Use template literals https://codereview.adblockplus.org/29860555/diff/29862555/test/firefox.js File test/firefox.js ...
1 year, 4 months ago (2018-08-23 13:42:15 UTC) #12
hub
https://codereview.adblockplus.org/29860555/diff/29862555/test/firefox.js File test/firefox.js (right): https://codereview.adblockplus.org/29860555/diff/29862555/test/firefox.js#newcode27 test/firefox.js:27: const Command = require("selenium-webdriver/lib/command").Command; On 2018/08/23 13:42:14, tlucas wrote: ...
1 year, 4 months ago (2018-08-23 14:46:33 UTC) #13
tlucas
Patch Set 7 * Reordered requires * Use even more destructuring https://codereview.adblockplus.org/29860555/diff/29862555/test/firefox.js File test/firefox.js (right): ...
1 year, 4 months ago (2018-08-23 15:32:05 UTC) #14
Sebastian Noack
https://codereview.adblockplus.org/29860555/diff/29861569/test/firefox.js File test/firefox.js (right): https://codereview.adblockplus.org/29860555/diff/29861569/test/firefox.js#newcode1 test/firefox.js:1: /* On 2018/08/23 08:47:27, tlucas wrote: > On 2018/08/22 ...
1 year, 4 months ago (2018-08-23 16:38:45 UTC) #15
tlucas
Patch Set 8 * reindentation https://codereview.adblockplus.org/29860555/diff/29862565/package.json File package.json (right): https://codereview.adblockplus.org/29860555/diff/29862565/package.json#newcode23 package.json:23: "test": "_run () { ...
1 year, 4 months ago (2018-08-23 18:56:02 UTC) #16
Sebastian Noack
https://codereview.adblockplus.org/29860555/diff/29862565/package.json File package.json (right): https://codereview.adblockplus.org/29860555/diff/29862565/package.json#newcode23 package.json:23: "test": "_run () { python build.py devenv -t $1; ...
1 year, 4 months ago (2018-08-23 21:54:22 UTC) #17
tlucas
Patch Set 8 * Restructured node scripts (remove parametrization) * Adding a fallback driver.quit() * ...
1 year, 4 months ago (2018-08-24 10:25:09 UTC) #18
Sebastian Noack
https://codereview.adblockplus.org/29860555/diff/29863555/README.md File README.md (right): https://codereview.adblockplus.org/29860555/diff/29863555/README.md#newcode96 README.md:96: npm test -- gecko This needs to be changed ...
1 year, 4 months ago (2018-08-24 12:29:11 UTC) #19
tlucas
Patch Set 10: * addressed Sebastian's comments https://codereview.adblockplus.org/29860555/diff/29863555/README.md File README.md (right): https://codereview.adblockplus.org/29860555/diff/29863555/README.md#newcode96 README.md:96: npm test ...
1 year, 4 months ago (2018-08-24 12:46:22 UTC) #20
Sebastian Noack
https://codereview.adblockplus.org/29860555/diff/29863562/test/firefox.js File test/firefox.js (right): https://codereview.adblockplus.org/29860555/diff/29863562/test/firefox.js#newcode34 test/firefox.js:34: By.css(`#qunit-tests ${success ? ".pass" : ".fail"} .test-name`)). Nit: (Same ...
1 year, 4 months ago (2018-08-24 12:54:43 UTC) #21
tlucas
https://codereview.adblockplus.org/29860555/diff/29863562/test/firefox.js File test/firefox.js (right): https://codereview.adblockplus.org/29860555/diff/29863562/test/firefox.js#newcode34 test/firefox.js:34: By.css(`#qunit-tests ${success ? ".pass" : ".fail"} .test-name`)). On 2018/08/24 ...
1 year, 4 months ago (2018-08-24 13:10:14 UTC) #22
Sebastian Noack
https://codereview.adblockplus.org/29860555/diff/29863569/test/firefox.js File test/firefox.js (right): https://codereview.adblockplus.org/29860555/diff/29863569/test/firefox.js#newcode29 test/firefox.js:29: Nit: We usually only put one blank line after ...
1 year, 4 months ago (2018-08-24 13:28:00 UTC) #23
tlucas
https://codereview.adblockplus.org/29860555/diff/29863569/test/firefox.js File test/firefox.js (right): https://codereview.adblockplus.org/29860555/diff/29863569/test/firefox.js#newcode29 test/firefox.js:29: On 2018/08/24 13:27:59, Sebastian Noack wrote: > Nit: We ...
1 year, 4 months ago (2018-08-24 15:26:01 UTC) #24
Sebastian Noack
LGTM. I'm still not sure about the directory structure test/firefox.js, as this is more a ...
1 year, 4 months ago (2018-08-24 18:37:10 UTC) #25
hub
1 year, 4 months ago (2018-08-24 21:03:03 UTC) #26
On 2018/08/24 18:37:10, Sebastian Noack wrote:
> LGTM.
> 
> I'm still not sure about the directory structure test/firefox.js, as this is
> more a test runner than a test suite, and looking at adblockpluscore, a single
> file on the top-level seems to make more sense. But before we add more stuff
> it's hard to say what architecture we will eventually end up with here. So
let's
> revisit this later.

The thing is that Tristan approach would be a way address issue 6822 in
adblockpluscore (were the failure in browser tests are ignored). I was actually
thinking going that route.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5