Issue 4055 - Test ElemHide.getSelectorsFordomain

test/elemHide.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2016 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 "use strict";
 
-GLOBAL.Ci = { };

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

Nit: We usually write that as {}, without any spaces. Similarly elsewhere.

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

Done.

+GLOBAL.Ci = {};
 GLOBAL.Cu = {
-  import: function() { }

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

Nit: eslint doesn't like that, should have proper newlines:

  import: function()
  {
  }

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

Done.

+  import: function()
+  {
+  }
 };
 
 let {ElemHide} = require("elemHide");
 let {Filter} = require("filterClasses");
 
 exports.testGetSelectorsForDomain = function(test)
 {
   function fromText(f)
   {
-    return (filterText) => ElemHide[f](Filter.fromText(filterText));
+    return (filterText) => f(Filter.fromText(filterText));
   }
-  let addFilter = fromText("add");
-  let removeFilter = fromText("remove");

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

Nit: I'd rather pass in the actual function: ElemHide.add and ElemHide.remove.

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

Done.

+  let addFilter = fromText(ElemHide.add);
+  let removeFilter = fromText(ElemHide.remove);
 
-  function selectorsEqual(domain, selectors, specificOnly)
+  function normalizeSelectors(selectors)
   {
-    test.deepEqual(ElemHide.getSelectorsForDomain(domain, specificOnly).sort(),

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

Different implementations produce the selectors in different orders, but I don't
think that's important.

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

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

-                   selectors.sort());
+    // getSelectorsForDomain is currently allowed to return duplicate selectors
+    // for performance reasons, so we need to remove duplicates here.
+    return selectors.sort().filter((selector, index, selectors) =>
+    {
+      return index == 0 || selector != selectors[index -  1];
+    });
+  }
+  function selectorsEqual(domain, expectedSelectors, specificOnly)
+  {
+    test.deepEqual(
+      normalizeSelectors(ElemHide.getSelectorsForDomain(domain, specificOnly)),
+      normalizeSelectors(expectedSelectors)
+    );
   }
 
   selectorsEqual("", []);
 
   addFilter("~foo.example.com,example.com##foo");
+  selectorsEqual("barfoo.example.com", ["foo"]);
+  selectorsEqual("bar.foo.example.com", []);
   selectorsEqual("foo.example.com", []);
   selectorsEqual("example.com", ["foo"]);

[Wladimir Palant, 2016/05/23 13:23:01]

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.

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

Done.

   selectorsEqual("com", []);
   selectorsEqual("", []);
 
   addFilter("foo.example.com##turnip");
   selectorsEqual("foo.example.com", ["turnip"]);
   selectorsEqual("example.com", ["foo"]);
   selectorsEqual("com", []);
   selectorsEqual("", []);
 
   addFilter("example.com#@#foo");
@@ skipping 53 matching lines @@
   removeFilter("com##bar");
   selectorsEqual("foo.example.com", ["turnip"]);
   selectorsEqual("example.com", []);
   selectorsEqual("com", []);
   selectorsEqual("", []);
 
   removeFilter("foo.example.com##turnip");
   selectorsEqual("foo.example.com", []);
   selectorsEqual("example.com", []);
   selectorsEqual("com", []);
   selectorsEqual("", []);

[Wladimir Palant, 2016/05/23 13:23:01]

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

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

Done.

 
-  // As a consession for performance we don't handle the case of more than one
-  // identical rule being added, then one of them removed. Ideally the final
-  // result here would be ["dupe"] instead of [].

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

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]

Acknowledged.

   addFilter("example.com##dupe");
   addFilter("example.com##dupe");
   selectorsEqual("example.com", ["dupe"]);
   removeFilter("example.com##dupe");
   selectorsEqual("example.com", []);
+  removeFilter("example.com##dupe");
+
+  addFilter("~foo.example.com,example.com##foo");
+
+  addFilter("##foo");
+  selectorsEqual("foo.example.com", ["foo"]);
+  selectorsEqual("example.com", ["foo"]);
+  selectorsEqual("com", ["foo"]);
+  selectorsEqual("", ["foo"]);
+  removeFilter("##foo");
+
+  addFilter("example.org##foo");
+  selectorsEqual("foo.example.com", []);
+  selectorsEqual("example.com", ["foo"]);
+  selectorsEqual("com", []);
+  selectorsEqual("", []);
+  removeFilter("example.org##foo");
+
+  addFilter("~example.com##foo");
+  selectorsEqual("foo.example.com", []);
+  selectorsEqual("example.com", ["foo"]);
+  selectorsEqual("com", ["foo"]);
+  selectorsEqual("", ["foo"]);
+  removeFilter("example.org##foo");
 
   test.done();
 };