Issue 6884 - Migrate from nodeunit to mocha

Issue 6884 - Migrate from nodeunit to mocha

[tlucas, 2018-08-26 15:10:51]

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:72: after(() => driver.quit());
Nit: IMHO this should go after it(), to reflect the actual execution order.

https://codereview.adblockplus.org/29864561/diff/29864562/test/firefox.js#new...
test/firefox.js:84: driver.navigate().to(origin + "/qunit/index.html").then(()
=>
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?

[Sebastian Noack, 2018-08-26 15:42:18]

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: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.

[Sebastian Noack, 2018-08-26 19:02:00]

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:72: after(() => driver.quit());
On 2018/08/26 15:10:51, tlucas wrote:
> Nit: IMHO this should go after it(), to reflect the actual execution order.

Done. Though one could argue the other way around to keep the hooks together.

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))
While changing this code anyway, I now, also use textContent instead of
innerHTML in order to avoid markup from potentially being printed.

[tlucas, 2018-08-26 21:13:51]

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.