Issue 6822 - Report browser test errors

test/runners/webdriver.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 21 matching lines @@
     .then(() => driver.executeAsyncScript(realScript, scriptArgs))
     .then(() => driver.executeScript("return window._consoleLogs;"))
     .then(result =>
     {
       console.log(`\nBrowser tests in ${name}\n`);
       for (let item of result.log)
         console.log(item);
       return driver.quit().then(() =>
       {
         if (result.failures != 0)
-          return Promise.reject(name);

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

Nit: You can just use throw here.

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

Done.

+          throw name;
       });
     }, error =>
     {
-      return driver.quit().then(() => Promise.reject(error));

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

Please leave a comment that this code should be replaced with
.finally(() => driver.quit()) once we require Node.js >= 10.

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

Now, we call driver.quit() twice if there are any failures. Does that work, or
would that cause a new promise rejection which silences the actual error?

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

No we don't. We are on the rejection handler for that `then`. So we don't reach
here if `result.failures != 0`
Here I pass the actual error if `driver.quit()` is successful. Otherwise we get
the `driver.quit()` error.
Note that to confirm the answer above I'd get an error if I call `driver.quit()`
twice.

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

Done.

[Sebastian Noack, 2018/09/21 15:58:46]

You are right. I misread the code, I thought we are in another catch callback
here. FWIW, if I have more than one callback, I prefer to wrap the code like
this, which makes that more obvious:

  .then(
    result =>
    {
    },
    error =>
    {
    }
  )

+      // This could be replaced by Promise.finally()
+      // once we require NodeJS >= 10
+      return driver.quit().then(() =>
+      {
+        throw error;
+      });
     });
 }
 
 module.exports.executeScript = executeScript;