Issue 29324604: Issue 2394 - Added unit tests for CSS property filters (Closed)
|
Created:
Aug. 25, 2015, 3:51 p.m. by Thomas Greiner Modified:
Dec. 11, 2015, 10:43 a.m. Visibility:
Public. |
DescriptionIssue 2394 - Added unit tests for CSS property filters
Patch Set 1 #
Total comments: 21
Patch Set 2 : Rebased and adapted to review changes from #2392 and #2393 #
Total comments: 1
Patch Set 3 : #
Total comments: 4
Patch Set 4 : #
MessagesTotal messages: 9
https://codereview.adblockplus.org/29324604/diff/29324605/chrome/content/test... File chrome/content/tests/cssRules.js (right): https://codereview.adblockplus.org/29324604/diff/29324605/chrome/content/test... chrome/content/tests/cssRules.js:36: // ["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']"]], I commented this one out since our current implementation fails this test as noted at https://codereview.adblockplus.org/4529242486341632/diff/5676830073815040/lib....
https://codereview.adblockplus.org/29324604/diff/29324605/chrome/content/test... File chrome/content/tests/cssRules.js (right): https://codereview.adblockplus.org/29324604/diff/29324605/chrome/content/test... chrome/content/tests/cssRules.js:24: ElemHide.add(filter); Why add to ElemHide here if you are only testing CSSRules container? FilterListener is tested elsewhere, no point trying to duplicate its functionality here. https://codereview.adblockplus.org/29324604/diff/29324605/chrome/content/test... chrome/content/tests/cssRules.js:27: let selectors = CSSRules.getRulesForDomain(domain); Note that getRulesForDomain() should return filters, not selectors (discussed in the other review). https://codereview.adblockplus.org/29324604/diff/29324605/chrome/content/test... chrome/content/tests/cssRules.js:31: ElemHide.clear(); You should add a CSSRules.clear() call to prepareFilterComponents() as well. https://codereview.adblockplus.org/29324604/diff/29324605/chrome/content/test... chrome/content/tests/cssRules.js:39: ["Ignore filters with parent domain if exception matches subdomain", "www.example.com", ["www.example.com#@#[-abp-properties='foo']", "example.com##[-abp-properties='foo']"], []] I'd expect a test here verifying that generic rules aren't being added to CSSRules. https://codereview.adblockplus.org/29324604/diff/29324605/chrome/content/test... chrome/content/tests/cssRules.js:45: runSelectorTest(selectorTest); selectorTests.forEach(runSelectorTest)? https://codereview.adblockplus.org/29324604/diff/29324605/chrome/content/test... chrome/content/tests/cssRules.js:82: [subdomain.selector]); 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? https://codereview.adblockplus.org/29324604/diff/29324605/chrome/content/test... File chrome/content/tests/filterClasses.js (right): https://codereview.adblockplus.org/29324604/diff/29324605/chrome/content/test... chrome/content/tests/filterClasses.js:205: compareCSSRule("foo"); Why is foo##[-abp-properties='abc'] invalid? https://codereview.adblockplus.org/29324604/diff/29324605/chrome/content/test... chrome/content/tests/filterClasses.js:285: compareFilter("##[-abp-properties='']", ["type=elemhide", "text=##[-abp-properties='']", "selector=[-abp-properties='']"]); Wouldn't it make sense to have this filter in the "CSS property filters" test since you've added one? https://codereview.adblockplus.org/29324604/diff/29324605/chrome/content/test... chrome/content/tests/filterClasses.js:301: compareFilter("foo.com#@#[-abp-properties='abc']", ["type=elemhideexception", "text=foo.com#@#[-abp-properties='abc']", "selectorDomain=foo.com", "selector=[-abp-properties='abc']", "domains=FOO.COM"]); Wouldn't it make sense to have this filter in the "CSS property filters" test since you've added one? https://codereview.adblockplus.org/29324604/diff/29324605/chrome/content/test... chrome/content/tests/filterClasses.js:310: compareFilter("~foo.com,bar.com##[-abp-properties='abc']", ["type=cssrule", "text=~foo.com,bar.com##[-abp-properties='abc']", "selectorDomain=bar.com", "selector=[-abp-properties='abc']", "domains=BAR.COM|~FOO.COM", "prefix=", "regexp=abc", "suffix="]); You make prefix and suffix default to an empty string so specifying these in the expected result it unnecessary.
https://codereview.adblockplus.org/29324604/diff/29324605/chrome/content/test... File chrome/content/tests/cssRules.js (right): https://codereview.adblockplus.org/29324604/diff/29324605/chrome/content/test... chrome/content/tests/cssRules.js:24: ElemHide.add(filter); On 2015/11/10 10:49:02, Wladimir Palant wrote: > Why add to ElemHide here if you are only testing CSSRules container? > FilterListener is tested elsewhere, no point trying to duplicate its > functionality here. Because exception rules are still handled by `ElemHide`. https://codereview.adblockplus.org/29324604/diff/29324605/chrome/content/test... chrome/content/tests/cssRules.js:27: let selectors = CSSRules.getRulesForDomain(domain); On 2015/11/10 10:49:02, Wladimir Palant wrote: > Note that getRulesForDomain() should return filters, not selectors (discussed in > the other review). Done. https://codereview.adblockplus.org/29324604/diff/29324605/chrome/content/test... chrome/content/tests/cssRules.js:31: ElemHide.clear(); On 2015/11/10 10:49:02, Wladimir Palant wrote: > You should add a CSSRules.clear() call to prepareFilterComponents() as well. I already did. https://codereview.adblockplus.org/29324604/diff/29324605/chrome/content/test... chrome/content/tests/cssRules.js:39: ["Ignore filters with parent domain if exception matches subdomain", "www.example.com", ["www.example.com#@#[-abp-properties='foo']", "example.com##[-abp-properties='foo']"], []] On 2015/11/10 10:49:02, Wladimir Palant wrote: > I'd expect a test here verifying that generic rules aren't being added to > CSSRules. 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`. https://codereview.adblockplus.org/29324604/diff/29324605/chrome/content/test... chrome/content/tests/cssRules.js:45: runSelectorTest(selectorTest); On 2015/11/10 10:49:02, Wladimir Palant wrote: > selectorTests.forEach(runSelectorTest)? Done. https://codereview.adblockplus.org/29324604/diff/29324605/chrome/content/test... chrome/content/tests/cssRules.js:82: [subdomain.selector]); On 2015/11/10 10:49:02, Wladimir Palant wrote: > 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? 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`. https://codereview.adblockplus.org/29324604/diff/29324605/chrome/content/test... File chrome/content/tests/filterClasses.js (right): https://codereview.adblockplus.org/29324604/diff/29324605/chrome/content/test... chrome/content/tests/filterClasses.js:205: compareCSSRule("foo"); On 2015/11/10 10:49:03, Wladimir Palant wrote: > Why is foo##[-abp-properties='abc'] invalid? Considering domains with only one part (e.g. "com", "de") as generic was suggested by Sebastian in the filter class review (see https://codereview.adblockplus.org/5730585574113280/diff/5629499534213120/lib...). https://codereview.adblockplus.org/29324604/diff/29324605/chrome/content/test... chrome/content/tests/filterClasses.js:285: compareFilter("##[-abp-properties='']", ["type=elemhide", "text=##[-abp-properties='']", "selector=[-abp-properties='']"]); On 2015/11/10 10:49:03, Wladimir Palant wrote: > Wouldn't it make sense to have this filter in the "CSS property filters" test > since you've added one? Done. https://codereview.adblockplus.org/29324604/diff/29324605/chrome/content/test... chrome/content/tests/filterClasses.js:301: compareFilter("foo.com#@#[-abp-properties='abc']", ["type=elemhideexception", "text=foo.com#@#[-abp-properties='abc']", "selectorDomain=foo.com", "selector=[-abp-properties='abc']", "domains=FOO.COM"]); On 2015/11/10 10:49:02, Wladimir Palant wrote: > Wouldn't it make sense to have this filter in the "CSS property filters" test > since you've added one? Done. https://codereview.adblockplus.org/29324604/diff/29324605/chrome/content/test... chrome/content/tests/filterClasses.js:310: compareFilter("~foo.com,bar.com##[-abp-properties='abc']", ["type=cssrule", "text=~foo.com,bar.com##[-abp-properties='abc']", "selectorDomain=bar.com", "selector=[-abp-properties='abc']", "domains=BAR.COM|~FOO.COM", "prefix=", "regexp=abc", "suffix="]); On 2015/11/10 10:49:03, Wladimir Palant wrote: > You make prefix and suffix default to an empty string so specifying these in the > expected result it unnecessary. Done. https://codereview.adblockplus.org/29324604/diff/29330830/chrome/content/test... File chrome/content/tests/cssRules.js (right): https://codereview.adblockplus.org/29324604/diff/29330830/chrome/content/test... chrome/content/tests/cssRules.js:37: // ["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']"]], Removed these ones because they can no longer apply since we switched from comparing selectors to comparing filters.
https://codereview.adblockplus.org/29324604/diff/29331838/chrome/content/test... File chrome/content/tests/cssRules.js (right): https://codereview.adblockplus.org/29324604/diff/29331838/chrome/content/test... chrome/content/tests/cssRules.js:58: let genericFilter = Filter.fromText("##filter1"); This comment hasn't been addressed from the previous review: > Also, you shouldn't be testing with a generic filters: CSSRules container doesn't support generic filters, so the behavior is undefined. We shouldn't be testing implementation details. https://codereview.adblockplus.org/29324604/diff/29331838/chrome/content/test... File chrome/content/tests/filterListener.js (right): https://codereview.adblockplus.org/29324604/diff/29331838/chrome/content/test... chrome/content/tests/filterListener.js:62: result.cssrule.push(filter.text); This is an unnecessary roundtrip - result.cssrule.push(key) will do.
I've just given patch set 3 a try with adblockpluschrome for the integration and I'm seeing a few test failures. It appears with some of the "Filter classes: Invalid filters" tests for CSS property filters we're expecting a prefix of "hasReason" but it's not present. https://gist.github.com/kzar/69dcad68704f1186cda5
On 2015/12/04 13:45:29, kzar wrote: > I've just given patch set 3 a try with adblockpluschrome for the integration and > I'm seeing a few test failures. It appears with some of the "Filter classes: > Invalid filters" tests for CSS property filters we're expecting a prefix of > "hasReason" but it's not present. > > https://gist.github.com/kzar/69dcad68704f1186cda5 The reason was specified in adblockplus/lib/filterClasses.js as `Utils.getString("filter_cssproperty_nodomain")` so I'd have to look into what causes this in adblockpluschrome since it works when running the tests in Firefox.
@kzar The failing tests in adblockpluschrome are caused by the error message string "filter_cssproperty_nodomain" not being imported from adblockplus/chrome/content/locale/*/global.properties. https://codereview.adblockplus.org/29324604/diff/29331838/chrome/content/test... File chrome/content/tests/cssRules.js (right): https://codereview.adblockplus.org/29324604/diff/29331838/chrome/content/test... chrome/content/tests/cssRules.js:58: let genericFilter = Filter.fromText("##filter1"); On 2015/12/04 13:09:48, Wladimir Palant wrote: > This comment hasn't been addressed from the previous review: > > > Also, you shouldn't be testing with a generic filters: CSSRules container > doesn't support generic filters, so the behavior is undefined. > > We shouldn't be testing implementation details. Done. https://codereview.adblockplus.org/29324604/diff/29331838/chrome/content/test... File chrome/content/tests/filterListener.js (right): https://codereview.adblockplus.org/29324604/diff/29331838/chrome/content/test... chrome/content/tests/filterListener.js:62: result.cssrule.push(filter.text); On 2015/12/04 13:09:48, Wladimir Palant wrote: > This is an unnecessary roundtrip - result.cssrule.push(key) will do. Done.
On 2015/12/07 17:29:27, Thomas Greiner wrote: > @kzar The failing tests in adblockpluschrome are caused by the error message > string "filter_cssproperty_nodomain" not being imported from > adblockplus/chrome/content/locale/*/global.properties. Ahh, I see. Just tested them again and they're all passing now, thanks for the tip.
LGTM 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
