Issue 4726 - Add tests for the element hiding emulation content script

package.json

Index: package.json
===================================================================
--- a/package.json
+++ b/package.json
@@ -1,13 +1,17 @@
 {
   "name": "adblockpluscore",
   "repository": "https://hg.adblockplus.org/adblockpluscore",
   "license": "GPL-3.0",
   "dependencies": {},
   "devDependencies": {
-    "sandboxed-module": "2.0.3",
-    "nodeunit": "0.9.1"
+    "node-qunit-phantomjs": "^1.5.0",

[kzar, 2017/01/09 08:28:43]

I wonder what node-qunit-phantomjs really adds? Couldn't we have the
test-browser command run the command we need directly instead and thus avoid the
dependency?

[Felix Dahlke, 2017/01/10 09:16:47]

QUnit tests run in an HTML page, we have to extract the results from there. In
my understanding that's what node-qunit-phantomjs does. I would rather not have
us do that ourselves.

[kzar, 2017/01/10 11:05:33]

Well it seems that most of the work is actually done by qunit-phantomjs-runner
which provides the test results as JSON. It seems that all
qunit-phantomjs-runner does is parse the JSON results and output them prettily.
(The other thing it seems to do is have the phantomjs-prebuilt dependency which
means phantomjs is installed automatically.)

I wonder if we could add the phantomjs-prebuilt and qunit-phantomjs-runner
dependencies ourselves instead.

While at it it would also be nice if we could combine the results from both unit
and browser tests, so that the test totals etc are correct. (It seems like at
the moment we simply print the formatted results for both, one after another.)
If we could get nodeunit to give us the results formatted as JSON too perhaps we
could then figure out a way to have them all rendered by nodeunit together
properly?

(That would have the bonus of allowing us to print all the results in a way that
a CI server understands in the future too.)

[Felix Dahlke, 2017/01/10 13:27:32]

I guess we can, but given how qunit-phantomjs-runner and node-qunit-phantomjs
are from the same author and thus about equally likely to become a problem for
us - I don't see the point.
Good point. I did a little bit of research and it looks like a bit of work, but
we should be able to have both runners output the same standardised format.
Seems like something we should tackle in a followup however - until we have
proper CI that would be purely cosmetic, and this review is blocking important
stuff. If you agree I'll create an issue.

[kzar, 2017/01/11 04:25:28]

Well the point is to minimise dependencies. One package depends on the other +
more. The fewer dependencies the better right? That's certainly how I remember
my reviews being treated, for example the Crowdin translation sync where we went
to painful lengths to replace the requests library with urllib3 on which it
depended.
Wouldn't it be better to just do it properly now? (I don't think this blocks the
other work since you can still run the tests even if they're not passed review
and pushed.) FWIW I agree that the output for CI can be tackled as a future
issue since we don't have any CI yet anyway.

[Felix Dahlke, 2017/01/11 10:06:02]

It's not that simple. We should aim to minimise dependencies in general, but not
religiously, it really depends on the case. In the case of the requests library
vs urllib3, my understanding is that the former is a somewhat "unnecessary"
wrapper around urllib3. I don't know the details so I don't know if that is true
and if not using the request library was a wise decision.

I see four problems with dependencies: 1) The dependency could cease to be
maintained, in which case we would have to fork or replace it 2) The dependency
increases complexity by handling more use cases than just ours 3) The dependency
would be hard to update/replace if we want/need to 4) The dependency could
violate someone else's copyright, a problem we would inherit by using it under
an open source license.

I personally have run into traps #1, #2 and #3 countless times, #4 once.
Wladimir had similar experiences, which lead us to be careful with external
dependencies from the start. At the same time, we need to find the right balance
between these dangers and pragmatism (i.e. how much time/trouble it saves us to
use a dependency vs what the actual risks with it are).

In this particular case, #1 does not really apply, since it was written by the
very same author to be used in tandem. In fact, it seems more like he split one
package up into two for modularisation purposes and nothing else. #2 does from
what I see not really apply either, since node-qunit-phantomjs does exactly one
thing - support the use case we need. #3 is actually _better_ if we use this,
since we currently only make a single API call, whereas if we would use
qunit-phantomjs-runner directly we would use more of its API.

Therefore I see absolutely no reason not to use node-qunit-phantomjs right now.
If we do end up parsing test results ourselves however, we probably want/need to
get rid of it.
 
That's exactly the part I mean. I feel pretty strong about wanting to use
node-qunit-phantomjs as long as we don't work on having the same output format,
see the reasoning above.

+    "nodeunit": "0.9.1",
+    "qunitjs": "^2.1.0",

[kzar, 2017/01/09 08:28:42]

IMO seems kind of a hack to use a nodejs script from a HTML page, since the same
APIs are not necessarily available. Maybe it's better to just include qunit.js
in the browser tests directly ourselves?

[Felix Dahlke, 2017/01/10 09:16:48]

qunitjs is a pure frontend dependency in my understanding - how would one use it
without a browser? I don't see a problem with using NPM to download frontend
dependencies if we're using it anyway.

[kzar, 2017/01/10 11:05:33]

Fair enough.

+    "sandboxed-module": "2.0.3"
   },
   "scripts": {
-    "test": "node test_runner.js"
+    "test": "npm run test-node; npm run test-browser",

[kzar, 2017/01/09 08:28:43]

IMO we should instead modify test_runner.js. It would be cool if `node test` ran
that and that in turn ran all the tests.

(That script also accepts the paths of specific test files to run, that means
you can run only a subset of the tests if you like. I think it should be updated
to work when passed the paths to your browser tests too. That way you could tell
the script to run one browser test and one node test file for example, that
might be pretty useful if they were related.)

[Felix Dahlke, 2017/01/10 09:16:48]

Done.

+    "test-node": "node test_runner.js",
+    "test-browser": "node_modules/node-qunit-phantomjs/bin/node-qunit-phantomjs test/browser/index.html"
   }
 }