Issue 30035555: Issue 7428, 7400 - Add test case for nested :-abp-properties() (Closed)
|
Created:
April 1, 2019, 8:46 p.m. by hub Modified:
April 3, 2019, 8:48 p.m. Reviewers:
Manish Jethani Base URL:
https://hg.adblockplus.org/adblockpluscore/ Visibility:
Public. |
DescriptionIssue 7428, 7400 - Add test case for nested :-abp-properties()
Patch Set 1 #
Total comments: 2
Patch Set 2 : Move test.done() out of runTest #
Total comments: 16
Patch Set 3 : Updated patch #
Total comments: 2
Patch Set 4 : Indent HTML #Patch Set 5 : Rebase #
Total comments: 2
Patch Set 6 : Added test for 7400 #Patch Set 7 : Remove span from test case #
Total comments: 5
Patch Set 8 : Moved comments #MessagesTotal messages: 21
https://codereview.adblockplus.org/30035555/diff/30035556/test/browser/elemHi... File test/browser/elemHideEmulation.js (right): https://codereview.adblockplus.org/30035555/diff/30035556/test/browser/elemHi... test/browser/elemHideEmulation.js:567: runTestCombinatorQualifier( Can this (and possibly more tests in the future) be included in `testPropertySelectorCombinatorQualifier()` and `test.done()` be moved out of the `runTestCombinatorQualifier()` function?
I have closed #7359 as a duplicate so let's assign this patch to 7400 and 7428 instead.
On 2019/04/02 07:02:28, Manish Jethani wrote: > I have closed #7359 as a duplicate so let's assign this patch to 7400 and 7428 > instead. Alternatively, if you are interested, you could file a new issue just for adding the test case. BTW I would be a bit more specific in the summary than "Add test case". e.g. "Add test case for nested :-abp-properties()`
https://codereview.adblockplus.org/30035555/diff/30035556/test/browser/elemHi... File test/browser/elemHideEmulation.js (right): https://codereview.adblockplus.org/30035555/diff/30035556/test/browser/elemHi... test/browser/elemHideEmulation.js:567: runTestCombinatorQualifier( On 2019/04/02 06:29:10, Manish Jethani wrote: > Can this (and possibly more tests in the future) be included in > `testPropertySelectorCombinatorQualifier()` and `test.done()` be moved out of > the `runTestCombinatorQualifier()` function? Done
https://codereview.adblockplus.org/30035555/diff/30036555/test/browser/elemHi... File test/browser/elemHideEmulation.js (right): https://codereview.adblockplus.org/30035555/diff/30036555/test/browser/elemHi... test/browser/elemHideEmulation.js:532: // See issue https://issues.adblockplus.org/ticket/7359 Shouldn't this be 7400? https://codereview.adblockplus.org/30035555/diff/30036555/test/browser/elemHi... test/browser/elemHideEmulation.js:534: async function runTestCombinatorQualifier(test, selector) I think what you mean is `runTestCompoundQualifier()`. It does not need to have combinators. In fact, it does even need to be a compound qualifier, so we can just call it `runTestQualifier()` and use to test all kinds of cases where the selector in the document must be qualified. https://codereview.adblockplus.org/30035555/diff/30036555/test/browser/elemHi... test/browser/elemHideEmulation.js:543: <a> Nit: It is more natural if `<a>` occurs with the `<p>` rather than the other way around. https://codereview.adblockplus.org/30035555/diff/30036555/test/browser/elemHi... test/browser/elemHideEmulation.js:551: { Nit: No need for the braces here. https://codereview.adblockplus.org/30035555/diff/30036555/test/browser/elemHi... test/browser/elemHideEmulation.js:563: test.done(); My reason for asking to move `test.done()` here was so we could combine everything into one test function, something like `exports.testPropertySelectorQualification`. I can see merits of keeping them separate also, so it's your call, but then I think we should name them more appropriately then suffixing with 2, 3, and so on. I'm OK if we keep one function and simply use comments to explain each case.
updated patch https://codereview.adblockplus.org/30035555/diff/30036555/test/browser/elemHi... File test/browser/elemHideEmulation.js (right): https://codereview.adblockplus.org/30035555/diff/30036555/test/browser/elemHi... test/browser/elemHideEmulation.js:532: // See issue https://issues.adblockplus.org/ticket/7359 On 2019/04/03 11:15:58, Manish Jethani wrote: > Shouldn't this be 7400? The test is actually all based on issue #7359. It was even written before the other bugs were filed. I don't think this even test what #7400 fixes. https://codereview.adblockplus.org/30035555/diff/30036555/test/browser/elemHi... test/browser/elemHideEmulation.js:534: async function runTestCombinatorQualifier(test, selector) On 2019/04/03 11:15:58, Manish Jethani wrote: > I think what you mean is `runTestCompoundQualifier()`. It does not need to have > combinators. In fact, it does even need to be a compound qualifier, so we can > just call it `runTestQualifier()` and use to test all kinds of cases where the > selector in the document must be qualified. Done. https://codereview.adblockplus.org/30035555/diff/30036555/test/browser/elemHi... test/browser/elemHideEmulation.js:543: <a> On 2019/04/03 11:15:58, Manish Jethani wrote: > Nit: It is more natural if `<a>` occurs with the `<p>` rather than the other way > around. if everything was done in a "natural way", lot of the stuff we do wouldn't be needed. https://codereview.adblockplus.org/30035555/diff/30036555/test/browser/elemHi... test/browser/elemHideEmulation.js:551: { On 2019/04/03 11:15:58, Manish Jethani wrote: > Nit: No need for the braces here. Done. https://codereview.adblockplus.org/30035555/diff/30036555/test/browser/elemHi... test/browser/elemHideEmulation.js:563: test.done(); On 2019/04/03 11:15:58, Manish Jethani wrote: > My reason for asking to move `test.done()` here was so we could combine > everything into one test function, something like > `exports.testPropertySelectorQualification`. I can see merits of keeping them > separate also, so it's your call, but then I think we should name them more > appropriately then suffixing with 2, 3, and so on. I'm OK if we keep one > function and simply use comments to explain each case. Having actually separate test cases is important because of the setUp and tearDown that runs for each test. I'll get back to having the test.done() in the other functions, to be consistent with the rest of tests.
https://codereview.adblockplus.org/30035555/diff/30036555/test/browser/elemHi... File test/browser/elemHideEmulation.js (right): https://codereview.adblockplus.org/30035555/diff/30036555/test/browser/elemHi... test/browser/elemHideEmulation.js:543: <a> On 2019/04/03 16:30:07, hub wrote: > On 2019/04/03 11:15:58, Manish Jethani wrote: > > Nit: It is more natural if `<a>` occurs with the `<p>` rather than the other > way > > around. > > if everything was done in a "natural way", lot of the stuff we do wouldn't be > needed. Fair enough :) https://codereview.adblockplus.org/30035555/diff/30036555/test/browser/elemHi... test/browser/elemHideEmulation.js:563: test.done(); On 2019/04/03 16:30:07, hub wrote: > On 2019/04/03 11:15:58, Manish Jethani wrote: > > My reason for asking to move `test.done()` here was so we could combine > > everything into one test function, something like > > `exports.testPropertySelectorQualification`. I can see merits of keeping them > > separate also, so it's your call, but then I think we should name them more > > appropriately then suffixing with 2, 3, and so on. I'm OK if we keep one > > function and simply use comments to explain each case. > > Having actually separate test cases is important because of the setUp and > tearDown that runs for each test. I'll get back to having the test.done() in the > other functions, to be consistent with the rest of tests. Well in that case I think it's OK to just move `test.done()` back to the function that is called from here. I had a different idea of what we were doing, now I think you had it right the first time.
https://codereview.adblockplus.org/30035555/diff/30036555/test/browser/elemHi... File test/browser/elemHideEmulation.js (right): https://codereview.adblockplus.org/30035555/diff/30036555/test/browser/elemHi... test/browser/elemHideEmulation.js:563: test.done(); > Well in that case I think it's OK to just move `test.done()` back to the > function that is called from here. I had a different idea of what we were doing, > now I think you had it right the first time. already done :-)
https://codereview.adblockplus.org/30035555/diff/30036555/test/browser/elemHi... File test/browser/elemHideEmulation.js (right): https://codereview.adblockplus.org/30035555/diff/30036555/test/browser/elemHi... test/browser/elemHideEmulation.js:532: // See issue https://issues.adblockplus.org/ticket/7359 On 2019/04/03 16:30:07, hub wrote: > On 2019/04/03 11:15:58, Manish Jethani wrote: > > Shouldn't this be 7400? > > The test is actually all based on issue #7359. It was even written before the > other bugs were filed. > I don't think this even test what #7400 fixes. Sorry, isn't the second test here affected by 7400? I thought "span::before" and "> span" would not merge correctly before the change. PS: From what I could tell, 7359 was a vague report of "does not work for me" but did not give a reproducible test case (both filters did not work unlike what the report said). It turned out that 7428 was the issue. Anyway, the tests here are addressing both 7400 and 7428. https://codereview.adblockplus.org/30035555/diff/30037571/test/browser/elemHi... File test/browser/elemHideEmulation.js (right): https://codereview.adblockplus.org/30035555/diff/30037571/test/browser/elemHi... test/browser/elemHideEmulation.js:537: <style> Nit: We could just indent the HTML here for readability, the indentation doesn't matter in the HTML context anyway (but it makes the code more readable if we indent it as if it were JS).
https://codereview.adblockplus.org/30035555/diff/30036555/test/browser/elemHi... File test/browser/elemHideEmulation.js (right): https://codereview.adblockplus.org/30035555/diff/30036555/test/browser/elemHi... test/browser/elemHideEmulation.js:532: // See issue https://issues.adblockplus.org/ticket/7359 On 2019/04/03 17:09:30, Manish Jethani wrote: > PS: From what I could tell, 7359 was a vague report of "does not work for me" > but did not give a reproducible test case (both filters did not work unlike what > the report said). It turned out that 7428 was the issue. Anyway, the tests here > are addressing both 7400 and 7428. OK, I looked at 7359 again. The only thing it adds to 7400 and 7428 is the :-abp-has() wrapper, which is a red herring: the issue has nothing to do with :-abp-has() at all. We can still add tests for :-abp-properties() (with `span` and with `>span`) and :-abp-has(:-abp-properties()) (with `span` and with `>span`). In that case, we can reference all three of 7359, 7400, and 7428.
https://codereview.adblockplus.org/30035555/diff/30036555/test/browser/elemHi... File test/browser/elemHideEmulation.js (right): https://codereview.adblockplus.org/30035555/diff/30036555/test/browser/elemHi... test/browser/elemHideEmulation.js:532: // See issue https://issues.adblockplus.org/ticket/7359 On 2019/04/03 17:26:53, Manish Jethani wrote: > OK, I looked at 7359 again. The only thing it adds to 7400 and 7428 is the > :-abp-has() wrapper, which is a red herring: the issue has nothing to do with > :-abp-has() at all. Misspoke, of course the `>span` case is valid only if there's a :-abp-has().
Patch does not apply cleanly, does this need a rebase?
udpated patch https://codereview.adblockplus.org/30035555/diff/30037571/test/browser/elemHi... File test/browser/elemHideEmulation.js (right): https://codereview.adblockplus.org/30035555/diff/30037571/test/browser/elemHi... test/browser/elemHideEmulation.js:537: <style> On 2019/04/03 17:09:30, Manish Jethani wrote: > Nit: We could just indent the HTML here for readability, the indentation doesn't > matter in the HTML context anyway (but it makes the code more readable if we > indent it as if it were JS). Done.
On 2019/04/03 17:30:41, Manish Jethani wrote: > Patch does not apply cleanly, does this need a rebase? not sure I have too many patches in my queue.
Rebased now
https://codereview.adblockplus.org/30035555/diff/30037602/test/browser/elemHi... File test/browser/elemHideEmulation.js (right): https://codereview.adblockplus.org/30035555/diff/30037602/test/browser/elemHi... test/browser/elemHideEmulation.js:568: "div:-abp-has(> a p:-abp-has(> span:-abp-properties(content: \"any\")))" Thanks for rebasing, it is looking good now, but some more comments ... This test here is not about a combinator qualifier, because the selector is `> span`, which is not the case we fixed for combinators. Instead we can make it exactly like the previous test: `div:-abp-has(> a p:-abp-has(> :-abp-properties(content: \"any\")))`, i.e. remove "span". Then ... we can add two more tests (if you like): div:-abp-has(span:-abp-properties(content: \"any\")) div:-abp-has(p:-abp-has(span:-abp-properties(content: \"any\"))) Here we would be testing specifically for 7400, which is about the case where the original selector and the qualifier both end in the same type. We could call these tests `testPropertySelectorIdenticalTypeQualifier` and `testPropertySelectorIdenticalTypeQualifierNested` respectively. If we include these last two suggestions (it is up to you), then we should reference 7400 in the commit message.
updated patch https://codereview.adblockplus.org/30035555/diff/30037602/test/browser/elemHi... File test/browser/elemHideEmulation.js (right): https://codereview.adblockplus.org/30035555/diff/30037602/test/browser/elemHi... test/browser/elemHideEmulation.js:568: "div:-abp-has(> a p:-abp-has(> span:-abp-properties(content: \"any\")))" On 2019/04/03 18:37:40, Manish Jethani wrote: > Thanks for rebasing, it is looking good now, but some more comments ... > > This test here is not about a combinator qualifier, because the selector is > `> span`, which is not the case we fixed for combinators. Instead we can make it > exactly like the previous test: `div:-abp-has(> a p:-abp-has(> > :-abp-properties(content: \"any\")))`, i.e. remove "span". > > Then ... we can add two more tests (if you like): > > div:-abp-has(span:-abp-properties(content: \"any\")) > div:-abp-has(p:-abp-has(span:-abp-properties(content: \"any\"))) > > Here we would be testing specifically for 7400, which is about the case where > the original selector and the qualifier both end in the same type. We could call > these tests `testPropertySelectorIdenticalTypeQualifier` and > `testPropertySelectorIdenticalTypeQualifierNested` respectively. > > If we include these last two suggestions (it is up to you), then we should > reference 7400 in the commit message. Done.
https://codereview.adblockplus.org/30035555/diff/30037606/test/browser/elemHi... File test/browser/elemHideEmulation.js (right): https://codereview.adblockplus.org/30035555/diff/30037606/test/browser/elemHi... test/browser/elemHideEmulation.js:532: // See issue https://issues.adblockplus.org/ticket/7359 Shouldn't these rather be moved to the individual tests below? https://codereview.adblockplus.org/30035555/diff/30037606/test/browser/elemHi... test/browser/elemHideEmulation.js:558: runTestQualifier( We could reference 7428 here. https://codereview.adblockplus.org/30035555/diff/30037606/test/browser/elemHi... test/browser/elemHideEmulation.js:566: runTestQualifier( We could reference 7359 here.
updated patch https://codereview.adblockplus.org/30035555/diff/30037606/test/browser/elemHi... File test/browser/elemHideEmulation.js (right): https://codereview.adblockplus.org/30035555/diff/30037606/test/browser/elemHi... test/browser/elemHideEmulation.js:558: runTestQualifier( On 2019/04/03 18:56:19, Manish Jethani wrote: > We could reference 7428 here. Done. https://codereview.adblockplus.org/30035555/diff/30037606/test/browser/elemHi... test/browser/elemHideEmulation.js:566: runTestQualifier( On 2019/04/03 18:56:19, Manish Jethani wrote: > We could reference 7359 here. Done.
LGTM, thanks! 
|
