Issue 4055 - Test ElemHide.getSelectorsFordomain

Issue 4055 - Test ElemHide.getSelectorsFordomain

[kzar, 2016-05-22 16:43:40]

Patch Set 1

https://codereview.adblockplus.org/29342884/diff/29342885/package.json
File package.json (right):

https://codereview.adblockplus.org/29342884/diff/29342885/package.json#newcode3
package.json:3: "repository": "https://hg.adblockplus.org/adblockpluscore",
This stops NPM complaining.

[kzar, 2016-05-22 20:31:27]

Patch Set 2 : Add test and note about known limitation.

[kzar, 2016-05-23 11:46:50]

Patch Set 3 : Ignore selector order

https://codereview.adblockplus.org/29342884/diff/29342919/test/elemHide.js
File test/elemHide.js (right):

https://codereview.adblockplus.org/29342884/diff/29342919/test/elemHide.js#ne...
test/elemHide.js:39: test.deepEqual(ElemHide.getSelectorsForDomain(domain,
specificOnly).sort(),
Different implementations produce the selectors in different orders, but I don't
think that's important.

[Wladimir Palant, 2016-05-23 13:23:02]

https://codereview.adblockplus.org/29342884/diff/29342919/test/elemHide.js
File test/elemHide.js (right):

https://codereview.adblockplus.org/29342884/diff/29342919/test/elemHide.js#ne...
test/elemHide.js:20: GLOBAL.Ci = { };
Nit: We usually write that as {}, without any spaces. Similarly elsewhere.

https://codereview.adblockplus.org/29342884/diff/29342919/test/elemHide.js#ne...
test/elemHide.js:22: import: function() { }
Nit: eslint doesn't like that, should have proper newlines:

  import: function()
  {
  }

https://codereview.adblockplus.org/29342884/diff/29342919/test/elemHide.js#ne...
test/elemHide.js:35: let removeFilter = fromText("remove");
Nit: I'd rather pass in the actual function: ElemHide.add and ElemHide.remove.

https://codereview.adblockplus.org/29342884/diff/29342919/test/elemHide.js#ne...
test/elemHide.js:39: test.deepEqual(ElemHide.getSelectorsForDomain(domain,
specificOnly).sort(),
On 2016/05/23 11:46:50, kzar wrote:
> Different implementations produce the selectors in different orders, but I
don't
> think that's important.

Yes, I was about to comment on this but you already fixed this issue.

https://codereview.adblockplus.org/29342884/diff/29342919/test/elemHide.js#ne...
test/elemHide.js:47: selectorsEqual("example.com", ["foo"]);
How about testing bar.example.com as well, to see that the exception doesn't
apply to it? Or even better, barfoo.example.com and bar.foo.example.com - the
former isn't subject to the exception whereas the latter is.

https://codereview.adblockplus.org/29342884/diff/29342919/test/elemHide.js#ne...
test/elemHide.js:121: selectorsEqual("", []);
This does a very thorough job testing interaction of filters with different
selectors. However, we should also test how multiple filters with the same
selector interact. For example:

~foo.example.com,example.com##foo + ##foo
~foo.example.com,example.com##foo + foo.example.com##foo
~foo.example.com,example.com##foo + example.org##foo
~foo.example.com,example.com##foo + ~example.com##foo

https://codereview.adblockplus.org/29342884/diff/29342919/test/elemHide.js#ne...
test/elemHide.js:125: // result here would be ["dupe"] instead of [].
This has nothing to do with performance - ElemHide component simply doesn't
handle duplicate adds/removes, other than making sure internal data structures
aren't messed up by them. There is an higher-level component taking care of
duplicates, FilterListener. The behavior below is correct.

[kzar, 2016-05-23 13:47:52]

Patch Set 4 : Addressed feedback

https://codereview.adblockplus.org/29342884/diff/29342919/test/elemHide.js
File test/elemHide.js (right):

https://codereview.adblockplus.org/29342884/diff/29342919/test/elemHide.js#ne...
test/elemHide.js:20: GLOBAL.Ci = { };
On 2016/05/23 13:23:02, Wladimir Palant wrote:
> Nit: We usually write that as {}, without any spaces. Similarly elsewhere.

Done.

https://codereview.adblockplus.org/29342884/diff/29342919/test/elemHide.js#ne...
test/elemHide.js:22: import: function() { }
On 2016/05/23 13:23:02, Wladimir Palant wrote:
> Nit: eslint doesn't like that, should have proper newlines:
> 
>   import: function()
>   {
>   }

Done.

https://codereview.adblockplus.org/29342884/diff/29342919/test/elemHide.js#ne...
test/elemHide.js:35: let removeFilter = fromText("remove");
On 2016/05/23 13:23:02, Wladimir Palant wrote:
> Nit: I'd rather pass in the actual function: ElemHide.add and ElemHide.remove.

Done.

https://codereview.adblockplus.org/29342884/diff/29342919/test/elemHide.js#ne...
test/elemHide.js:47: selectorsEqual("example.com", ["foo"]);
On 2016/05/23 13:23:01, Wladimir Palant wrote:
> How about testing http://bar.example.com as well, to see that the exception
doesn't
> apply to it? Or even better, http://barfoo.example.com and
http://bar.foo.example.com - the
> former isn't subject to the exception whereas the latter is.

Done.

https://codereview.adblockplus.org/29342884/diff/29342919/test/elemHide.js#ne...
test/elemHide.js:121: selectorsEqual("", []);
On 2016/05/23 13:23:01, Wladimir Palant wrote:
> This does a very thorough job testing interaction of filters with different
> selectors. However, we should also test how multiple filters with the same
> selector interact. For example:
> 
> ~foo.example.com,example.com##foo + ##foo
> ~foo.example.com,example.com##foo + foo.example.com##foo
> ~foo.example.com,example.com##foo + example.org##foo
> ~foo.example.com,example.com##foo + ~example.com##foo

Done.

https://codereview.adblockplus.org/29342884/diff/29342919/test/elemHide.js#ne...
test/elemHide.js:125: // result here would be ["dupe"] instead of [].
On 2016/05/23 13:23:02, Wladimir Palant wrote:
> This has nothing to do with performance - ElemHide component simply doesn't
> handle duplicate adds/removes, other than making sure internal data structures
> aren't messed up by them. There is an higher-level component taking care of
> duplicates, FilterListener. The behavior below is correct.

Acknowledged.

[Wladimir Palant, 2016-05-23 15:06:47]

https://codereview.adblockplus.org/29342884/diff/29342925/test/elemHide.js
File test/elemHide.js (right):

https://codereview.adblockplus.org/29342884/diff/29342925/test/elemHide.js#ne...
test/elemHide.js:138: selectorsEqual("example.com", ["foo", "foo"]);
Here you are cementing what is considerably a bug in the current implementation.
Instead, you should make sure that selectorsEqual will remove duplicates in
addition to sorting the list - and note in a comment that getSelectorsByDomain
is currently allowed to return duplicates for performance reasons.

[kzar, 2016-05-23 15:49:20]

Patch Set 5 : Properly test and document duplicate selector behaviour

https://codereview.adblockplus.org/29342884/diff/29342925/test/elemHide.js
File test/elemHide.js (right):

https://codereview.adblockplus.org/29342884/diff/29342925/test/elemHide.js#ne...
test/elemHide.js:138: selectorsEqual("example.com", ["foo", "foo"]);
On 2016/05/23 15:06:47, Wladimir Palant wrote:
> Here you are cementing what is considerably a bug in the current
implementation.
> Instead, you should make sure that selectorsEqual will remove duplicates in
> addition to sorting the list - and note in a comment that getSelectorsByDomain
> is currently allowed to return duplicates for performance reasons.

Done.

[Wladimir Palant, 2016-05-23 17:34:48]

https://codereview.adblockplus.org/29342884/diff/29342937/test/elemHide.js
File test/elemHide.js (right):

https://codereview.adblockplus.org/29342884/diff/29342937/test/elemHide.js#ne...
test/elemHide.js:47: });
How about a simpler filter function?

  (selector, index, array) =>
  {
    return index == 0 || selector != array[index - 1];
  }

https://codereview.adblockplus.org/29342884/diff/29342937/test/elemHide.js#ne...
test/elemHide.js:151: // comparison.
I think that this explanation belongs into normalizeSelectors() function -
that's where we remove duplicates and we should explain why we do it.

[kzar, 2016-05-23 18:56:27]

Patch Set 6 : Simplify filter function and move comment

https://codereview.adblockplus.org/29342884/diff/29342937/test/elemHide.js
File test/elemHide.js (right):

https://codereview.adblockplus.org/29342884/diff/29342937/test/elemHide.js#ne...
test/elemHide.js:47: });
On 2016/05/23 17:34:48, Wladimir Palant wrote:
> How about a simpler filter function?
> 
>   (selector, index, array) =>
>   {
>     return index == 0 || selector != array[index - 1];
>   }

Cool, didn't realise filter provided the index and array. Done.

https://codereview.adblockplus.org/29342884/diff/29342937/test/elemHide.js#ne...
test/elemHide.js:151: // comparison.
On 2016/05/23 17:34:48, Wladimir Palant wrote:
> I think that this explanation belongs into normalizeSelectors() function -
> that's where we remove duplicates and we should explain why we do it.

Done.

[Wladimir Palant, 2016-05-23 19:01:07]

LGTM