Issue 2394 - Added unit tests for CSS property filters

chrome/content/tests/cssRules.js

Index: chrome/content/tests/cssRules.js
===================================================================
new file mode 100644
--- /dev/null
+++ b/chrome/content/tests/cssRules.js
@@ -0,0 +1,84 @@
+(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);
+
+    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 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`.

+  ];
+
+  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.

+  });
+
+  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);
+    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");
+
+    CSSRules.add(generic.filter);
+    CSSRules.add(domain.filter);
+    CSSRules.add(subdomain.filter);
+    CSSRules.add(otherDomain.filter);
+    compareRules("Return all matching filters", "www.example.com",
+      [generic.selector, domain.selector, subdomain.selector]);
+
+    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`.

+  });
+})();