Issue 4838 - Use nodeunit framework for integration tests running in browser

Issue 4838 - Use nodeunit framework for integration tests running in browser

[Wladimir Palant, 2017-01-24 16:39:24]

https://codereview.adblockplus.org/29373596/diff/29373597/test/browser/elemHi...
File test/browser/elemHideEmulation.js (left):

https://codereview.adblockplus.org/29373596/diff/29373597/test/browser/elemHi...
test/browser/elemHideEmulation.js:45:
document.head.removeChild(styleElements[0]);
This piece of code has multiple issues:

* getElementsByTagName returns a live list, so removing elements from the
document will remove them from the list as well.
* This always removes styleElements[0], regardless of the current index.
* This assumes that the parent of the style nodes is always document.head - this
isn't necessarily the case, the parent could be another element inside
document.head (getElementsByTagName recurses into subtrees).

[Wladimir Palant, 2017-01-24 20:10:18]

I realized that the PhantomJS script is running in a window environment so
creating a page is unnecessary - this makes things a lot simpler. Side-effect:
the tests have access to all PhantomJS APIs and console.log().

[Wladimir Palant, 2017-01-25 09:55:52]

One more change, relatively minor this time - loading all modules into the same
page might cause naming conflicts due to shared scope. So I'm creating a new
frame for each module now.

[Felix Dahlke, 2017-01-30 14:37:59]

Nice! Could you update the README to reflect this?

https://codereview.adblockplus.org/29373596/diff/29373597/test/browser/elemHi...
File test/browser/elemHideEmulation.js (left):

https://codereview.adblockplus.org/29373596/diff/29373597/test/browser/elemHi...
test/browser/elemHideEmulation.js:45:
document.head.removeChild(styleElements[0]);
On 2017/01/24 16:39:24, Wladimir Palant wrote:
> * getElementsByTagName returns a live list, so removing elements from the
> document will remove them from the list as well.
> * This always removes styleElements[0], regardless of the current index.
> * This assumes that the parent of the style nodes is always document.head -
this
> isn't necessarily the case, the parent could be another element inside
> document.head (getElementsByTagName recurses into subtrees).

Oops - all true.

https://codereview.adblockplus.org/29373596/diff/29373625/browsertests.js
File browsertests.js (right):

https://codereview.adblockplus.org/29373596/diff/29373625/browsertests.js#new...
browsertests.js:3: * Copyright (C) 2006-2016 Eyeo GmbH
It's 2017 :)

https://codereview.adblockplus.org/29373596/diff/29373625/browsertests.js#new...
browsertests.js:22: // TODO: This should use promises once PhantomJS supports
them, this will make
Could you reference issue 4796 here? In the other files where we are restricted
to ES5 because of PhantomJS, we have this comment, I'd personally just add it
the same way at the top in this file:

// We are currently limited to ECMAScript 5 in this file, because it is being
// used in the browser tests. See https://issues.adblockplus.org/ticket/4796

https://codereview.adblockplus.org/29373596/diff/29373625/browsertests.js#new...
browsertests.js:24: function safeCall(callback)
Have you considered using phantom.onError instead?

https://codereview.adblockplus.org/29373596/diff/29373625/browsertests.js#new...
browsertests.js:37: console.log(message);
Since it's an error message, how about console.error?

https://codereview.adblockplus.org/29373596/diff/29373625/browsertests.js#new...
browsertests.js:38: phantom.exit();
phantom.exit(1) in this case?

https://codereview.adblockplus.org/29373596/diff/29373625/browsertests.js#new...
browsertests.js:40: }
Missing semicolon?

https://codereview.adblockplus.org/29373596/diff/29373625/browsertests.js#new...
browsertests.js:53: function run()
Nit: Move this below runTests() to have the functions declared in a consistent
order? (Maybe I'm being meticulous though.)

https://codereview.adblockplus.org/29373596/diff/29373625/package.json
File package.json (right):

https://codereview.adblockplus.org/29373596/diff/29373625/package.json#newcode8
package.json:8: "phantomjs2": "^2.2.0",
Why not use phantomjs-prebuilt [1], which is what node-qunit-phantomjs depended
on? The phantomjs2 package looks a bit dead - still refers to the old name of
the former package and claims it will only install PhantomJS 1.9.x.

[1]: https://www.npmjs.com/package/phantomjs-prebuilt

https://codereview.adblockplus.org/29373596/diff/29373625/test/browser/elemHi...
File test/browser/elemHideEmulation.js (right):

https://codereview.adblockplus.org/29373596/diff/29373625/test/browser/elemHi...
test/browser/elemHideEmulation.js:105: loadScript(myUrl +
"/../../../chrome/content/elemHideEmulation.js", function()
Can you wrap this to stay within 80 columns?

https://codereview.adblockplus.org/29373596/diff/29373625/test/browser/elemHi...
test/browser/elemHideEmulation.js:205:
exports.testPropertySelectorWithDynamicallyChangedProperty = function(test)
I found this easier to understand with "works for" rather than "with", but I can
see how it doesn't fit. Maybe just "testDynamicallyChangedProperty"? This tests
logic that is the same for all emulated features, I presume, therefore not
specific to property selectors.

https://codereview.adblockplus.org/29373596/diff/29373625/test_runner.js
File test_runner.js (right):

https://codereview.adblockplus.org/29373596/diff/29373625/test_runner.js#newc...
test_runner.js:28: let nodeunitFiles = [];
Since the browser tests are using nodeunit as well now, we should probably
rename this. `unitFiles` comes to mind - as in "unit tests and browser tests" -
doesn't seem brilliant, but I suppose it would be clear enough.

https://codereview.adblockplus.org/29373596/diff/29373625/test_runner.js#newc...
test_runner.js:86: if (nodeunitFiles.length)
IMO we should run the unit tests first - should fail faster that way.

[Wladimir Palant, 2017-02-24 09:18:56]

https://codereview.adblockplus.org/29373596/diff/29373625/browsertests.js
File browsertests.js (right):

https://codereview.adblockplus.org/29373596/diff/29373625/browsertests.js#new...
browsertests.js:3: * Copyright (C) 2006-2016 Eyeo GmbH
On 2017/01/30 14:37:58, Felix Dahlke wrote:
> It's 2017 :)

Yes, but the copyright header hasn't been updated for the other files either.
Well, whatever...

https://codereview.adblockplus.org/29373596/diff/29373625/browsertests.js#new...
browsertests.js:22: // TODO: This should use promises once PhantomJS supports
them, this will make
On 2017/01/30 14:37:58, Felix Dahlke wrote:
> Could you reference issue 4796 here? In the other files where we are
restricted
> to ES5 because of PhantomJS, we have this comment, I'd personally just add it
> the same way at the top in this file:
> 
> // We are currently limited to ECMAScript 5 in this file, because it is being
> // used in the browser tests. See https://issues.adblockplus.org/ticket/4796

Done.

https://codereview.adblockplus.org/29373596/diff/29373625/browsertests.js#new...
browsertests.js:24: function safeCall(callback)
On 2017/01/30 14:37:58, Felix Dahlke wrote:
> Have you considered using phantom.onError instead?

I was using that originally, it wasn't catching all errors reliably.

https://codereview.adblockplus.org/29373596/diff/29373625/browsertests.js#new...
browsertests.js:37: console.log(message);
On 2017/01/30 14:37:58, Felix Dahlke wrote:
> Since it's an error message, how about console.error?

nodeunit prints all output via console.log(), including errors. I guess this
makes redirection of test results easier.

https://codereview.adblockplus.org/29373596/diff/29373625/browsertests.js#new...
browsertests.js:38: phantom.exit();
On 2017/01/30 14:37:58, Felix Dahlke wrote:
> phantom.exit(1) in this case?

Done.

https://codereview.adblockplus.org/29373596/diff/29373625/browsertests.js#new...
browsertests.js:40: }
On 2017/01/30 14:37:58, Felix Dahlke wrote:
> Missing semicolon?

Done.

https://codereview.adblockplus.org/29373596/diff/29373625/browsertests.js#new...
browsertests.js:53: function run()
On 2017/01/30 14:37:58, Felix Dahlke wrote:
> Nit: Move this below runTests() to have the functions declared in a consistent
> order? (Maybe I'm being meticulous though.)

Done.

https://codereview.adblockplus.org/29373596/diff/29373625/package.json
File package.json (right):

https://codereview.adblockplus.org/29373596/diff/29373625/package.json#newcode8
package.json:8: "phantomjs2": "^2.2.0",
On 2017/01/30 14:37:58, Felix Dahlke wrote:
> Why not use phantomjs-prebuilt [1], which is what node-qunit-phantomjs
depended
> on? The phantomjs2 package looks a bit dead - still refers to the old name of
> the former package and claims it will only install PhantomJS 1.9.x.

This seems to be a misunderstanding - the package they forked was installing
PhantomJS 1.9.x, whereas phantomjs2 is installing PhantomJS 2.x. It will
currently install PhantomJS 2.1.1 which is the latest version.

My understanding was that phantomjs2 is the official distribution. I see now
that this is not the case. I don't see any real advantages of one over the
other, let's wait for PhantomJS 2.5 - if phantomjs-prebuilt gets updated sooner
we can easily switch to it again.

https://codereview.adblockplus.org/29373596/diff/29373625/test/browser/elemHi...
File test/browser/elemHideEmulation.js (right):

https://codereview.adblockplus.org/29373596/diff/29373625/test/browser/elemHi...
test/browser/elemHideEmulation.js:105: loadScript(myUrl +
"/../../../chrome/content/elemHideEmulation.js", function()
On 2017/01/30 14:37:59, Felix Dahlke wrote:
> Can you wrap this to stay within 80 columns?

Done. And while at it, I've done some linting in order to get other nits fixed
as well.

https://codereview.adblockplus.org/29373596/diff/29373625/test/browser/elemHi...
test/browser/elemHideEmulation.js:205:
exports.testPropertySelectorWithDynamicallyChangedProperty = function(test)
On 2017/01/30 14:37:59, Felix Dahlke wrote:
> I found this easier to understand with "works for" rather than "with", but I
can
> see how it doesn't fit. Maybe just "testDynamicallyChangedProperty"? This
tests
> logic that is the same for all emulated features, I presume, therefore not
> specific to property selectors.

Done.

https://codereview.adblockplus.org/29373596/diff/29373625/test_runner.js
File test_runner.js (right):

https://codereview.adblockplus.org/29373596/diff/29373625/test_runner.js#newc...
test_runner.js:28: let nodeunitFiles = [];
On 2017/01/30 14:37:59, Felix Dahlke wrote:
> Since the browser tests are using nodeunit as well now, we should probably
> rename this. `unitFiles` comes to mind - as in "unit tests and browser tests"
-
> doesn't seem brilliant, but I suppose it would be clear enough.

Done.

https://codereview.adblockplus.org/29373596/diff/29373625/test_runner.js#newc...
test_runner.js:86: if (nodeunitFiles.length)
On 2017/01/30 14:37:59, Felix Dahlke wrote:
> IMO we should run the unit tests first - should fail faster that way.

This is not so easy, unit tests are async and we would need to wait for
completion somehow. Also, browsertests aren't really slow for me, no real delay
here.

[Felix Dahlke, 2017-02-24 10:43:39]

LGTM, up to you whether you want to stick to phantomjs2, see my comment.

https://codereview.adblockplus.org/29373596/diff/29373625/browsertests.js
File browsertests.js (right):

https://codereview.adblockplus.org/29373596/diff/29373625/browsertests.js#new...
browsertests.js:3: * Copyright (C) 2006-2016 Eyeo GmbH
On 2017/02/24 09:18:55, Wladimir Palant wrote:
> On 2017/01/30 14:37:58, Felix Dahlke wrote:
> > It's 2017 :)
> 
> Yes, but the copyright header hasn't been updated for the other files either.
> Well, whatever...

:)

https://codereview.adblockplus.org/29373596/diff/29373625/browsertests.js#new...
browsertests.js:24: function safeCall(callback)
On 2017/02/24 09:18:55, Wladimir Palant wrote:
> On 2017/01/30 14:37:58, Felix Dahlke wrote:
> > Have you considered using phantom.onError instead?
> 
> I was using that originally, it wasn't catching all errors reliably.

Acknowledged.

https://codereview.adblockplus.org/29373596/diff/29373625/browsertests.js#new...
browsertests.js:37: console.log(message);
On 2017/02/24 09:18:55, Wladimir Palant wrote:
> On 2017/01/30 14:37:58, Felix Dahlke wrote:
> > Since it's an error message, how about console.error?
> 
> nodeunit prints all output via console.log(), including errors. I guess this
> makes redirection of test results easier.

Acknowledged.

https://codereview.adblockplus.org/29373596/diff/29373625/package.json
File package.json (right):

https://codereview.adblockplus.org/29373596/diff/29373625/package.json#newcode8
package.json:8: "phantomjs2": "^2.2.0",
On 2017/02/24 09:18:55, Wladimir Palant wrote:
> On 2017/01/30 14:37:58, Felix Dahlke wrote:
> > Why not use phantomjs-prebuilt [1], which is what node-qunit-phantomjs
> depended
> > on? The phantomjs2 package looks a bit dead - still refers to the old name
of
> > the former package and claims it will only install PhantomJS 1.9.x.
> 
> This seems to be a misunderstanding - the package they forked was installing
> PhantomJS 1.9.x, whereas phantomjs2 is installing PhantomJS 2.x. It will
> currently install PhantomJS 2.1.1 which is the latest version.
> 
> My understanding was that phantomjs2 is the official distribution. I see now
> that this is not the case. I don't see any real advantages of one over the
> other, let's wait for PhantomJS 2.5 - if phantomjs-prebuilt gets updated
sooner
> we can easily switch to it again.

How I see it, the story was like this:

1. The following package was called phantomjs:
https://github.com/Medium/phantomjs

2. The phantomjs2 guy forked it to support PhantomJS >=2, presumably because the
phantomjs package wasn't fast enough to update.

3. The phantomjs package renamed itself to phantomjs-prebuilt. From the NPM
page: "Pre-2.0, this package was published to NPM as phantomjs. We changed the
name to phantomjs-prebuilt at the request of PhantomJS team."

So phantomjs2 is a fork of phantomjs-prebuilt - and it seems to be slightly
behind. phantomjs-prebuilt had the last release in December 2016, phantomjs2 had
the last release in February 2016.

Ultimately I'm OK with phantomjs2. But phantomjs-prebuilt is the one I'd pick.

https://codereview.adblockplus.org/29373596/diff/29373625/test_runner.js
File test_runner.js (right):

https://codereview.adblockplus.org/29373596/diff/29373625/test_runner.js#newc...
test_runner.js:86: if (nodeunitFiles.length)
On 2017/02/24 09:18:55, Wladimir Palant wrote:
> On 2017/01/30 14:37:59, Felix Dahlke wrote:
> > IMO we should run the unit tests first - should fail faster that way.
> 
> This is not so easy, unit tests are async and we would need to wait for
> completion somehow. Also, browsertests aren't really slow for me, no real
delay
> here.

Acknowledged.

[Wladimir Palant, 2017-02-24 10:56:19]

https://codereview.adblockplus.org/29373596/diff/29373625/package.json
File package.json (right):

https://codereview.adblockplus.org/29373596/diff/29373625/package.json#newcode8
package.json:8: "phantomjs2": "^2.2.0",
On 2017/02/24 10:43:39, Felix Dahlke wrote:
> So phantomjs2 is a fork of phantomjs-prebuilt - and it seems to be slightly
> behind. phantomjs-prebuilt had the last release in December 2016, phantomjs2
had
> the last release in February 2016.

Given that last PhantomJS release was in January 2016 - that's not "behind." ;)

As I said, I don't really have indicators as to which one is better to use.
We'll see when PhantomJS 2.5 comes out and switching is easy.