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

Issue 29864561: Issue 6884 - Migrate from nodeunit to mocha (Closed)

Created:
Aug. 25, 2018, 5 p.m. by Sebastian Noack
Modified:
Aug. 27, 2018, 10 a.m.
Reviewers:
tlucas
CC:
kzar, hub
Visibility:
Public.

Description

Issue 6884 - Migrate from nodeunit to mocha

Patch Set 1 #

Total comments: 5

Patch Set 2 : Move after() to bottom, use textContent instead of innerHTML #

Total comments: 2

Patch Set 3 : Inline reportElements() for better code locality #

Unified diffs Side-by-side diffs Delta from patch set Stats (+51 lines, -61 lines) Patch
M package.json View 1 chunk +2 lines, -2 lines 0 comments Download
M test/firefox.js View 1 2 3 chunks +49 lines, -59 lines 0 comments Download

Messages

Total messages: 5
Sebastian Noack
Aug. 25, 2018, 11:17 p.m. (2018-08-25 23:17:36 UTC) #1
tlucas
https://codereview.adblockplus.org/29864561/diff/29864562/test/firefox.js File test/firefox.js (right): https://codereview.adblockplus.org/29864561/diff/29864562/test/firefox.js#newcode72 test/firefox.js:72: after(() => driver.quit()); Nit: IMHO this should go after ...
Aug. 26, 2018, 3:10 p.m. (2018-08-26 15:10:51 UTC) #2
Sebastian Noack
https://codereview.adblockplus.org/29864561/diff/29864562/test/firefox.js File test/firefox.js (right): https://codereview.adblockplus.org/29864561/diff/29864562/test/firefox.js#newcode84 test/firefox.js:84: driver.navigate().to(origin + "/qunit/index.html").then(() => On 2018/08/26 15:10:51, tlucas wrote: ...
Aug. 26, 2018, 3:42 p.m. (2018-08-26 15:42:18 UTC) #3
Sebastian Noack
https://codereview.adblockplus.org/29864561/diff/29864562/test/firefox.js File test/firefox.js (right): https://codereview.adblockplus.org/29864561/diff/29864562/test/firefox.js#newcode72 test/firefox.js:72: after(() => driver.quit()); On 2018/08/26 15:10:51, tlucas wrote: > ...
Aug. 26, 2018, 7:02 p.m. (2018-08-26 19:02:00 UTC) #4
tlucas
Aug. 26, 2018, 9:13 p.m. (2018-08-26 21:13:51 UTC) #5
LGTM

https://codereview.adblockplus.org/29864561/diff/29864562/test/firefox.js
File test/firefox.js (right):

https://codereview.adblockplus.org/29864561/diff/29864562/test/firefox.js#new...
test/firefox.js:84: driver.navigate().to(origin + "/qunit/index.html").then(()
=>
On 2018/08/26 15:42:18, Sebastian Noack wrote:
> On 2018/08/26 15:10:51, tlucas wrote:
> > Just FMI, why do you deem this more appropriate than
> > "driver.executeScript("location.href = \"qunit/index.html\";")",
> > especially since you run executeScript() anyway in line 67 to read to
retrieve
> > origin?
> 
> I need the extension URL prefix for the tests I'm going to add anyway. Also
once
> we add more tests (reusing the same browser instance), it will be a hassle to
> guarantee that the first run page will be open and focused at the beginning of
> each test. By getting the origin upfront, we have larger code reuse, and less
> assumptions in the individual tests.

Thanks for clarifying.

https://codereview.adblockplus.org/29864561/diff/29865577/test/firefox.js
File test/firefox.js (right):

https://codereview.adblockplus.org/29864561/diff/29865577/test/firefox.js#new...
test/firefox.js:77: elem.getAttribute("textContent").then(data =>
assert.ok(success, data))
On 2018/08/26 19:02:00, Sebastian Noack wrote:
> While changing this code anyway, I now, also use textContent instead of
> innerHTML in order to avoid markup from potentially being printed.

Acknowledged.

Powered by Google App Engine
This is Rietveld