Issue 2394 - Added unit tests for CSS property filters

chrome/content/tests/cssRules.js

 (function()
 {
   module("CSS property filter", {
     setup: function()
     {
       prepareFilterComponents.call(this);
       preparePrefs.call(this);
     },
     teardown: function()
     {
       restoreFilterComponents.call(this);
       restorePrefs.call(this);
     }
   });
 
   function runSelectorTest([text, domain, filters, expected])
   {
     for (let filter of filters)
     {
       filter = Filter.fromText(filter);
       if (filter instanceof CSSPropertyFilter)
         CSSRules.add(filter);
       else
         ElemHide.add(filter);

[Wladimir Palant, 2015/11/10 10:49:02]

Why add to ElemHide here if you are only testing CSSRules container?
FilterListener is tested elsewhere, no point trying to duplicate its
functionality here.

[Thomas Greiner, 2015/12/03 12:55:19]

Because exception rules are still handled by `ElemHide`.

     }
 
-    let selectors = CSSRules.getRulesForDomain(domain);

[Wladimir Palant, 2015/11/10 10:49:02]

Note that getRulesForDomain() should return filters, not selectors (discussed in
the other review).

[Thomas Greiner, 2015/12/03 12:55:19]

Done.

-    equal(selectors.sort().join("\n"), expected.sort().join("\n"), text);
+    let result = CSSRules.getRulesForDomain(domain)
+        .map((filter) => filter.text);
+    deepEqual(result.sort(), expected.sort(), text);
 
     CSSRules.clear();
     ElemHide.clear();

[Wladimir Palant, 2015/11/10 10:49:02]

You should add a CSSRules.clear() call to prepareFilterComponents() as well.

[Thomas Greiner, 2015/12/03 12:55:19]

I already did.

   }
 
   let selectorTests = [
-    ["Return unique selectors from single filter", "www.example.com", ["www.example.com,example.com##[-abp-properties='foo']"], ["[-abp-properties='foo']"]],
-    // ["Return unique selectors from multiple filters", "www.example.com", ["www.example.com##[-abp-properties='foo']", "other.example.org,www.example.com##[-abp-properties='foo']"], ["[-abp-properties='foo']"]],

[Thomas Greiner, 2015/08/25 16:11:47]

I commented this one out since our current implementation fails this test as
noted at
https://codereview.adblockplus.org/4529242486341632/diff/5676830073815040/lib....

-    ["Ignore selectors with exceptions", "example.com", ["example.com##[-abp-properties='foo']", "example.com##[-abp-properties='bar']", "example.com#@#[-abp-properties='foo']"], ["[-abp-properties='bar']"]],
+    ["Ignore generic filters", "example.com", ["##[-abp-properties='foo']", "example.com##[-abp-properties='foo']", "~example.com##[-abp-properties='foo']"], ["example.com##[-abp-properties='foo']"]],
+    ["Ignore selectors with exceptions", "example.com", ["example.com##[-abp-properties='foo']", "example.com##[-abp-properties='bar']", "example.com#@#[-abp-properties='foo']"], ["example.com##[-abp-properties='bar']"]],
     ["Ignore filters that include parent domain but exclude subdomain", "www.example.com", ["~www.example.com,example.com##[-abp-properties='foo']"], []],
-    ["Ignore filters with parent domain if exception matches subdomain", "www.example.com", ["www.example.com#@#[-abp-properties='foo']", "example.com##[-abp-properties='foo']"], []]

[Wladimir Palant, 2015/11/10 10:49:02]

I'd expect a test here verifying that generic rules aren't being added to
CSSRules.

[Thomas Greiner, 2015/12/03 12:55:20]

Done. Added a test for ignoring generic filters.

Note that we already check for invalid filters in filterClasses.js so this might
be redundant since invalid filters shouldn't be added in the first place.

In general, we could add a test that checks that invalid filters are not being
added to either `defaultMatcher`, `ElemHide` or `CSSRules`.

+    ["Ignore filters with parent domain if exception matches subdomain", "www.example.com", ["www.example.com#@#[-abp-properties='foo']", "example.com##[-abp-properties='foo']"], []],
+    ["Ignore filters for other subdomain", "other.example.com", ["www.example.com##[-abp-properties='foo']", "other.example.com##[-abp-properties='foo']"], ["other.example.com##[-abp-properties='foo']"]]
   ];
 
   test("Domain restrictions", function()
   {
-    for (let selectorTest of selectorTests)
-      runSelectorTest(selectorTest);

[Wladimir Palant, 2015/11/10 10:49:02]

selectorTests.forEach(runSelectorTest)?

[Thomas Greiner, 2015/12/03 12:55:20]

Done.

+    selectorTests.forEach(runSelectorTest);
   });
-
-  let testObjectCount = 0;
-  function constructTestObject(domain)
-  {
-    let selector = "filter_" + (++testObjectCount);
-    return {
-      filter: Filter.fromText(domain + "##" + selector),
-      selector: selector
-    };
-  }
 
   function compareRules(text, domain, expected)
   {
-    let result = CSSRules.getRulesForDomain(domain);
+    let result = CSSRules.getRulesForDomain(domain)
+        .map((filter) => filter.text);
+    expected = expected.map((filter) => filter.text);
     deepEqual(result.sort(), expected.sort(), text);
   }
 
   test("CSS property filters container", function()
   {
-    let generic = constructTestObject("");
-    let domain = constructTestObject("example.com");
-    let subdomain = constructTestObject("www.example.com");
-    let otherDomain = constructTestObject("other.example.com");
+    let domainFilter = Filter.fromText("example.com##filter1");
+    let subdomainFilter = Filter.fromText("www.example.com##filter2");
+    let otherDomainFilter = Filter.fromText("other.example.com##filter3");
 
-    CSSRules.add(generic.filter);
-    CSSRules.add(domain.filter);
-    CSSRules.add(subdomain.filter);
-    CSSRules.add(otherDomain.filter);
+    CSSRules.add(domainFilter);
+    CSSRules.add(subdomainFilter);
+    CSSRules.add(otherDomainFilter);
     compareRules("Return all matching filters", "www.example.com",
-      [generic.selector, domain.selector, subdomain.selector]);
+        [domainFilter, subdomainFilter]);
+
+    CSSRules.remove(domainFilter);
+    compareRules("Return all matching filters after removing one",
+        "www.example.com", [subdomainFilter]);
 
     CSSRules.clear();
-    CSSRules.add(subdomain.filter);
-    CSSRules.add(otherDomain.filter);
-    compareRules("Don't match other subdomain", "www.example.com",
-      [subdomain.selector]);

[Wladimir Palant, 2015/11/10 10:49:02]

This check makes little sense IMHO, it's already covered by the check above
(with four filters). Also, you shouldn't be testing with a generic filters:
CSSRules container doesn't support generic filters, so the behavior is
undefined. Frankly, I'm not sure how these container tests are necessary at all
given the domain restriction tests above. Maybe just add this scenario to
selectorTests?

[Thomas Greiner, 2015/12/03 12:55:20]

Done. Moved this to `selectorTests`.

Generally, these tests are unit tests for the `CSSRules` methods whereas the
ones in `selectorTests` are integration tests. Both will probably lead to the
same results but they don't necessarily have to.

Anyway, I added a couple more tests here since I realized that it wasn't
covering all aspects of the methods in `CSSRules`.

+    compareRules("Return no filters after clearing", "www.example.com", []);
   });
 })();