Issue 4167 - getSelectorsForDomain criteria + keys

test/elemHide.js

Index: test/elemHide.js
diff --git a/test/elemHide.js b/test/elemHide.js
index b93baa4e4400ec3a923fb6ad515e874206128154..76d3827c3fe209c243ac9163898a10c6fe3e7a05 100644
--- a/test/elemHide.js
+++ b/test/elemHide.js
@@ -41,10 +41,13 @@ exports.testGetSelectorsForDomain = function(test)
       return index == 0 || selector != selectors[index -  1];
     });
   }
-  function selectorsEqual(domain, expectedSelectors, specificOnly)
+  function selectorsEqual(domain, expectedSelectors, specificOnly,
+                          noUnconditional, provideFilterKeys)
   {
     test.deepEqual(
-      normalizeSelectors(ElemHide.getSelectorsForDomain(domain, specificOnly)),
+      normalizeSelectors(ElemHide.getSelectorsForDomain(
+        domain, specificOnly, noUnconditional, provideFilterKeys
+      )),
       normalizeSelectors(expectedSelectors)
     );
   }
@@ -159,7 +162,37 @@ exports.testGetSelectorsForDomain = function(test)
   selectorsEqual("example.com", ["foo"]);
   selectorsEqual("com", ["foo"]);
   selectorsEqual("", ["foo"]);
-  removeFilter("example.org##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");
 
   test.done();
 };