Issue 4167 - getSelectorsForDomain criteria + keys

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
@@ skipping 23 matching lines @@
 
   function normalizeSelectors(selectors)
   {
     // 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,
-                          noUnconditional, provideFilterKeys)
+  function testResult(domain, expectedSelectors, criteria)
   {
+    let normalizedExpectedSelectors = normalizeSelectors(expectedSelectors);
+
+    // Test without filter keys
     test.deepEqual(
-      normalizeSelectors(ElemHide.getSelectorsForDomain(
-        domain, specificOnly, noUnconditional, provideFilterKeys
-      )),
-      normalizeSelectors(expectedSelectors)
+      normalizeSelectors(ElemHide.getSelectorsForDomain(domain, criteria)),
+      normalizedExpectedSelectors
     );
+
+    // With filter keys
+    let [selectors, filterKeys] = ElemHide.getSelectorsForDomain(domain, criteria,
+                                                                 true);
+    test.deepEqual(filterKeys.map(k => ElemHide.getFilterByKey(k).selector),
+                   selectors);
+    test.deepEqual(normalizeSelectors(selectors), normalizedExpectedSelectors);
   }
 
-  selectorsEqual("", []);
+  testResult("", []);
 
   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"]);
-  selectorsEqual("com", []);
-  selectorsEqual("", []);
+  testResult("barfoo.example.com", ["foo"]);
+  testResult("bar.foo.example.com", []);
+  testResult("foo.example.com", []);
+  testResult("example.com", ["foo"]);
+  testResult("com", []);
+  testResult("", []);
 
   addFilter("foo.example.com##turnip");
-  selectorsEqual("foo.example.com", ["turnip"]);
-  selectorsEqual("example.com", ["foo"]);
-  selectorsEqual("com", []);
-  selectorsEqual("", []);
+  testResult("foo.example.com", ["turnip"]);
+  testResult("example.com", ["foo"]);
+  testResult("com", []);
+  testResult("", []);
 
   addFilter("example.com#@#foo");
-  selectorsEqual("foo.example.com", ["turnip"]);
-  selectorsEqual("example.com", []);
-  selectorsEqual("com", []);
-  selectorsEqual("", []);
+  testResult("foo.example.com", ["turnip"]);
+  testResult("example.com", []);
+  testResult("com", []);
+  testResult("", []);
 
   addFilter("com##bar");
-  selectorsEqual("foo.example.com", ["turnip", "bar"]);
-  selectorsEqual("example.com", ["bar"]);
-  selectorsEqual("com", ["bar"]);
-  selectorsEqual("", []);
+  testResult("foo.example.com", ["turnip", "bar"]);
+  testResult("example.com", ["bar"]);
+  testResult("com", ["bar"]);
+  testResult("", []);
 
   addFilter("example.com#@#bar");
-  selectorsEqual("foo.example.com", ["turnip"]);
-  selectorsEqual("example.com", []);
-  selectorsEqual("com", ["bar"]);
-  selectorsEqual("", []);
+  testResult("foo.example.com", ["turnip"]);
+  testResult("example.com", []);
+  testResult("com", ["bar"]);
+  testResult("", []);
 
   removeFilter("example.com#@#foo");
-  selectorsEqual("foo.example.com", ["turnip"]);
-  selectorsEqual("example.com", ["foo"]);
-  selectorsEqual("com", ["bar"]);
-  selectorsEqual("", []);
+  testResult("foo.example.com", ["turnip"]);
+  testResult("example.com", ["foo"]);
+  testResult("com", ["bar"]);
+  testResult("", []);
 
   removeFilter("example.com#@#bar");
-  selectorsEqual("foo.example.com", ["turnip", "bar"]);
-  selectorsEqual("example.com", ["foo", "bar"]);
-  selectorsEqual("com", ["bar"]);
-  selectorsEqual("", []);
+  testResult("foo.example.com", ["turnip", "bar"]);
+  testResult("example.com", ["foo", "bar"]);
+  testResult("com", ["bar"]);
+  testResult("", []);
 
   addFilter("##generic");
-  selectorsEqual("foo.example.com", ["turnip", "bar", "generic"]);
-  selectorsEqual("example.com", ["foo", "bar", "generic"]);
-  selectorsEqual("com", ["bar", "generic"]);
-  selectorsEqual("", ["generic"]);
-  selectorsEqual("foo.example.com", ["turnip", "bar"], true);
-  selectorsEqual("example.com", ["foo", "bar"], true);
-  selectorsEqual("com", ["bar"], true);
-  selectorsEqual("", [], true);
+  testResult("foo.example.com", ["turnip", "bar", "generic"]);
+  testResult("example.com", ["foo", "bar", "generic"]);
+  testResult("com", ["bar", "generic"]);
+  testResult("", ["generic"]);
+  testResult("foo.example.com", ["turnip", "bar"], ElemHide.SPECIFIC_ONLY);
+  testResult("example.com", ["foo", "bar"], ElemHide.SPECIFIC_ONLY);
+  testResult("com", ["bar"], ElemHide.SPECIFIC_ONLY);
+  testResult("", [], ElemHide.SPECIFIC_ONLY);
   removeFilter("##generic");
 
   addFilter("~adblockplus.org##example");
-  selectorsEqual("adblockplus.org", []);
-  selectorsEqual("", ["example"]);
-  selectorsEqual("foo.example.com", ["turnip", "bar", "example"]);
-  selectorsEqual("foo.example.com", ["turnip", "bar"], true);
+  testResult("adblockplus.org", []);
+  testResult("", ["example"]);
+  testResult("foo.example.com", ["turnip", "bar", "example"]);
+  testResult("foo.example.com", ["turnip", "bar"], ElemHide.SPECIFIC_ONLY);
   removeFilter("~adblockplus.org##example");
 
   removeFilter("~foo.example.com,example.com##foo");
-  selectorsEqual("foo.example.com", ["turnip", "bar"]);
-  selectorsEqual("example.com", ["bar"]);
-  selectorsEqual("com", ["bar"]);
-  selectorsEqual("", []);
+  testResult("foo.example.com", ["turnip", "bar"]);
+  testResult("example.com", ["bar"]);
+  testResult("com", ["bar"]);
+  testResult("", []);
 
   removeFilter("com##bar");
-  selectorsEqual("foo.example.com", ["turnip"]);
-  selectorsEqual("example.com", []);
-  selectorsEqual("com", []);
-  selectorsEqual("", []);
+  testResult("foo.example.com", ["turnip"]);
+  testResult("example.com", []);
+  testResult("com", []);
+  testResult("", []);
 
   removeFilter("foo.example.com##turnip");
-  selectorsEqual("foo.example.com", []);
-  selectorsEqual("example.com", []);
-  selectorsEqual("com", []);
-  selectorsEqual("", []);
+  testResult("foo.example.com", []);
+  testResult("example.com", []);
+  testResult("com", []);
+  testResult("", []);
 
   addFilter("example.com##dupe");
   addFilter("example.com##dupe");
-  selectorsEqual("example.com", ["dupe"]);
+  testResult("example.com", ["dupe"]);
   removeFilter("example.com##dupe");
-  selectorsEqual("example.com", []);
+  testResult("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"]);
+  testResult("foo.example.com", ["foo"]);
+  testResult("example.com", ["foo"]);
+  testResult("com", ["foo"]);
+  testResult("", ["foo"]);
   removeFilter("##foo");
 
   addFilter("example.org##foo");
-  selectorsEqual("foo.example.com", []);
-  selectorsEqual("example.com", ["foo"]);
-  selectorsEqual("com", []);
-  selectorsEqual("", []);
+  testResult("foo.example.com", []);
+  testResult("example.com", ["foo"]);
+  testResult("com", []);
+  testResult("", []);
   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 specificOnly and noUnconditional
-  addFilter("~example.com##foo");

[Wladimir Palant, 2016/09/20 10:36:07]

Please add at least one unconditional filter, otherwise you aren't really
testing noUnconditional. Ideally, you should also test that two unconditional
filters with the same selector don't result in the selector being duplicated,
and that removing one of the two selectors still leaves the other one intact.

[kzar, 2016/09/20 14:23:48]

Done.

-  selectorsEqual("foo.com", [], true, false);
-  selectorsEqual("foo.com", ["foo"], false, true);

[Wladimir Palant, 2016/09/20 10:36:08]

You are not testing the scenario where both specificOnly and noUnconditional are
true, this implies that it's an invalid combination. I don't think we want
implicit assumptions here, so it's probably better to replace these two
parameters by a single numerical parameter:

  ALL_MATCHING = 0;
  NO_UNCONDITIONAL = 1;
  SPECIFIC_ONLY = 2;

Note that the set supposed to be returned is getting smaller with higher values,
so you can use comparison operators:

  if (criteria < NO_UNCONDITIONAL)
    selectors = unconditionalSelectors;

[kzar, 2016/09/20 14:23:49]

Done. (I use slightly different logic for the checks since the parameter is
optional.)

-  addFilter("foo.com##foo");
-  selectorsEqual("foo.com", ["foo"], true, false);
-  selectorsEqual("foo.com", ["foo"], false, true);
-  removeFilter("foo.com##foo");
-  removeFilter("~example.org##foo");
-
-  // Test provideFilterKeys
-  addFilter("~foo.com##nope");
-  addFilter("##foo");
-  addFilter("##bar");
-  addFilter("##hello");
-  addFilter("##world");
-  let selectorsWithKeys = ElemHide.getSelectorsForDomain("foo.com", false,

[Wladimir Palant, 2016/09/20 10:36:08]

How about:

  let [selectors, filterKeys] = ...

[kzar, 2016/09/20 14:23:48]

Done.

-                                                         false, true);
-  let selectors = selectorsWithKeys[0];
-  let filterKeys = selectorsWithKeys[1];
-  test.deepEqual(filterKeys.map(k => ElemHide.getFilterByKey(k).selector),
-                 selectors);
-  test.deepEqual(normalizeSelectors(selectors),
-                 normalizeSelectors(["bar", "foo", "hello", "world"]));

[Wladimir Palant, 2016/09/20 10:36:08]

One-time correctness test is definitely not sufficient here, we need to verify
that result stays correct on modification as well. For example:

  add bar.com##foo
  remove bar.com##foo
  add ,##hello
  remove ##hello
  remove ,##hello
  add ~bar.com##nope
  remove ~bar.com##nope

I didn't verify that these operations will really trigger all the code branches
but it's a start.

[kzar, 2016/09/20 14:23:48]

I've made it so the filter keys are tested for all the other tests too. Also
added some more advanced tests below.

-  removeFilter("##world");
-  removeFilter("##hello");
-  removeFilter("##bar");
-  removeFilter("##foo");
-  removeFilter("~foo.com##nope");
+  testResult("foo.example.com", []);
+  testResult("example.com", ["foo"]);
+  testResult("com", ["foo"]);
+  testResult("", ["foo"]);
+  removeFilter("~example.com##foo");
+
+  removeFilter("~foo.example.com,example.com##foo");
+
+  // Test criteria
+  addFilter("##hello");
+  addFilter("~example.com##world");
+  addFilter("foo.com##specific");
+  testResult("foo.com", ["specific"], ElemHide.SPECIFIC_ONLY);
+  testResult("foo.com", ["specific", "world"], ElemHide.NO_UNCONDITIONAL);
+  testResult("foo.com", ["hello", "specific", "world"], ElemHide.ALL_MATCHING);
+  testResult("foo.com", ["hello", "specific", "world"]);
+  removeFilter("foo.com##specific");
+  removeFilter("~example.com##world");
+  removeFilter("##hello");
+  testResult("foo.com", []);
+
+  addFilter("##hello");
+  testResult("foo.com", [], ElemHide.SPECIFIC_ONLY);
+  testResult("foo.com", [], ElemHide.NO_UNCONDITIONAL);
+  testResult("foo.com", ["hello"], ElemHide.ALL_MATCHING);
+  testResult("foo.com", ["hello"]);
+  testResult("bar.com", [], ElemHide.SPECIFIC_ONLY);
+  testResult("bar.com", [], ElemHide.NO_UNCONDITIONAL);
+  testResult("bar.com", ["hello"], ElemHide.ALL_MATCHING);
+  testResult("bar.com", ["hello"]);
+  addFilter("foo.com#@#hello");
+  testResult("foo.com", [], ElemHide.SPECIFIC_ONLY);
+  testResult("foo.com", [], ElemHide.NO_UNCONDITIONAL);
+  testResult("foo.com", [], ElemHide.ALL_MATCHING);
+  testResult("foo.com", []);
+  testResult("bar.com", [], ElemHide.SPECIFIC_ONLY);
+  testResult("bar.com", ["hello"], ElemHide.NO_UNCONDITIONAL);
+  testResult("bar.com", ["hello"], ElemHide.ALL_MATCHING);
+  testResult("bar.com", ["hello"]);
+  removeFilter("foo.com#@#hello");
+  testResult("foo.com", [], ElemHide.SPECIFIC_ONLY);
+  // Note: We don't take care to track conditional selectors which became
+  //       unconditional when a filter was removed. This was too expensive.
+  //testResult("foo.com", [], ElemHide.NO_UNCONDITIONAL);
+  testResult("foo.com", ["hello"], ElemHide.ALL_MATCHING);
+  testResult("foo.com", ["hello"]);
+  testResult("bar.com", [], ElemHide.SPECIFIC_ONLY);
+  testResult("bar.com", ["hello"], ElemHide.NO_UNCONDITIONAL);
+  testResult("bar.com", ["hello"], ElemHide.ALL_MATCHING);
+  testResult("bar.com", ["hello"]);
+  removeFilter("##hello");
+  testResult("foo.com", []);
+  testResult("bar.com", []);
+
+  addFilter("##hello");
+  addFilter("foo.com##hello");
+  testResult("foo.com", ["hello"]);
+  removeFilter("foo.com##hello");
+  testResult("foo.com", ["hello"]);
+  removeFilter("##hello");
+  testResult("foo.com", []);
+
+  addFilter("##hello");
+  addFilter("foo.com##hello");
+  testResult("foo.com", ["hello"]);
+  removeFilter("##hello");
+  testResult("foo.com", ["hello"]);
+  removeFilter("foo.com##hello");
+  testResult("foo.com", []);
+
+  // Advanced filter keys test
+  testResult("", []);
+  addFilter("##dupe");
+  addFilter(",,##dupe");
+  addFilter(",,,##dupe");
+  addFilter("foo.com##dupe");
+  testResult("", ["dupe"]);
+  removeFilter(",,,##dupe");
+  testResult("", ["dupe"]);
+  removeFilter("foo.com##dupe");
+  testResult("", ["dupe"]);
+  removeFilter(",,##dupe");
+  testResult("", ["dupe"]);
+  removeFilter("##dupe");
+  testResult("", []);
 
   test.done();
 };