Issue 4495 - Update adblockplustests dependency

Issue 4495 - Update adblockplustests dependency

[kzar, 2016-10-06 09:21:07]

Patch Set 1

[kzar, 2016-10-06 09:37:59]

Patch Set 2 : Remove adblockplustests

[Wladimir Palant, 2016-10-06 09:44:44]

LGTM

[Sebastian Noack, 2016-10-06 11:47:36]

https://codereview.adblockplus.org/29356068/diff/29356072/metadata.common
File metadata.common (left):

https://codereview.adblockplus.org/29356068/diff/29356072/metadata.common#old...
metadata.common:107: adblockplustests/chrome/content/tests/filterClasses.js
Do I miss something here, or shouldn't we import those tests from
adblockpluscore instead in order to run them on Chrome?

[Wladimir Palant, 2016-10-06 12:09:29]

https://codereview.adblockplus.org/29356068/diff/29356072/metadata.common
File metadata.common (left):

https://codereview.adblockplus.org/29356068/diff/29356072/metadata.common#old...
metadata.common:107: adblockplustests/chrome/content/tests/filterClasses.js
On 2016/10/06 11:47:36, Sebastian Noack wrote:
> Do I miss something here, or shouldn't we import those tests from
> adblockpluscore instead in order to run them on Chrome?

This makes little sense. In adblockpluscore these tests run in a sandbox, making
them run in Chrome against the real extension would require considerable effort.
And effectively we would only be testing JSHydra which is expected to go away
soon.

[Sebastian Noack, 2016-10-06 12:24:17]

https://codereview.adblockplus.org/29356068/diff/29356072/metadata.common
File metadata.common (left):

https://codereview.adblockplus.org/29356068/diff/29356072/metadata.common#old...
metadata.common:107: adblockplustests/chrome/content/tests/filterClasses.js
On 2016/10/06 12:09:29, Wladimir Palant wrote:
> On 2016/10/06 11:47:36, Sebastian Noack wrote:
> > Do I miss something here, or shouldn't we import those tests from
> > adblockpluscore instead in order to run them on Chrome?
> 
> This makes little sense. In adblockpluscore these tests run in a sandbox,
making
> them run in Chrome against the real extension would require considerable
effort.
> And effectively we would only be testing JSHydra which is expected to go away
> soon.

Not sure about the effort, but I think it generally makes sense to run these
tests on the actual platform, the code is used on. One could argue that nodejs
is the same platform, but that's not quite true. Though nodejs is using the same
JavaScript engine, chances are that your nodejs version is using a different V8
version than the latest Chrome, not to mention that the global namespace is for
the most part quite different in nodejs from what you have in the browser. Also
what is about platforms like Microsoft Edge that don't even use V8? Do you
remember, when a misoptimization by JavaScriptCore (WebKit) of our element
hiding code caused each page to be rendered blank?

[Wladimir Palant, 2016-10-06 14:27:59]

https://codereview.adblockplus.org/29356068/diff/29356072/metadata.common
File metadata.common (left):

https://codereview.adblockplus.org/29356068/diff/29356072/metadata.common#old...
metadata.common:107: adblockplustests/chrome/content/tests/filterClasses.js
On 2016/10/06 12:24:17, Sebastian Noack wrote:
> Not sure about the effort, but I think it generally makes sense to run these
> tests on the actual platform, the code is used on. One could argue that nodejs
> is the same platform, but that's not quite true. Though nodejs is using the
same
> JavaScript engine, chances are that your nodejs version is using a different
V8
> version than the latest Chrome, not to mention that the global namespace is
for
> the most part quite different in nodejs from what you have in the browser.
Also
> what is about platforms like Microsoft Edge that don't even use V8? Do you
> remember, when a misoptimization by JavaScriptCore (WebKit) of our element
> hiding code caused each page to be rendered blank?

I don't really remember that misoptimization being caught by unit tests, I think
it was manual testing?

Either way, these are *unit* tests, their purpose it to test *our* code and
making sure that it is correct. The only platform relevant here is ECMAScript 6.
Catching errors in the ECMAScript implementation cannot be the purpose - we
cannot possibly know what to test.

Globals are different, you are right. But these are integration points and
should be subject to integration tests if necessary - making sure that the
platform behaves the way we expect it to. Luckily, the unit tests now show
clearly what globals they depend on
(https://hg.adblockplus.org/adblockpluscore/file/cc214664f060/test/_common.js#l35)
so we can add integration tests if we have concerns. The only non-trivial thing
in there is the version comparator and that one is already tested in Chrome.

[Sebastian Noack, 2016-10-06 16:24:23]

https://codereview.adblockplus.org/29356068/diff/29356072/metadata.common
File metadata.common (left):

https://codereview.adblockplus.org/29356068/diff/29356072/metadata.common#old...
metadata.common:107: adblockplustests/chrome/content/tests/filterClasses.js
On 2016/10/06 14:27:58, Wladimir Palant wrote:
> On 2016/10/06 12:24:17, Sebastian Noack wrote:
> > Not sure about the effort, but I think it generally makes sense to run these
> > tests on the actual platform, the code is used on. One could argue that
nodejs
> > is the same platform, but that's not quite true. Though nodejs is using the
> same
> > JavaScript engine, chances are that your nodejs version is using a different
> V8
> > version than the latest Chrome, not to mention that the global namespace is
> for
> > the most part quite different in nodejs from what you have in the browser.
> Also
> > what is about platforms like Microsoft Edge that don't even use V8? Do you
> > remember, when a misoptimization by JavaScriptCore (WebKit) of our element
> > hiding code caused each page to be rendered blank?
> 
> I don't really remember that misoptimization being caught by unit tests, I
think
> it was manual testing?
> 
> Either way, these are *unit* tests, their purpose it to test *our* code and
> making sure that it is correct. The only platform relevant here is ECMAScript
6.
> Catching errors in the ECMAScript implementation cannot be the purpose - we
> cannot possibly know what to test.
> 
> Globals are different, you are right. But these are integration points and
> should be subject to integration tests if necessary - making sure that the
> platform behaves the way we expect it to. Luckily, the unit tests now show
> clearly what globals they depend on
>
(https://hg.adblockplus.org/adblockpluscore/file/cc214664f060/test/_common.js#l35)
> so we can add integration tests if we have concerns. The only non-trivial
thing
> in there is the version comparator and that one is already tested in Chrome.

I don't agree, but if we still want to run the core tests on Chrome and Edge
this is apparently a separate effort, unrelated of this change. So LGTM.