Issue 6439460933730304: Issue 616 - Add tests for $generichide and $genericblock (Closed)
|
Created:
March 19, 2015, 10:58 a.m. by kzar Modified:
Oct. 2, 2015, 4:06 p.m. CC:
Wladimir Palant, Sebastian Noack Visibility:
Public. |
DescriptionIssue 616 - Add tests for $generichide and $genericblock
Depends on these changes:
http://codereview.adblockplus.org/5840485868371968/
http://codereview.adblockplus.org/5991150368325632/
Patch Set 1 #
Total comments: 7
Patch Set 2 : Removed comment that I had forgotten about. #Patch Set 3 : Rebased onto typeMask changes #Patch Set 4 : Rebased onto Wladimir's test fixes and added test for genericblock sitekey interaction #Patch Set 5 : Fixed contentpolicy tests with recent changes to underlying logic #
Total comments: 8
Patch Set 6 : Addressed Felix's feedback #
Total comments: 2
Patch Set 7 : Slight improvement #
Total comments: 12
Patch Set 8 : Addressed The Professor's feedback! #Patch Set 9 : Couple more tweaks #
MessagesTotal messages: 18
Patch Set 1 http://codereview.adblockplus.org/6439460933730304/diff/5629499534213120/chro... File chrome/content/tests/matcher.js (right): http://codereview.adblockplus.org/6439460933730304/diff/5629499534213120/chro... chrome/content/tests/matcher.js:101: { Most of this change is down to the addition of the new specificOnly parameter to checkMatch. http://codereview.adblockplus.org/6439460933730304/diff/5629499534213120/chro... chrome/content/tests/matcher.js:165: checkMatch(["abc$sitekey=foo-publickey,domain=foo.com", "abc$sitekey=bar-publickey,domain=bar.com"], "http://abc/def", "IMAGE", "bar.com", false, "bar-publickey", false, "abc$sitekey=bar-publickey,domain=bar.com"); The new tests are below.
http://codereview.adblockplus.org/6439460933730304/diff/5629499534213120/chro... File chrome/content/tests/elemhide.js (right): http://codereview.adblockplus.org/6439460933730304/diff/5629499534213120/chro... chrome/content/tests/elemhide.js:151: if (filter.substr(0, 2) == "@@") How about using two seperate lists istead this ugly switch? http://codereview.adblockplus.org/6439460933730304/diff/5629499534213120/chro... chrome/content/tests/elemhide.js:152: defaultMatcher.add(Filter.fromText(filter));//exception_filters.push(filter); I suppose you forgot to remove that comment?
Patch Set 2 : Removed comment that I had forgotten about. http://codereview.adblockplus.org/6439460933730304/diff/5629499534213120/chro... File chrome/content/tests/elemhide.js (right): http://codereview.adblockplus.org/6439460933730304/diff/5629499534213120/chro... chrome/content/tests/elemhide.js:151: if (filter.substr(0, 2) == "@@") On 2015/03/19 11:58:49, Sebastian Noack wrote: > How about using two seperate lists istead this ugly switch? You're right the switch is a little ugly but the alternative of modifying all the existing test cases to add an empty array as an extra parameter seems worse. I prefer how I've done it. http://codereview.adblockplus.org/6439460933730304/diff/5629499534213120/chro... chrome/content/tests/elemhide.js:152: defaultMatcher.add(Filter.fromText(filter));//exception_filters.push(filter); On 2015/03/19 11:58:49, Sebastian Noack wrote: > I suppose you forgot to remove that comment? Done.
LGTM http://codereview.adblockplus.org/6439460933730304/diff/5629499534213120/chro... File chrome/content/tests/elemhide.js (right): http://codereview.adblockplus.org/6439460933730304/diff/5629499534213120/chro... chrome/content/tests/elemhide.js:151: if (filter.substr(0, 2) == "@@") On 2015/03/19 16:24:26, kzar wrote: > On 2015/03/19 11:58:49, Sebastian Noack wrote: > > How about using two seperate lists istead this ugly switch? > > You're right the switch is a little ugly but the alternative of modifying all > the existing test cases to add an empty array as an extra parameter seems worse. > I prefer how I've done it. You are right, this seems to be consistent with existing tests.
Patch Set 3 : Rebased onto typeMask changes
Patch Set 4 : Rebased onto Wladimir's test fixes and added test for genericblock sitekey interaction Sorry, for mixing up the new test with the rebase. (I forgot to push the rebase as a separate patch set and later rebased my new changes into my old ones. Doh...)
Patch Set 5 : Fixed contentpolicy tests with recent changes to underlying logic
LGTM
https://codereview.adblockplus.org/6439460933730304/diff/29325968/chrome/cont... File chrome/content/tests/elemhide.js (right): https://codereview.adblockplus.org/6439460933730304/diff/29325968/chrome/cont... chrome/content/tests/elemhide.js:151: if (filter.substr(0, 2) == "@@") How I understand it, what you want to check for here is whether the filter is NOT an element hiding rule, right? If so, I think it would make more sense to create the filter Object (you'll call `Filter.fromText(filter)` either way), then check if that one is an element hiding rule. https://codereview.adblockplus.org/6439460933730304/diff/29325968/chrome/cont... File chrome/content/tests/matcher.js (right): https://codereview.adblockplus.org/6439460933730304/diff/29325968/chrome/cont... chrome/content/tests/matcher.js:166: checkMatch(["@@kzar.co.uk$generichide"], "http://kzar.co.uk/dave", "GENERICHIDE", "kzar.co.uk", false, null, false, "@@kzar.co.uk$generichide"); Nit: I'd prefer to use the usual foo/bar flavoured samples here rather than kzar.co.uk/dave etc. here. Distracts from what the test is about IMHO. https://codereview.adblockplus.org/6439460933730304/diff/29325968/chrome/cont... chrome/content/tests/matcher.js:170: checkMatch(["###dave"], "http://kzar.co.uk/dave", "IMAGE", "kzar.co.uk", false, null, true, null); I don't really understand why this test and the three below are being added in the context of $generichide/$genericblock - how could those fail? https://codereview.adblockplus.org/6439460933730304/diff/29325968/chrome/cont... File chrome/content/tests/policy.js (right): https://codereview.adblockplus.org/6439460933730304/diff/29325968/chrome/cont... chrome/content/tests/policy.js:301: if (stage == 7) I think this block can be simplified: if (stage == 7) defaultMatcher.add(Filter.fromText(expectedURL + "$domain=127.0.0.1")); else if (stage > 1) defaultMatcher.add(Filter.fromText(expectedURL));
Patch Set 6 : Addressed Felix's feedback https://codereview.adblockplus.org/6439460933730304/diff/29325968/chrome/cont... File chrome/content/tests/elemhide.js (right): https://codereview.adblockplus.org/6439460933730304/diff/29325968/chrome/cont... chrome/content/tests/elemhide.js:151: if (filter.substr(0, 2) == "@@") On 2015/09/28 18:24:07, Felix Dahlke wrote: > How I understand it, what you want to check for here is whether the filter is > NOT an element hiding rule, right? If so, I think it would make more sense to > create the filter Object (you'll call `Filter.fromText(filter)` either way), > then check if that one is an element hiding rule. Done. https://codereview.adblockplus.org/6439460933730304/diff/29325968/chrome/cont... File chrome/content/tests/matcher.js (right): https://codereview.adblockplus.org/6439460933730304/diff/29325968/chrome/cont... chrome/content/tests/matcher.js:166: checkMatch(["@@kzar.co.uk$generichide"], "http://kzar.co.uk/dave", "GENERICHIDE", "kzar.co.uk", false, null, false, "@@kzar.co.uk$generichide"); On 2015/09/28 18:24:07, Felix Dahlke wrote: > Nit: I'd prefer to use the usual foo/bar flavoured samples here rather than > kzar.co.uk/dave etc. here. Distracts from what the test is about IMHO. Done. https://codereview.adblockplus.org/6439460933730304/diff/29325968/chrome/cont... chrome/content/tests/matcher.js:170: checkMatch(["###dave"], "http://kzar.co.uk/dave", "IMAGE", "kzar.co.uk", false, null, true, null); On 2015/09/28 18:24:07, Felix Dahlke wrote: > I don't really understand why this test and the three below are being added in > the context of $generichide/$genericblock - how could those fail? I wrote these tests in March so to be honest I can't exactly remember. That said it looks like I'm testing the specificOnly parameter of matcher.matchesAny, which is something I added to support the $generichide and block filter options. I don't think the parameter is tested anywhere else so I guess these four tests should be left in. https://codereview.adblockplus.org/6439460933730304/diff/29325968/chrome/cont... File chrome/content/tests/policy.js (right): https://codereview.adblockplus.org/6439460933730304/diff/29325968/chrome/cont... chrome/content/tests/policy.js:301: if (stage == 7) On 2015/09/28 18:24:07, Felix Dahlke wrote: > I think this block can be simplified: > > if (stage == 7) > defaultMatcher.add(Filter.fromText(expectedURL + "$domain=127.0.0.1")); > else if (stage > 1) > defaultMatcher.add(Filter.fromText(expectedURL)); Done.
Almost there, wouldn't even insist on that last change. https://codereview.adblockplus.org/6439460933730304/diff/29328689/chrome/cont... File chrome/content/tests/elemhide.js (right): https://codereview.adblockplus.org/6439460933730304/diff/29328689/chrome/cont... chrome/content/tests/elemhide.js:153: if (filter instanceof WhitelistFilter) What I (also) meant is that `!(filter instanceof ElemHideFilter)` seemed to be what you really want to check for here.
Patch Set 7 : Slight improvement https://codereview.adblockplus.org/6439460933730304/diff/29328689/chrome/cont... File chrome/content/tests/elemhide.js (right): https://codereview.adblockplus.org/6439460933730304/diff/29328689/chrome/cont... chrome/content/tests/elemhide.js:153: if (filter instanceof WhitelistFilter) On 2015/10/01 11:53:40, Felix Dahlke wrote: > What I (also) meant is that `!(filter instanceof ElemHideFilter)` seemed to be > what you really want to check for here. Ah I see, and good point. Unfortunately for reasons I don't understand it broke a few of the old tests. I've made a slight improvement here though, I mistakenly was still parsing the filter text again.
LGTM Sebastian is unavailable for the rest of this week and LGTMd an earlier patch set here, so feel free to move him to CC. If he sees something we can still fix it in a separate commit.
https://codereview.adblockplus.org/6439460933730304/diff/29328786/chrome/cont... File chrome/content/tests/elemhide.js (right): https://codereview.adblockplus.org/6439460933730304/diff/29328786/chrome/cont... chrome/content/tests/elemhide.js:110: [["#div(test1)", "@@localhost$generichide"], ["visible", "visible"]], The simplified element hiding filter syntax is deprecated so we should refrain from using it. See https://adblockplus.org/en/filters#elemhide_simplified https://codereview.adblockplus.org/6439460933730304/diff/29328786/chrome/cont... File chrome/content/tests/filterClasses.js (right): https://codereview.adblockplus.org/6439460933730304/diff/29328786/chrome/cont... chrome/content/tests/filterClasses.js:95: addProperty("contentType", 0x7FFFFFFF & ~(RegExpFilter.typeMap.DOCUMENT | RegExpFilter.typeMap.ELEMHIDE | RegExpFilter.typeMap.POPUP | Detail: If you think that the 80 characters rule should not be applied here, that's fine with me but the way it breaks now is suboptimal as shown below. How it looks like without line wraps: call(abc | def | ghi | jkl | mno); How it looks like in an environment limited to 80 characters (e.g. Rietveld): call(abc | def | ghi | jkl | mno); https://codereview.adblockplus.org/6439460933730304/diff/29328786/chrome/cont... File chrome/content/tests/matcher.js (right): https://codereview.adblockplus.org/6439460933730304/diff/29328786/chrome/cont... chrome/content/tests/matcher.js:54: filters = filters.filter((text) => text.substr(0, 2) != "@@").map((text) => "@@" + text); This line is now only run under the condition that `expected` is truthy which is different from what it was before. Doesn't that cause issues? https://codereview.adblockplus.org/6439460933730304/diff/29328786/chrome/cont... chrome/content/tests/matcher.js:55: if (expected) Detail: This if-statement is redundant. https://codereview.adblockplus.org/6439460933730304/diff/29328786/chrome/cont... chrome/content/tests/matcher.js:170: checkMatch(["###bar"], "http://foo.com/bar", "IMAGE", "foo.com", false, null, true, null); Shouldn't this be "ELEMHIDE" instead of "IMAGE"? https://codereview.adblockplus.org/6439460933730304/diff/29328786/chrome/cont... chrome/content/tests/matcher.js:171: checkMatch(["foo.com###bar"], "http://foo.com/bar", "IMAGE", "foo.com", false, null, true, null); From what I understand this should return a filter for foo.com that is specific but the expected result is `null`.
Patch Set 8 : Addressed The Professor's feedback! https://codereview.adblockplus.org/6439460933730304/diff/29328786/chrome/cont... File chrome/content/tests/elemhide.js (right): https://codereview.adblockplus.org/6439460933730304/diff/29328786/chrome/cont... chrome/content/tests/elemhide.js:110: [["#div(test1)", "@@localhost$generichide"], ["visible", "visible"]], On 2015/10/01 16:12:04, Thomas Greiner wrote: > The simplified element hiding filter syntax is deprecated so we should refrain > from using it. See https://adblockplus.org/en/filters#elemhide_simplified Done. https://codereview.adblockplus.org/6439460933730304/diff/29328786/chrome/cont... File chrome/content/tests/filterClasses.js (right): https://codereview.adblockplus.org/6439460933730304/diff/29328786/chrome/cont... chrome/content/tests/filterClasses.js:95: addProperty("contentType", 0x7FFFFFFF & ~(RegExpFilter.typeMap.DOCUMENT | RegExpFilter.typeMap.ELEMHIDE | RegExpFilter.typeMap.POPUP | On 2015/10/01 16:12:04, Thomas Greiner wrote: > Detail: If you think that the 80 characters rule should not be applied here, > that's fine with me but the way it breaks now is suboptimal as shown below. > > How it looks like without line wraps: > > call(abc | def | ghi | > jkl | mno); > > How it looks like in an environment limited to 80 characters (e.g. Rietveld): > > call(abc | > def | ghi | > jkl > | mno); Most of these test files are pretty bad when it comes to following the 80 character rule and often it's probably for the best. I agree with you here though, was no trouble to make it look a bit nicer. Done. https://codereview.adblockplus.org/6439460933730304/diff/29328786/chrome/cont... File chrome/content/tests/matcher.js (right): https://codereview.adblockplus.org/6439460933730304/diff/29328786/chrome/cont... chrome/content/tests/matcher.js:54: filters = filters.filter((text) => text.substr(0, 2) != "@@").map((text) => "@@" + text); On 2015/10/01 16:12:04, Thomas Greiner wrote: > This line is now only run under the condition that `expected` is truthy which is > different from what it was before. Doesn't that cause issues? Yes, well spotted, I don't like this logic at all. I have given it some more thought and re-written it. Done. https://codereview.adblockplus.org/6439460933730304/diff/29328786/chrome/cont... chrome/content/tests/matcher.js:55: if (expected) On 2015/10/01 16:12:05, Thomas Greiner wrote: > Detail: This if-statement is redundant. Done. https://codereview.adblockplus.org/6439460933730304/diff/29328786/chrome/cont... chrome/content/tests/matcher.js:171: checkMatch(["foo.com###bar"], "http://foo.com/bar", "IMAGE", "foo.com", false, null, true, null); On 2015/10/01 16:12:05, Thomas Greiner wrote: > From what I understand this should return a filter for http://foo.com that is specific > but the expected result is `null`. I don't think these tests _can_ work for element hiding. I think this mistake just masked the fact they weren't working. I've removed them.
Patch Set 9 : Couple more tweaks
LGTM https://codereview.adblockplus.org/6439460933730304/diff/29328786/chrome/cont... File chrome/content/tests/filterClasses.js (right): https://codereview.adblockplus.org/6439460933730304/diff/29328786/chrome/cont... chrome/content/tests/filterClasses.js:95: addProperty("contentType", 0x7FFFFFFF & ~(RegExpFilter.typeMap.DOCUMENT | RegExpFilter.typeMap.ELEMHIDE | RegExpFilter.typeMap.POPUP | On 2015/10/02 10:19:28, kzar wrote: > On 2015/10/01 16:12:04, Thomas Greiner wrote: > > Detail: If you think that the 80 characters rule should not be applied here, > > that's fine with me but the way it breaks now is suboptimal as shown below. > > > > How it looks like without line wraps: > > > > call(abc | def | ghi | > > jkl | mno); > > > > How it looks like in an environment limited to 80 characters (e.g. Rietveld): > > > > call(abc | > > def | ghi | > > jkl > > | mno); > > Most of these test files are pretty bad when it comes to following the 80 > character rule and often it's probably for the best. I agree with you here > though, was no trouble to make it look a bit nicer. Done. Thanks, I appreciate that.
LGTM 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
