Issue 29860555: Issue 6717 - Part 2: run qunit in headless firefox (Closed)
|
Created:
Aug. 21, 2018, 10:57 a.m. by tlucas Modified:
Aug. 26, 2018, 9:12 p.m. Base URL:
https://hg.adblockplus.org/adblockpluschrome/file/3270e924ba9f Visibility:
Public. |
DescriptionIssue 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 #
MessagesTotal messages: 26
Patch Set 1 * introduce "npm test -- gecko" This patch involves a dependency update for adblockpluscore to https://hg.adblockplus.org/adblockpluscore/rev/4a504a8ac939, in order to use core's implementation of download a firefox executable (I may adjust the ticket accordingly) This is **by no means** final, but i want to get as much feedback as early as possible, since this is my first JS patch (and furthermore since this is a Prio implementation.) Looking forward to your comments! Cheers, Tristan
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: by running I think this should be merged with the paragraph `Running the unit tests` https://codereview.adblockplus.org/29860555/diff/29860556/package.json File package.json (right): https://codereview.adblockplus.org/29860555/diff/29860556/package.json#newcode12 package.json:12: "extract-zip": "1.6.5", we tend to keep the modules in alphabetical order. https://codereview.adblockplus.org/29860555/diff/29860556/package.json#newcode13 package.json:13: "selenium-webdriver": "^3.6.0" it is missing "dmg" in that list. (it is need for downloading on macOS) ` "dmg": "0.1.0",`
Patch Set 2: * Removed test_runner.js * Restructured README.md / package.json On 2018/08/21 13:23:21, hub wrote: > If the tests fail, `npm test` doesn't return failure. You are right. Looking at this made test_runner.js appear obsolete, so i removed it - now it fails as expected. 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: by running On 2018/08/21 13:23:21, hub wrote: > I think this should be merged with the paragraph `Running the unit tests` Good point - done. https://codereview.adblockplus.org/29860555/diff/29860556/package.json File package.json (right): https://codereview.adblockplus.org/29860555/diff/29860556/package.json#newcode12 package.json:12: "extract-zip": "1.6.5", On 2018/08/21 13:23:21, hub wrote: > we tend to keep the modules in alphabetical order. Done. https://codereview.adblockplus.org/29860555/diff/29860556/package.json#newcode13 package.json:13: "selenium-webdriver": "^3.6.0" On 2018/08/21 13:23:21, hub wrote: > it is missing "dmg" in that list. (it is need for downloading on macOS) > > ` "dmg": "0.1.0",` Done.
Also since you bump the revisions from core, it is probably necessary to add the list of changes in the issue. Or maybe do the dependency bump prior to that bug with a proper blocking dependency. https://codereview.adblockplus.org/29860555/diff/29861569/.eslintrc.json File .eslintrc.json (right): https://codereview.adblockplus.org/29860555/diff/29861569/.eslintrc.json#newc... .eslintrc.json:14: "__dirname": true also I'm wondering if this shouldn't go into a sub configuration file in test/ like we have for adblockpluscore, since these are exception we make for the tests.
https://codereview.adblockplus.org/29860555/diff/29861569/.eslintrc.json File .eslintrc.json (right): https://codereview.adblockplus.org/29860555/diff/29861569/.eslintrc.json#newc... .eslintrc.json:14: "__dirname": true On 2018/08/22 16:13:56, hub wrote: > also I'm wondering if this shouldn't go into a sub configuration file in test/ > like we have for adblockpluscore, since these are exception we make for the > tests. Well, rather than whitelisting particular globals, you should rather set eslint-env to node. Since there is only a single file you might want to do it inline just for that file, which is exactly what you did in adblockpluscore. 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", Any reason you pin exact versions for dmg, extract-zip, ncp and nodeunit? Can you please also explain why each of these packages are required? The only new module I see you import is "selenium-webdriver", and "nodeunit" is invoked below, I guess "geckodriver" is a peer dependency, but it doesn't occur to me why any of the other packages needs to be made an explicit dependency. 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#new... test/firefox.js:1: /* Why not calling it test_runner.js and put it under the top level, and later put stuff for the browsers in here as well, just like you did in adblockpluscore? Also, I wonder whether there is any code we could potentially share between test_runner.js in adblockpluscore and this script (maybe with some abstraction maybe)? Note that I didn't compare the code in both test runners yet, so it might as well not be worth it. I just want to put it up for discussion. https://codereview.adblockplus.org/29860555/diff/29861569/test/firefox.js#new... test/firefox.js:23: const By = webdriver.By; Nit: Use desctrucuring. https://codereview.adblockplus.org/29860555/diff/29861569/test/firefox.js#new... test/firefox.js:24: const until = webdriver.until; Nit: This seems to be unused. https://codereview.adblockplus.org/29860555/diff/29861569/test/firefox.js#new... test/firefox.js:34: test.expect(14); Where is this number coming from? https://codereview.adblockplus.org/29860555/diff/29861569/test/firefox.js#new... test/firefox.js:49: // Spawn the Firefox process in headless mode Nit: This comment seems redundant to me. https://codereview.adblockplus.org/29860555/diff/29861569/test/firefox.js#new... test/firefox.js:82: driver.executeScript("location.href = \"qunit/index.html\";"); Wouldn't driver.navigate().to() be more appropriate? https://codereview.adblockplus.org/29860555/diff/29861569/test/firefox.js#new... test/firefox.js:92: return driver.findElement(By.id("qunit-testresult")) Nit: return + braces are redundant. https://codereview.adblockplus.org/29860555/diff/29861569/test/firefox.js#new... test/firefox.js:104: elements.forEach(elem => Nit: We prefer for..of loops over forEach(). https://codereview.adblockplus.org/29860555/diff/29861569/test/firefox.js#new... test/firefox.js:116: elements.forEach(elem => Nit: See above.
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: > Any reason you pin exact versions for dmg, extract-zip, ncp and nodeunit? > > Can you please also explain why each of these packages are required? The only > new module I see you import is "selenium-webdriver", and "nodeunit" is invoked > below, I guess "geckodriver" is a peer dependency, but it doesn't occur to me > why any of the other packages needs to be made an explicit dependency. For "dmg" it is the exact same as in adblockpluscore. "dmg" is needed to unpack Firefox on macOS. I'm sure we can use ^0.1.0 instead. (see firefox_download.js in adblockpluscore).
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 2018/08/22 18:16:38, Sebastian Noack wrote: > > Any reason you pin exact versions for dmg, extract-zip, ncp and nodeunit? > > > > Can you please also explain why each of these packages are required? The only > > new module I see you import is "selenium-webdriver", and "nodeunit" is invoked > > below, I guess "geckodriver" is a peer dependency, but it doesn't occur to me > > why any of the other packages needs to be made an explicit dependency. > > For "dmg" it is the exact same as in adblockpluscore. > "dmg" is needed to unpack Firefox on macOS. I'm sure we can use ^0.1.0 instead. > (see firefox_download.js in adblockpluscore). Acknowledged, but what is about the other dependencies?
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#newc... .eslintrc.json:14: "__dirname": true On 2018/08/22 18:16:38, Sebastian Noack wrote: > On 2018/08/22 16:13:56, hub wrote: > > also I'm wondering if this shouldn't go into a sub configuration file in test/ > > like we have for adblockpluscore, since these are exception we make for the > > tests. > > Well, rather than whitelisting particular globals, you should rather set > eslint-env to node. Since there is only a single file you might want to do it > inline just for that file, which is exactly what you did in adblockpluscore. As a matter of fact, this was previously only necessary for the test_runner.js - which is now removed. Undid these lines accordingly. 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: > Any reason you pin exact versions for dmg, extract-zip, ncp and nodeunit? > > Can you please also explain why each of these packages are required? The only > new module I see you import is "selenium-webdriver", and "nodeunit" is invoked > below, I guess "geckodriver" is a peer dependency, but it doesn't occur to me > why any of the other packages needs to be made an explicit dependency. No real reason, besides following Hubert's suggestion (and the package.json from adblockpluscore pins to the same version). If requested, i can switch to "gte". The other packages, i.e. "ncp" and "extract-zip" are required for adblockpluscore/test/download_firefox.js. This script is executed once, when no Firefox is present on the executing machine (invoked by test/firefox.js:50). Since those packages are only devDependencies for adblockpluscore and ensure_dependencies.py doesn't install those, setting the NODE_PATH is unfortunately not an option. 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#new... test/firefox.js:1: /* On 2018/08/22 18:16:38, Sebastian Noack wrote: > Why not calling it test_runner.js and put it under the top level, and later put > stuff for the browsers in here as well, just like you did in adblockpluscore? > > Also, I wonder whether there is any code we could potentially share between > test_runner.js in adblockpluscore and this script (maybe with some abstraction > maybe)? Note that I didn't compare the code in both test runners yet, so it > might as well not be worth it. I just want to put it up for discussion. From what i saw, there's virtually no code from adblockpluscore/test_runner.js that could be reused, except nodeunit.reporters.default.run(files); and the generation of the "files"-array from the command line arguments. That's now all together done by "nodeunit test/*" from a script in package.json. I wouldn't mind adding an abstraction layer for future browsers thought, if anyone insists. https://codereview.adblockplus.org/29860555/diff/29861569/test/firefox.js#new... test/firefox.js:23: const By = webdriver.By; On 2018/08/22 18:16:38, Sebastian Noack wrote: > Nit: Use desctrucuring. I assume you expected something like (with the comment below in mind) const webdriver = require("selenium-webdriver"); const {By:By, until:until} = webdriver; Or am i mislead by google? https://codereview.adblockplus.org/29860555/diff/29861569/test/firefox.js#new... test/firefox.js:24: const until = webdriver.until; On 2018/08/22 18:16:38, Sebastian Noack wrote: > Nit: This seems to be unused. It's used in line 86; ... driver.wait(until. ... https://codereview.adblockplus.org/29860555/diff/29861569/test/firefox.js#new... test/firefox.js:34: test.expect(14); On 2018/08/22 18:16:38, Sebastian Noack wrote: > Where is this number coming from? It's the number of test-groups for qunit, when run inside Firefox (i.e. the number of assertions you get from this script in total, from the lines 108/120), e.g.: firefox.js ✔ runFirefox OK: 14 assertions (17553ms) https://codereview.adblockplus.org/29860555/diff/29861569/test/firefox.js#new... test/firefox.js:49: // Spawn the Firefox process in headless mode On 2018/08/22 18:16:38, Sebastian Noack wrote: > Nit: This comment seems redundant to me. Done. https://codereview.adblockplus.org/29860555/diff/29861569/test/firefox.js#new... test/firefox.js:82: driver.executeScript("location.href = \"qunit/index.html\";"); On 2018/08/22 18:16:38, Sebastian Noack wrote: > Wouldn't driver.navigate().to() be more appropriate? driver.navigate().to() expects absolute URLs, so we'd have to figure that out first - whereas the script above also accepts relative urls. https://codereview.adblockplus.org/29860555/diff/29861569/test/firefox.js#new... test/firefox.js:92: return driver.findElement(By.id("qunit-testresult")) On 2018/08/22 18:16:38, Sebastian Noack wrote: > Nit: return + braces are redundant. Could you help me out here, what should it look like? https://codereview.adblockplus.org/29860555/diff/29861569/test/firefox.js#new... test/firefox.js:104: elements.forEach(elem => On 2018/08/22 18:16:38, Sebastian Noack wrote: > Nit: We prefer for..of loops over forEach(). Done. https://codereview.adblockplus.org/29860555/diff/29861569/test/firefox.js#new... test/firefox.js:116: elements.forEach(elem => On 2018/08/22 18:16:38, Sebastian Noack wrote: > Nit: See above. Done.
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 2018/08/22 18:16:38, Sebastian Noack wrote: > > Any reason you pin exact versions for dmg, extract-zip, ncp and nodeunit? > > > > Can you please also explain why each of these packages are required? The only > > new module I see you import is "selenium-webdriver", and "nodeunit" is invoked > > below, I guess "geckodriver" is a peer dependency, but it doesn't occur to me > > why any of the other packages needs to be made an explicit dependency. > > No real reason, besides following Hubert's suggestion (and the package.json from > adblockpluscore pins to the same version). If requested, i can switch to "gte". > > The other packages, i.e. "ncp" and "extract-zip" are required for > adblockpluscore/test/download_firefox.js. This script is executed once, when no > Firefox is present on the executing machine (invoked by test/firefox.js:50). > Since those packages are only devDependencies for adblockpluscore and > ensure_dependencies.py doesn't install those, setting the NODE_PATH is > unfortunately not an option. The canonical way to refer to a dependency's version in package.json is to use ^ (meaning latest compatible version), unless there is an actual requirement to pin an old version. If there is none, we should rather update package.json in adblockpluscore as well than further spreading this arbitrary pattern. Can we have comments in here? If so we also might want to indicate where those non-obvious dependencies come from. 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#new... test/firefox.js:1: /* On 2018/08/22 20:24:01, tlucas wrote: > On 2018/08/22 18:16:38, Sebastian Noack wrote: > > Why not calling it test_runner.js and put it under the top level, and later > put > > stuff for the browsers in here as well, just like you did in adblockpluscore? > > > > Also, I wonder whether there is any code we could potentially share between > > test_runner.js in adblockpluscore and this script (maybe with some abstraction > > maybe)? Note that I didn't compare the code in both test runners yet, so it > > might as well not be worth it. I just want to put it up for discussion. > > From what i saw, there's virtually no code from adblockpluscore/test_runner.js > that could be reused, except > > nodeunit.reporters.default.run(files); > > and the generation of the "files"-array from the command line arguments. > > That's now all together done by "nodeunit test/*" from a script in package.json. > I wouldn't mind adding an abstraction layer for future browsers thought, if > anyone insists. Fair enough. Still what is the advantage of the directory structure test/<browser> over a single script in the top-level? It seems even when we add support for Chrome, it's mostly the same code we want to run. https://codereview.adblockplus.org/29860555/diff/29861569/test/firefox.js#new... test/firefox.js:23: const By = webdriver.By; On 2018/08/22 20:24:02, tlucas wrote: > On 2018/08/22 18:16:38, Sebastian Noack wrote: > > Nit: Use desctrucuring. > > I assume you expected something like (with the comment below in mind) > > const webdriver = require("selenium-webdriver"); > const {By:By, until:until} = webdriver; > > Or am i mislead by google? Better: const {By, until} = webdriver; https://codereview.adblockplus.org/29860555/diff/29861569/test/firefox.js#new... test/firefox.js:24: const until = webdriver.until; On 2018/08/22 20:24:01, tlucas wrote: > On 2018/08/22 18:16:38, Sebastian Noack wrote: > > Nit: This seems to be unused. > > It's used in line 86; > > ... > driver.wait(until. ... My bad. https://codereview.adblockplus.org/29860555/diff/29861569/test/firefox.js#new... test/firefox.js:34: test.expect(14); On 2018/08/22 20:24:01, tlucas wrote: > On 2018/08/22 18:16:38, Sebastian Noack wrote: > > Where is this number coming from? > > It's the number of test-groups for qunit, when run inside Firefox (i.e. the > number of assertions you get from this script in total, from the lines 108/120), > e.g.: > > > firefox.js > ✔ runFirefox > > OK: 14 assertions (17553ms) So that means whenever the number of qunit tests change we have to change the hard-coded number here as well? That sounds quite unpractical. Can we figure out how many tests there are and the call expect() asynchronously? What if we not call expect() at all? Any other ideas? https://codereview.adblockplus.org/29860555/diff/29861569/test/firefox.js#new... test/firefox.js:82: driver.executeScript("location.href = \"qunit/index.html\";"); On 2018/08/22 20:24:02, tlucas wrote: > On 2018/08/22 18:16:38, Sebastian Noack wrote: > > Wouldn't driver.navigate().to() be more appropriate? > > driver.navigate().to() expects absolute URLs, so we'd have to figure that out > first - whereas the script above also accepts relative urls. Acknowledged. https://codereview.adblockplus.org/29860555/diff/29861569/test/firefox.js#new... test/firefox.js:92: return driver.findElement(By.id("qunit-testresult")) On 2018/08/22 20:24:01, tlucas wrote: > On 2018/08/22 18:16:38, Sebastian Noack wrote: > > Nit: return + braces are redundant. > > Could you help me out here, what should it look like? Sorry, I thought for some reason Hubert was the author. The following are equivalent: () => x () => { return x; }
Patch Set 5:
* Removed redundant { return x; } statements
* Added a "comment" to package.json, explaining extra packages
* removed test.expect()
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:54:34, Sebastian Noack wrote:
> On 2018/08/22 20:24:00, tlucas wrote:
> > On 2018/08/22 18:16:38, Sebastian Noack wrote:
> > > Any reason you pin exact versions for dmg, extract-zip, ncp and nodeunit?
> > >
> > > Can you please also explain why each of these packages are required? The
> only
> > > new module I see you import is "selenium-webdriver", and "nodeunit" is
> invoked
> > > below, I guess "geckodriver" is a peer dependency, but it doesn't occur to
> me
> > > why any of the other packages needs to be made an explicit dependency.
> >
> > No real reason, besides following Hubert's suggestion (and the package.json
> from
> > adblockpluscore pins to the same version). If requested, i can switch to
> "gte".
> >
> > The other packages, i.e. "ncp" and "extract-zip" are required for
> > adblockpluscore/test/download_firefox.js. This script is executed once, when
> no
> > Firefox is present on the executing machine (invoked by test/firefox.js:50).
> > Since those packages are only devDependencies for adblockpluscore and
> > ensure_dependencies.py doesn't install those, setting the NODE_PATH is
> > unfortunately not an option.
>
> The canonical way to refer to a dependency's version in package.json is to use
^
> (meaning latest compatible version), unless there is an actual requirement to
> pin an old version. If there is none, we should rather update package.json in
> adblockpluscore as well than further spreading this arbitrary pattern.
>
> Can we have comments in here? If so we also might want to indicate where those
> non-obvious dependencies come from.
Adjusted the pinnings.
I also found a way to add comment-ish data, please let me know if that's ok.
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#new...
test/firefox.js:1: /*
On 2018/08/22 20:54:35, Sebastian Noack wrote:
> On 2018/08/22 20:24:01, tlucas wrote:
> > On 2018/08/22 18:16:38, Sebastian Noack wrote:
> > > Why not calling it test_runner.js and put it under the top level, and
later
> > put
> > > stuff for the browsers in here as well, just like you did in
> adblockpluscore?
> > >
> > > Also, I wonder whether there is any code we could potentially share
between
> > > test_runner.js in adblockpluscore and this script (maybe with some
> abstraction
> > > maybe)? Note that I didn't compare the code in both test runners yet, so
it
> > > might as well not be worth it. I just want to put it up for discussion.
> >
> > From what i saw, there's virtually no code from
adblockpluscore/test_runner.js
> > that could be reused, except
> >
> > nodeunit.reporters.default.run(files);
> >
> > and the generation of the "files"-array from the command line arguments.
> >
> > That's now all together done by "nodeunit test/*" from a script in
> package.json.
> > I wouldn't mind adding an abstraction layer for future browsers thought, if
> > anyone insists.
>
> Fair enough. Still what is the advantage of the directory structure
> test/<browser> over a single script in the top-level? It seems even when we
add
> support for Chrome, it's mostly the same code we want to run.
With the current implementation of the packagers, a .js file on the top-level
would be added to builds. A not explicitly listed folder is skipped (and that's
what we want).
https://codereview.adblockplus.org/29860555/diff/29861569/test/firefox.js#new...
test/firefox.js:23: const By = webdriver.By;
On 2018/08/22 20:54:35, Sebastian Noack wrote:
> On 2018/08/22 20:24:02, tlucas wrote:
> > On 2018/08/22 18:16:38, Sebastian Noack wrote:
> > > Nit: Use desctrucuring.
> >
> > I assume you expected something like (with the comment below in mind)
> >
> > const webdriver = require("selenium-webdriver");
> > const {By:By, until:until} = webdriver;
> >
> > Or am i mislead by google?
>
> Better:
>
> const {By, until} = webdriver;
Done.
https://codereview.adblockplus.org/29860555/diff/29861569/test/firefox.js#new...
test/firefox.js:34: test.expect(14);
On 2018/08/22 20:54:35, Sebastian Noack wrote:
> On 2018/08/22 20:24:01, tlucas wrote:
> > On 2018/08/22 18:16:38, Sebastian Noack wrote:
> > > Where is this number coming from?
> >
> > It's the number of test-groups for qunit, when run inside Firefox (i.e. the
> > number of assertions you get from this script in total, from the lines
> 108/120),
> > e.g.:
> >
> >
> > firefox.js
> > ✔ runFirefox
> >
> > OK: 14 assertions (17553ms)
>
> So that means whenever the number of qunit tests change we have to change the
> hard-coded number here as well? That sounds quite unpractical. Can we figure
out
> how many tests there are and the call expect() asynchronously? What if we not
> call expect() at all? Any other ideas?
The only drawback of not using expect() is, we would (in this script) have no
idea if all of the test have been run - but i guess qunit is taking care of
that. Removing it won't have any other side effect - so let's just remove it.
https://codereview.adblockplus.org/29860555/diff/29861569/test/firefox.js#new...
test/firefox.js:92: return driver.findElement(By.id("qunit-testresult"))
On 2018/08/22 20:54:35, Sebastian Noack wrote:
> On 2018/08/22 20:24:01, tlucas wrote:
> > On 2018/08/22 18:16:38, Sebastian Noack wrote:
> > > Nit: return + braces are redundant.
> >
> > Could you help me out here, what should it look like?
>
> Sorry, I thought for some reason Hubert was the author. The following are
> equivalent:
>
> () => x
> () => { return x; }
Done. (also for the braces / return around "Wait for the firstrun-page to be
loaded")
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#new... test/firefox.js:27: const Command = require("selenium-webdriver/lib/command").Command; you could use destructuring here too. https://codereview.adblockplus.org/29860555/diff/29862555/test/firefox.js#new... test/firefox.js:44: return driver.schedule(cmd, "installWebExt(" + extension + ")"); I'd rather use template a string here: `installWebExt(${extension})` https://codereview.adblockplus.org/29860555/diff/29862555/test/firefox.js#new... test/firefox.js:77: driver.executeScript("location.href = \"qunit/index.html\";"); you could go `() => driver.executeScript(...)` (dropping the curly braces) https://codereview.adblockplus.org/29860555/diff/29862555/test/firefox.js#new... test/firefox.js:81: driver.wait(until.elementLocated(By.id("qunit-testresult"))); here too. https://codereview.adblockplus.org/29860555/diff/29862555/test/firefox.js#new... test/firefox.js:85: driver.wait(() => and here https://codereview.adblockplus.org/29860555/diff/29862555/test/firefox.js#new... test/firefox.js:111: test.ok(false, "Undefined error in " + data); And a template string here too.
Patch Set 6:
* Removed even more {}
* Use template literals
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#new...
test/firefox.js:27: const Command =
require("selenium-webdriver/lib/command").Command;
On 2018/08/23 12:53:59, hub wrote:
> you could use destructuring here too.
Could you please explain which lines / how exactly?
https://codereview.adblockplus.org/29860555/diff/29862555/test/firefox.js#new...
test/firefox.js:44: return driver.schedule(cmd, "installWebExt(" + extension +
")");
On 2018/08/23 12:54:00, hub wrote:
> I'd rather use template a string here:
> `installWebExt(${extension})`
Done.
https://codereview.adblockplus.org/29860555/diff/29862555/test/firefox.js#new...
test/firefox.js:77: driver.executeScript("location.href =
\"qunit/index.html\";");
On 2018/08/23 12:54:00, hub wrote:
> you could go `() => driver.executeScript(...)` (dropping the curly braces)
Done.
https://codereview.adblockplus.org/29860555/diff/29862555/test/firefox.js#new...
test/firefox.js:81:
driver.wait(until.elementLocated(By.id("qunit-testresult")));
On 2018/08/23 12:53:59, hub wrote:
> here too.
Done.
https://codereview.adblockplus.org/29860555/diff/29862555/test/firefox.js#new...
test/firefox.js:85: driver.wait(() =>
On 2018/08/23 12:53:59, hub wrote:
> and here
Done.
https://codereview.adblockplus.org/29860555/diff/29862555/test/firefox.js#new...
test/firefox.js:111: test.ok(false, "Undefined error in " + data);
On 2018/08/23 12:54:00, hub wrote:
> And a template string here too.
Done.
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#new... test/firefox.js:27: const Command = require("selenium-webdriver/lib/command").Command; On 2018/08/23 13:42:14, tlucas wrote: > On 2018/08/23 12:53:59, hub wrote: > > you could use destructuring here too. > > Could you please explain which lines / how exactly? change const Command = require("selenium-webdriver/lib/command").Command; to const {Command} = require("selenium-webdriver/lib/command"); `npm run lint` should have caught this.
Patch Set 7 * Reordered requires * Use even more destructuring 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#new... test/firefox.js:27: const Command = require("selenium-webdriver/lib/command").Command; On 2018/08/23 14:46:33, hub wrote: > On 2018/08/23 13:42:14, tlucas wrote: > > On 2018/08/23 12:53:59, hub wrote: > > > you could use destructuring here too. > > > > Could you please explain which lines / how exactly? > > change > > const Command = require("selenium-webdriver/lib/command").Command; > > to > > const {Command} = require("selenium-webdriver/lib/command"); > > `npm run lint` should have caught this. Thank you! And no, "npm run lint" did not complain about this. Done.
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#new... test/firefox.js:1: /* On 2018/08/23 08:47:27, tlucas wrote: > On 2018/08/22 20:54:35, Sebastian Noack wrote: > > On 2018/08/22 20:24:01, tlucas wrote: > > > On 2018/08/22 18:16:38, Sebastian Noack wrote: > > > > Why not calling it test_runner.js and put it under the top level, and > later > > > put > > > > stuff for the browsers in here as well, just like you did in > > adblockpluscore? > > > > > > > > Also, I wonder whether there is any code we could potentially share > between > > > > test_runner.js in adblockpluscore and this script (maybe with some > > abstraction > > > > maybe)? Note that I didn't compare the code in both test runners yet, so > it > > > > might as well not be worth it. I just want to put it up for discussion. > > > > > > From what i saw, there's virtually no code from > adblockpluscore/test_runner.js > > > that could be reused, except > > > > > > nodeunit.reporters.default.run(files); > > > > > > and the generation of the "files"-array from the command line arguments. > > > > > > That's now all together done by "nodeunit test/*" from a script in > > package.json. > > > I wouldn't mind adding an abstraction layer for future browsers thought, if > > > anyone insists. > > > > Fair enough. Still what is the advantage of the directory structure > > test/<browser> over a single script in the top-level? It seems even when we > add > > support for Chrome, it's mostly the same code we want to run. > > With the current implementation of the packagers, a .js file on the top-level > would be added to builds. A not explicitly listed folder is skipped (and that's > what we want). Well changing buildtools to not include test_runner.js in the builds would be trivial. The question rather is how will the test runner code look like once we add more stuff (e.g. other browsers). Is there a point in splitting up things into different test scripts? It seems adblockpluscore gets away with a combined test runner for Chrome and Firefox. 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; npm run run_tests ; } ; _run", I'm wondering why do we need to define a sheel script function just to call it right after? Why can't we inline it? Also why make the target for build.py configurable? The tests will run on Firefox regardless. So passing in anything other than "gecko" seems pointless. https://codereview.adblockplus.org/29860555/diff/29862565/test/firefox.js File test/firefox.js (right): https://codereview.adblockplus.org/29860555/diff/29862565/test/firefox.js#new... test/firefox.js:23: Nit: Unlike in Python, we generally don't group imports by corelib/third-party/first-party. https://codereview.adblockplus.org/29860555/diff/29862565/test/firefox.js#new... test/firefox.js:55: .setBinary(binary); Nit: The indentation seems a little weird here (and below). It should be two spaces like above in installWebExt().
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 () { python build.py devenv -t $1; npm run run_tests ; } ; _run", On 2018/08/23 16:38:44, Sebastian Noack wrote: > I'm wondering why do we need to define a sheel script function just to call it > right after? Why can't we inline it? > > Also why make the target for build.py configurable? The tests will run on > Firefox regardless. So passing in anything other than "gecko" seems pointless. Making it configurable was the reason to do this, yes - the argument ("gecko" in our case) would have been passed in some weird way to "npm run run_tests" instead. For "why configurable" -> we might consider this premature optimization, but already given that Edge (and likely Chrome too) will be build on a different machine (and in parallel), this is meant to "build" and "test" for whatever platform is handled by the current CI - job. Then again, this strongly supports adding an abstract test_runner.js right from the start. Please tell me what you think. https://codereview.adblockplus.org/29860555/diff/29862565/test/firefox.js File test/firefox.js (right): https://codereview.adblockplus.org/29860555/diff/29862565/test/firefox.js#new... test/firefox.js:23: On 2018/08/23 16:38:44, Sebastian Noack wrote: > Nit: Unlike in Python, we generally don't group imports by > corelib/third-party/first-party. So no grouping at all? Or an arbitrary grouping? https://codereview.adblockplus.org/29860555/diff/29862565/test/firefox.js#new... test/firefox.js:55: .setBinary(binary); On 2018/08/23 16:38:44, Sebastian Noack wrote: > Nit: The indentation seems a little weird here (and below). It should be two > spaces like above in installWebExt(). Done.
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; npm run run_tests ; } ; _run", On 2018/08/23 18:56:02, tlucas wrote: > On 2018/08/23 16:38:44, Sebastian Noack wrote: > > I'm wondering why do we need to define a sheel script function just to call it > > right after? Why can't we inline it? > > > > Also why make the target for build.py configurable? The tests will run on > > Firefox regardless. So passing in anything other than "gecko" seems pointless. > > Making it configurable was the reason to do this, yes - the argument ("gecko" in > our case) would have been passed in some weird way to "npm run run_tests" > instead. > > For "why configurable" -> we might consider this premature optimization, but > already given that Edge (and likely Chrome too) will be build on a different > machine (and in parallel), this is meant to "build" and "test" for whatever > platform is handled by the current CI - job. Then again, this strongly supports > adding an abstract test_runner.js right from the start. > > Please tell me what you think. It seems we have to touch the CI configuration again anyway, once support for other browsers have been added, since if the CI (or yourself) would already try to run "npm test -- chrome" it will potentially explode, since with the current implementation this would essentially run a build for Chrome on Firefox. So IMO we could as well just hard-code gecko here for the time being, making sure that always the correct devenv is built for the only browser supported so far. https://codereview.adblockplus.org/29860555/diff/29862565/test/firefox.js File test/firefox.js (right): https://codereview.adblockplus.org/29860555/diff/29862565/test/firefox.js#new... test/firefox.js:23: On 2018/08/23 18:56:02, tlucas wrote: > On 2018/08/23 16:38:44, Sebastian Noack wrote: > > Nit: Unlike in Python, we generally don't group imports by > > corelib/third-party/first-party. > > So no grouping at all? Or an arbitrary grouping? Just remove the blank line here and in line 28. https://codereview.adblockplus.org/29860555/diff/29862575/test/firefox.js File test/firefox.js (right): https://codereview.adblockplus.org/29860555/diff/29862575/test/firefox.js#new... test/firefox.js:111: driver.quit(); This looks like a potential race condition. You want to call driver.quit() and test.done() once the above actions completed (asynchronously), don't you? So it seems what you want to do is something like this: .then(() => Promise.all([ driver.findElements(...).then(elements => Promise.all(elements.map(...)) ), driver.findElements(...).then(elements => Promise.all(elements.map(...)) ) ])).then(() => { driver.quit(); test.done(); });
Patch Set 8 * Restructured node scripts (remove parametrization) * Adding a fallback driver.quit() * Made passed/failed tests finding dryer (and removed the potential race condition) * Replied to several other comments 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#new... test/firefox.js:1: /* On 2018/08/23 16:38:44, Sebastian Noack wrote: > On 2018/08/23 08:47:27, tlucas wrote: > > On 2018/08/22 20:54:35, Sebastian Noack wrote: > > > On 2018/08/22 20:24:01, tlucas wrote: > > > > On 2018/08/22 18:16:38, Sebastian Noack wrote: > > > > > Why not calling it test_runner.js and put it under the top level, and > > later > > > > put > > > > > stuff for the browsers in here as well, just like you did in > > > adblockpluscore? > > > > > > > > > > Also, I wonder whether there is any code we could potentially share > > between > > > > > test_runner.js in adblockpluscore and this script (maybe with some > > > abstraction > > > > > maybe)? Note that I didn't compare the code in both test runners yet, so > > it > > > > > might as well not be worth it. I just want to put it up for discussion. > > > > > > > > From what i saw, there's virtually no code from > > adblockpluscore/test_runner.js > > > > that could be reused, except > > > > > > > > nodeunit.reporters.default.run(files); > > > > > > > > and the generation of the "files"-array from the command line arguments. > > > > > > > > That's now all together done by "nodeunit test/*" from a script in > > > package.json. > > > > I wouldn't mind adding an abstraction layer for future browsers thought, > if > > > > anyone insists. > > > > > > Fair enough. Still what is the advantage of the directory structure > > > test/<browser> over a single script in the top-level? It seems even when we > > add > > > support for Chrome, it's mostly the same code we want to run. > > > > With the current implementation of the packagers, a .js file on the top-level > > would be added to builds. A not explicitly listed folder is skipped (and > that's > > what we want). > > Well changing buildtools to not include test_runner.js in the builds would be > trivial. The question rather is how will the test runner code look like once we > add more stuff (e.g. other browsers). Is there a point in splitting up things > into different test scripts? It seems adblockpluscore gets away with a combined > test runner for Chrome and Firefox. For the buildtools, same argument as somewhere else in this mail, i'd rather not initiate a buildtools change / dependency update. For the future, we'll also get away with an abstraction (and maybe even without a test_runner), i.e. outsourcing every "controlling" functionality from firefox.js to something like _common.js and leaving the setups to firefox.js / chrome.js / edge.js. 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; npm run run_tests ; } ; _run", On 2018/08/23 21:54:20, Sebastian Noack wrote: > On 2018/08/23 18:56:02, tlucas wrote: > > On 2018/08/23 16:38:44, Sebastian Noack wrote: > > > I'm wondering why do we need to define a sheel script function just to call > it > > > right after? Why can't we inline it? > > > > > > Also why make the target for build.py configurable? The tests will run on > > > Firefox regardless. So passing in anything other than "gecko" seems > pointless. > > > > Making it configurable was the reason to do this, yes - the argument ("gecko" > in > > our case) would have been passed in some weird way to "npm run run_tests" > > instead. > > > > For "why configurable" -> we might consider this premature optimization, but > > already given that Edge (and likely Chrome too) will be build on a different > > machine (and in parallel), this is meant to "build" and "test" for whatever > > platform is handled by the current CI - job. Then again, this strongly > supports > > adding an abstract test_runner.js right from the start. > > > > Please tell me what you think. > > It seems we have to touch the CI configuration again anyway, once support for > other browsers have been added, since if the CI (or yourself) would already try > to run "npm test -- chrome" it will potentially explode, since with the current > implementation this would essentially run a build for Chrome on Firefox. So IMO > we could as well just hard-code gecko here for the time being, making sure that > always the correct devenv is built for the only browser supported so far. Done. https://codereview.adblockplus.org/29860555/diff/29862565/test/firefox.js File test/firefox.js (right): https://codereview.adblockplus.org/29860555/diff/29862565/test/firefox.js#new... test/firefox.js:23: On 2018/08/23 21:54:20, Sebastian Noack wrote: > On 2018/08/23 18:56:02, tlucas wrote: > > On 2018/08/23 16:38:44, Sebastian Noack wrote: > > > Nit: Unlike in Python, we generally don't group imports by > > > corelib/third-party/first-party. > > > > So no grouping at all? Or an arbitrary grouping? > > Just remove the blank line here and in line 28. Done. https://codereview.adblockplus.org/29860555/diff/29862575/test/firefox.js File test/firefox.js (right): https://codereview.adblockplus.org/29860555/diff/29862575/test/firefox.js#new... test/firefox.js:111: driver.quit(); On 2018/08/23 21:54:20, Sebastian Noack wrote: > This looks like a potential race condition. You want to call driver.quit() > and test.done() once the above actions completed (asynchronously), don't you? > So it seems what you want to do is something like this: > > .then(() => Promise.all([ > driver.findElements(...).then(elements => > Promise.all(elements.map(...)) > ), > driver.findElements(...).then(elements => > Promise.all(elements.map(...)) > ) > ])).then(() => > { > driver.quit(); > test.done(); > }); Done + made it dry
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 too now. https://codereview.adblockplus.org/29860555/diff/29863555/test/firefox.js File test/firefox.js (right): https://codereview.adblockplus.org/29860555/diff/29863555/test/firefox.js#new... test/firefox.js:92: data.search("Tests completed") >= 0)) If you use .includes() instead of .search() you get a boolean instead of the position, so that you don't have to compare the return value. https://codereview.adblockplus.org/29860555/diff/29863555/test/firefox.js#new... test/firefox.js:95: reportElements(test, driver, "#qunit-tests .fail .test-name", false) You could make it even a little more DRY by removing the "css" argument, and then constructing the selector like this inside reportElements(): `#qunit-tests ${success ? ".pass" : ".fail"} .test-name`
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 -- gecko On 2018/08/24 12:29:10, Sebastian Noack wrote: > This needs to be changed too now. Done. https://codereview.adblockplus.org/29860555/diff/29863555/test/firefox.js File test/firefox.js (right): https://codereview.adblockplus.org/29860555/diff/29863555/test/firefox.js#new... test/firefox.js:92: data.search("Tests completed") >= 0)) On 2018/08/24 12:29:10, Sebastian Noack wrote: > If you use .includes() instead of .search() you get a boolean instead of the > position, so that you don't have to compare the return value. Done. https://codereview.adblockplus.org/29860555/diff/29863555/test/firefox.js#new... test/firefox.js:95: reportElements(test, driver, "#qunit-tests .fail .test-name", false) On 2018/08/24 12:29:11, Sebastian Noack wrote: > You could make it even a little more DRY by removing the "css" argument, and > then constructing the selector like this inside reportElements(): > > `#qunit-tests ${success ? ".pass" : ".fail"} .test-name` Good point, done.
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#new... test/firefox.js:34: By.css(`#qunit-tests ${success ? ".pass" : ".fail"} .test-name`)). Nit: (Same like in Python) please put closing parenthesis on a line with the same indentation level as the line with the corresponding opening parenthesis. return driver.findElements( By.css(`#qunit-tests ${success ? ".pass" : ".fail"} .test-name`) ).then(elements => Promise.all(elements.map(elem => elem.getAttribute("innerHTML").then(data => test.ok(success, data)) )));
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#new... test/firefox.js:34: By.css(`#qunit-tests ${success ? ".pass" : ".fail"} .test-name`)). On 2018/08/24 12:54:42, Sebastian Noack wrote: > Nit: (Same like in Python) please put closing parenthesis on a line with the > same indentation level as the line with the corresponding opening parenthesis. > > return driver.findElements( > By.css(`#qunit-tests ${success ? ".pass" : ".fail"} .test-name`) > ).then(elements => Promise.all(elements.map(elem => > elem.getAttribute("innerHTML").then(data => test.ok(success, data)) > ))); Done.
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#new... test/firefox.js:29: Nit: We usually only put one blank line after the imports. https://codereview.adblockplus.org/29860555/diff/29863569/test/firefox.js#new... test/firefox.js:102: }).catch((err) => I'd suggest to change the error handling like that: .then( () => { driver.quit(); test.done(); }, err => { driver.quit(); throw err; } ) 1. There is no reason to call driver.quit() again in case driver.quit() or test.done() (after calling driver.quit()) failed for some reason. 2. We don't need to wait for driver.quit() to complete before emitting the error. 3. Nit: The parenthesis are redundant (and we usually omit them) in arrow functions with exactly one argument. Even better would be to call the Promise's finally() method, but that would require Node.js >=10, and it doesn't seem to be worthwile to bump up the minimum version just for that.
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#new... test/firefox.js:29: On 2018/08/24 13:27:59, Sebastian Noack wrote: > Nit: We usually only put one blank line after the imports. Done. https://codereview.adblockplus.org/29860555/diff/29863569/test/firefox.js#new... test/firefox.js:102: }).catch((err) => On 2018/08/24 13:27:59, Sebastian Noack wrote: > I'd suggest to change the error handling like that: > > .then( > () => > { > driver.quit(); > test.done(); > }, > err => > { > driver.quit(); > throw err; > } > ) > > 1. There is no reason to call driver.quit() again in case driver.quit() > or test.done() (after calling driver.quit()) failed for some reason. > 2. We don't need to wait for driver.quit() to complete before emitting > the error. > 3. Nit: The parenthesis are redundant (and we usually omit them) in > arrow functions with exactly one argument. > > Even better would be to call the Promise's finally() method, but that would > require Node.js >=10, and it doesn't seem to be worthwile to bump up the minimum > version just for that. Done. (Still waiting for driver.quit() to finish before throwing the error, as discussed)
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.
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. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
