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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 2 months ago by Sebastian Noack
Modified:
1 year, 2 months ago
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
1 year, 2 months ago (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 ...
1 year, 2 months ago (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: ...
1 year, 2 months ago (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: > ...
1 year, 2 months ago (2018-08-26 19:02:00 UTC) #4
tlucas
1 year, 2 months ago (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.
Sign in to reply to this message.

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