Issue 6822 - Report browser test errors

test_runner.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 136 matching lines @@
   }).then(bundle =>
     Promise.all(
       processes.map(currentProcess =>
         runnerDefinitions[currentProcess](
           bundle, bundleFilename,
           browserFiles.map(
             file => path.relative(path.join(__dirname, "test", "browser"),
                                   file).replace(/\.js$/, "")
           )
         )
-      )
-    )
+        // We need to convert rejected promise to a resolved one
+        // or the test will not let close the webdriver.

[Sebastian Noack, 2018/09/21 14:22:21]

Does that still hold true with the changes to test/runners/webdriver.js where we
now make sure the driver is properly closed on failure?

[hub, 2018/09/21 15:39:35]

Yes. The problem is that as soon as one of the promise is rejected in
`Promise.all` we continue past, and the the script terminates which doesn't
leave the time for the browser instances to be quit.

+        .catch(e => e)
+    )).then(results =>
+    {
+      let errors = results.filter(e => typeof e != "undefined");
+      if (errors.length)
+        return Promise.reject(`Browser unit test failed: ${errors.join(", ")}`);

[Sebastian Noack, 2018/09/21 14:22:21]

Nit: You can just use throw here.

[hub, 2018/09/21 15:39:36]

Done.

+    })
   );
 }
 
 if (process.argv.length > 2)
   addTestPaths(process.argv.slice(2), true);
 else
 {
   addTestPaths(
     [path.join(__dirname, "test"), path.join(__dirname, "test", "browser")],
     true
   );
 }
 
-Promise.resolve(runBrowserTests(runnerProcesses)).catch(error =>
-{
-  console.error("Failed running browser tests");
-  console.error(error);
-}).then(() =>
+runBrowserTests(runnerProcesses).then(() =>
 {
   if (unitFiles.length)
     nodeunit.reporters.default.run(unitFiles);
+}).catch(error =>
+{
+  console.error(error);
+  process.exit(1);

[Sebastian Noack, 2018/09/21 14:22:21]

What would happen if we just leave this error unhandled?

[hub, 2018/09/21 15:39:36]

Currently you get a warning about unhandled promise rejection. On more recent
Nodejs you are supposed to get a non-zero exit code with termination, at least
that's what the deprecation warning says.

 });