Issue 7428, 7400 - Add test case for nested :-abp-properties()

test/browser/elemHideEmulation.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 511 matching lines @@
     middile: true,
     inside: true,
     sibling: true,
     sibling2: true,
     toHide: true
   };
   runTestPseudoClassHasSelectorWithHasAndWithSuffixSibling(
     test, "div:-abp-has(> span:-abp-contains(Advertisment))", expectations);
 };
 
+// See issue https://issues.adblockplus.org/ticket/7359

[Manish Jethani, 2019/04/03 11:15:58]

Shouldn't this be 7400?

[hub, 2019/04/03 16:30:07]

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.

[Manish Jethani, 2019/04/03 17:09:30]

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.

[Manish Jethani, 2019/04/03 17:26:53]

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.

[Manish Jethani, 2019/04/03 17:28:49]

Misspoke, of course the `>span` case is valid only if there's a :-abp-has().

+// and issue https://issues.adblockplus.org/ticket/7428
+async function runTestCombinatorQualifier(test, selector)

[Manish Jethani, 2019/04/03 11:15:58]

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.

[hub, 2019/04/03 16:30:07]

Done.

+{
+  testDocument.body.innerHTML = `
+<style>
+span::before {
+  content: "any";
+}
+</style>
+<div id="toHide">
+  <a>

[Manish Jethani, 2019/04/03 11:15:58]

Nit: It is more natural if `<a>` occurs with the `<p>` rather than the other way
around.

[hub, 2019/04/03 16:30:07]

if everything was done in a "natural way", lot of the stuff we do wouldn't be
needed. 

[Manish Jethani, 2019/04/03 16:40:02]

Fair enough :)

+    <p>
+      <span></span>
+    </p>
+  </a>
+</div>
+`;
+  if (await applyElemHideEmulation(test, [selector]))
+  {

[Manish Jethani, 2019/04/03 11:15:58]

Nit: No need for the braces here.

[hub, 2019/04/03 16:30:07]

Done.

+    expectHidden(test, testDocument.getElementById("toHide"));
+  }
+}
+
+exports.testPropertySelectorCombinatorQualifier = async function(test)
+{
+  await runTestCombinatorQualifier(
+    test,
+    "div:-abp-has(> a p > :-abp-properties(content: \"any\"))"
+  );
+
+  test.done();

[Manish Jethani, 2019/04/03 11:15:58]

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.

[hub, 2019/04/03 16:30:07]

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.

[Manish Jethani, 2019/04/03 16:40:02]

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.

[hub, 2019/04/03 16:41:57]

already done :-)

+};
+
+exports.testPropertySelectorCombinatorQualifier2 = async function(test)
+{
+  await runTestCombinatorQualifier(
+    test,
+    "div:-abp-has(> a p:-abp-has(> span:-abp-properties(content: \"any\")))"
+  );
+
+  test.done();
+};
+
 async function runTestPseudoClassContains(test, selector, expectations)
 {
   testDocument.body.innerHTML = `  <style type="text/css">
     body {
       margin: 0;
       padding: 0;
     }
     .transparent {
       opacity: 0;
       position: absolute;
@@ skipping 604 matching lines @@
     {
       if (element.id == "n2" || element.id == "n2_3")
         expectProcessed(test, element, element.id);
       else
         expectNotProcessed(test, element, element.id);
     }
   }
 
   test.done();
 };