Issue 4796 - Use a modern JS engine in the browser tests and convert all files to ECMAScript 6

test/browser/_bootstrap.js

Index: test/browser/_bootstrap.js
===================================================================
rename from browsertests.js
rename to test/browser/_bootstrap.js
--- a/browsertests.js
+++ b/test/browser/_bootstrap.js
@@ -12,169 +12,140 @@
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 "use strict";
 
-(function()
+/* globals nodeunit */
+
+(function(nodeunitUrl, ...moduleUrls)

[Felix Dahlke, 2017/05/03 09:54:51]

As discussed on IRC, I find this approach of loading the code (i.e. as an
expression whose result is saved) a bit unintuitive. Maybe we could set
`module.exports` instead, CommonJS style?

[Wladimir Palant, 2017/05/03 12:19:42]

This isn't a module, we are loading code in global scope. So I'd rather not have
it touch any global symbols.

 {
-  // We are currently limited to ECMAScript 5 in this file, because it is being
-  // used in the browser tests. See https://issues.adblockplus.org/ticket/4796
-  // Once this is resolved we should use promises here.
-  function safeCall(callback)
+  function loadScript(doc, url)
   {
-    return function()
+    return new Promise((resolve, reject) =>
     {
-      try
-      {
-        callback.apply(this, arguments);
-      }
-      catch (e)
-      {
-        var message = String(e);
-        if (e.stack)
-          message += "\n" + e.stack;
-        console.log(message);
-        phantom.exit(1);
-      }
-    };
+      let script = doc.createElement("script");
+      script.src = url;
+      script.onload = resolve;
+      doc.head.appendChild(script);
+    });
   }
 
-  function loadScript(doc, url, callback)
+  function loadModules(urls)
   {
-    var script = doc.createElement("script");
-    script.src = url;
-    script.async = false;
-    doc.head.appendChild(script);
-    if (callback)
-      window.setTimeout(callback, 0);
-  }
+    let modules = {};
 
-  function loadModules(urls, callback)
-  {
-    var modules = {};
-
-    var loadNext = safeCall(function()
+    let loadNext = () =>

[Felix Dahlke, 2017/05/03 09:54:51]

I might be missing the reason for using let here, but if it's not actually
necessary, I would suggest to simplify the code like this:

  (function loadNext()
  {
    ...
  })();

[Wladimir Palant, 2017/05/03 12:19:42]

Not sure whether it makes the code better but ok.

     {
       if (urls.length)
       {
         // Load each module into a new frame so that their scopes don't clash
-        var frame = document.createElement("iframe");
+        let frame = document.createElement("iframe");
         document.body.appendChild(frame);
 
-        var wnd = frame.contentWindow;
-        wnd.loadScript = loadScript.bind(null, wnd.document);
-        wnd.console = console;
-        wnd.require = require;
+        let wnd = frame.contentWindow;
+        wnd.loadScript = url => loadScript(wnd.document, url);
         wnd.exports = {};
         wnd.module = {exports: wnd.exports};
 
-        var url = urls.shift();
-        var name = url.split("/").pop();
-        wnd.loadScript(url, safeCall(function()
+        let url = urls.shift();
+        let name = url.split("/").pop();
+        return wnd.loadScript(url).then(() =>
         {
           modules[name] = nodeunit.testCase(wnd.module.exports);
-          loadNext();
-        }));
+          return loadNext();
+        });
       }
-      else
-        callback(modules);
-    });
 
-    loadNext();
+      return Promise.resolve(modules);
+    };
+
+    return loadNext();
   }
 
-  function runTests(modules, callback)
+  function runTests(modules)
   {
     function bold(str)
     {
       return "\u001B[1m" + str + "\u001B[22m";
     }
 
     function ok(str)
     {
       return "\u001B[32m" + str + "\u001B[39m";
     }
 
     function error(str)
     {
       return "\u001B[31m" + str + "\u001B[39m";
     }
 
-    nodeunit.runModules(modules, {
-      moduleStart: function(name)
-      {
-        console.log(bold(name));
-      },
-      testDone: function(name, assertions)
-      {
-        var errors = assertions.filter(function(assertion)
+    return new Promise((resolve, reject) =>
+    {
+      nodeunit.runModules(modules, {
+        moduleStart(name)
         {
-          return assertion.failed();
-        }).map(function(assertion)
+          console.log(bold(name));
+        },
+        testDone(name, assertions)
         {
-          return assertion.error;
-        });
-
-        if (errors.length == 0)
-          console.log("\u2714 " + name);
-        else
-        {
-          console.log(error("\u2716 " + name) + "\n");
-          errors.forEach(function(error)
+          let errors = assertions.filter(assertion =>
           {
-            console.log(String(error));
-            if (error.stack)
-              console.log(error.stack);
-            console.log("");
+            return assertion.failed();
+          }).map(assertion =>
+          {
+            return assertion.error;
           });

[Felix Dahlke, 2017/05/03 09:54:51]

Nit: How I see it, this could be shortened quite a bit without violating the 80
column rule:

  let errors = assertions.filter(assertion => assertion.failed())
    .map(assertion => assertion.error);

[Wladimir Palant, 2017/05/03 12:19:42]

Done.

-        }
-      },
-      done: function(assertions)
-      {
-        var failures = assertions.filter(function(assertion)
-        {
-          return assertion.failed();
-        });
-        if (failures.length)
+
+          if (errors.length == 0)
+            console.log("\u2714 " + name);
+          else
+          {
+            console.log(error("\u2716 " + name) + "\n");
+            errors.forEach(e =>
+            {
+              if (e.stack)
+                console.log(e.stack);
+              else
+                console.log(String(e));
+              console.log("");
+            });
+          }
+        },
+        done(assertions)
         {
-          console.log(
-            "\n" +
-            bold(error("FAILURES: ")) +
-            failures.length + "/" + assertions.length + " assertions failed"
-          );
+          let failures = assertions.filter(assertion =>

[Felix Dahlke, 2017/05/03 09:54:51]

Nit: As above, I think this can be shortened:

  let failures = assertions.filter(assertion => assertion.failed());

[Wladimir Palant, 2017/05/03 12:19:42]

Done.

+          {
+            return assertion.failed();
+          });
+          if (failures.length)
+          {
+            console.log(
+              "\n" +
+              bold(error("FAILURES: ")) +
+              failures.length + "/" + assertions.length + " assertions failed"
+            );
+          }
+          else
+          {
+            console.log(
+              "\n" + bold(ok("OK: ")) +
+              assertions.length + " assertions"
+            );
+          }
+
+          resolve();
         }
-        else
-        {
-          console.log(
-            "\n" + bold(ok("OK: ")) +
-            assertions.length + " assertions"
-          );
-        }
-
-        callback(!failures.length);
-      }
+      });
     });
   }
 
-  function run()
+  return loadScript(document, nodeunitUrl).then(() =>
   {
-    var system = require("system");
-    var nodeunitUrl = system.args[1];
-    var urls = system.args.slice(2);
-
-    loadScript(document, nodeunitUrl, safeCall(function()
-    {
-      loadModules(urls, safeCall(function(modules)
-      {
-        runTests(modules, function(success)
-        {
-          phantom.exit(success ? 0 : 1);
-        });
-      }));
-    }));
-  }
-
-  safeCall(run)();
-})();
+    return loadModules(moduleUrls);
+  }).then(modules =>
+  {
+    return runTests(modules);
+  });
+});