Issue 3143 - Filter elements with :-abp-has()

test/browser/elemHideEmulation.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2017 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 14 matching lines @@
 let myUrl = document.currentScript.src;
 
 exports.tearDown = function(callback)
 {
   let styleElements = document.head.getElementsByTagName("style");
   while (styleElements.length)
     styleElements[0].parentNode.removeChild(styleElements[0]);
 
   let child;
   while (child = document.body.firstChild)
-    document.body.removeChild(child);

[Wladimir Palant, 2017/06/08 13:12:42]

If you store the child in an intermediate variable, you should use it:

  child.parentNode.removeChild(child);

[hub, 2017/06/08 15:45:48]

Done.

+    child.parentNode.removeChild(child);
 
   callback();
 };
 
 function unexpectedError(error)
 {
   console.error(error);
   this.ok(false, "Unexpected error: " + error);
 }
 
@@ skipping 24 matching lines @@
   if (styleElements.length)
     styleElement = styleElements[0];
   else
   {
     styleElement = document.createElement("style");
     document.head.appendChild(styleElement);
   }
   styleElement.sheet.insertRule(rule, styleElement.sheet.cssRules.length);
 }
 
-// insert a <div> with a unique id and and empty CSS rule

[Wladimir Palant, 2017/06/08 13:12:43]

You have "and" twice here. Also, how is this an empty CSS rule? It actually has
a value, according to the styleBlock parameter.

[hub, 2017/06/08 15:45:50]

Done.

+// insert a <div> with a unique id and a CSS rule
 // for the the selector matching the id.
 function createElementWithStyle(styleBlock, parent)
 {
   let element = document.createElement("div");
   element.id = findUniqueId();
   if (!parent)
     document.body.appendChild(element);
   else
     parent.appendChild(element);
   insertStyleRule("#" + element.id + " " + styleBlock);
   return element;
 }
 
-// Will ensure the class ElemHideEmulation is loaded
-// and then will call the callback.

[Wladimir Palant, 2017/06/08 13:12:41]

There are no callbacks here, this function returns a promise.

[hub, 2017/06/08 15:45:50]

I forgot to update the comment when we switched this to promise. :-/

Done

-// NOTE: if it never loads, this will probably hang in an infinite
-// loop

[Wladimir Palant, 2017/06/08 13:12:42]

There is no loop here. But - yes, loadScript() might not detect the error
condition and reject the promise.

[hub, 2017/06/08 15:45:52]

Done.

+// Will ensure the class ElemHideEmulation is loaded.
+// NOTE: if it never loads, this will probably hang.
 function loadElemHideEmulation()
 {
   if (typeof ElemHideEmulation == "undefined")
   {
     return loadScript(myUrl + "/../../../lib/common.js").then(() =>
     {
       return loadScript(myUrl + "/../../../chrome/content/elemHideEmulation.js");
     }).then(() =>
     {
       return loadElemHideEmulation();
     });
   }
 
   return Promise.resolve();
 }
 
-// instantiate a ElemHideEmulation with @selectors.

[Wladimir Palant, 2017/06/08 13:12:43]

"an ElemHideEmulation instance"?

[hub, 2017/06/08 15:45:48]

Done.

+// Create a new ElemHideEmulation instance with @selectors.
 function applyElemHideEmulation(selectors)
 {
   return loadElemHideEmulation().then(() =>
   {
     let elemHideEmulation = new ElemHideEmulation(
       window,
       callback =>
       {
         let patterns = [];
         selectors.forEach(selector =>
         {
           patterns.push({selector});
         });
         callback(patterns);
       },
       newSelectors =>
       {
         if (!newSelectors.length)
           return;
         let selector = newSelectors.join(", ");
         insertStyleRule(selector + "{display: none !important;}");
       },
       elems =>
       {
-        if (!elems.length)
-          return;

[Wladimir Palant, 2017/06/08 13:12:42]

This check is pointless, we can just let the loop run even with an empty array.

[hub, 2017/06/08 15:45:49]

Done.

         for (let elem of elems)
           elem.style.display = "none";
       }
     );
 
     elemHideEmulation.apply();
-    return Promise.resolve();

[Wladimir Palant, 2017/06/08 13:12:43]

While it was like that before your changes already, it would probably make sense
to return Promise.resolve(elemHideEmulation) here. This way you can test
ElemHideEmulation properties where other options don't work.

[hub, 2017/06/08 15:45:50]

Done.

+    return Promise.resolve(elemHideEmulation);
   });
 }
+
+// internals testing
 
 exports.testParseSelectorContent = function(test)
 {
   loadElemHideEmulation().then(() =>
   {
     let parsed = parseSelectorContent(":-abp-has(> div) > div", 10);
     test.equal(parsed.text, "> div");
     test.equal(parsed.end, 15);
 
-    parsed = parseSelectorContent("\"> div\") > div", 0);
-    test.equal(parsed.text, "\"> div\"");
-    test.equal(parsed.end, 7);
-    parsed = parseSelectorContent(" \"> div\" ) > div", 0);
-    test.equal(parsed.text, " \"> div\" ");
-    test.equal(parsed.end, 9);
+    parsed = parseSelectorContent("> div) > div", 0);
+    test.equal(parsed.text, "> div");
+    test.equal(parsed.end, 5);
 
     // parens not closed.
     parsed = parseSelectorContent("> div > div", 0);
     test.equal(parsed, null);
   }).catch(unexpectedError.bind(test)).then(() => test.done());

[Wladimir Palant, 2017/06/08 13:12:40]

I'm not really a friend of testing implementation details - we should be testing
the public API, otherwise we will have to adjust the tests with each change of
the internals. This should work for most of the tests here, these can be
replaced by verifying that the selector `[foo] >
:abp-has(div[foo="yxcv)'"]:not([bar='ddd'])) > [bar]` works correctly.

Testing invalid selectors via the public API isn't possible unfortunately, error
conditions won't be reported. Still, these should be tested via high-level APIs
IMHO - testing ElemHideEmulation.patterns should do.

[hub, 2017/06/08 15:45:49]

I actually think that *also* testing internal functions has a huge value. It
allows easier detection of problems when changing things as it is more granular.
It also serve as self documenting example of how it works. Yes they need to be
fixed if we change the internals behaviour, but that's also true if we change
public API.

As you said above, there are even some test that are impossible with the public
API, yet we want to ensure they work.

I can remove line 159-164 as they were used to test quoting which is no longer
done in this patch revision.

I'll mark internals vs API so that we can distinguish.

[Wladimir Palant, 2017/06/13 13:16:25]

As you wish but I certainly won't object to anybody removing these tests again
when rewriting code. The point of unit tests is catching behavior changes that
are visible outside, not flagging rewrites and requiring tests to be adapted
despite unchanged behavior.

In fact, that "anybody" might be me - once that code is moved to Emscripten,
testing internals will no longer be feasible.

 };
 
 exports.testParseSelector = function(test)
 {
   loadElemHideEmulation().then(() =>
   {
     let selectors = parseSelector("");
     test.equal(selectors.length, 0);
 
     let selector = "div > :-abp-properties('background-color: rgb(0, 0, 0)')";
@@ skipping 137 matching lines @@
     let selectors = splitSelector("div:-abp-has(div) > [-abp-properties='background-color: rgb(0, 0, 0)'] > span");
     test.ok(selectors);
     test.equal(selectors.length, 1, "There is only one selector");
 
     selectors = splitSelector("div:-abp-has(div), [-abp-properties='background-color: rgb(0, 0, 0)']");
     test.ok(selectors);
     test.equal(selectors.length, 2, "There are two selectors");
   }).catch(unexpectedError.bind(test)).then(() => test.done());
 };
 
+// API testing
+
 exports.testVerbatimPropertySelector = function(test)
 {
   let toHide = createElementWithStyle("{background-color: #000}");
   applyElemHideEmulation(
     [":-abp-properties(background-color: rgb(0, 0, 0))"]
   ).then(() =>
   {
     expectHidden(test, toHide);
   }).catch(unexpectedError.bind(test)).then(() => test.done());
 };
@@ skipping 243 matching lines @@
   let parent = createElementWithStyle("{}");
   let child = createElementWithStyle("{background-color: #000}", parent);
   applyElemHideEmulation(
     ["div:-abp-has(:-abp-properties(background-color: rgb(0, 0, 0)))"]
   ).then(() =>
   {
     expectVisible(test, child);
     expectHidden(test, parent);
   }).catch(unexpectedError.bind(test)).then(() => test.done());
 };