test: Add cross browsers smoke tests and add support for sendMessage rejected promise behavior #115

rpl · 2018-05-14T19:10:37Z

This pull request applies some changes to the "smoke tests", which currently runs only on Chrome using the puppeteer npm package, to make them run on more then one browser (currently Chrome and Firefox).

The new approach is collecting test results from the test extensions, in the "tap protocol" format, and merge them into a single "per browser" test suite, then run the entire test suite on different browsers.

The supported browsers (currently Chrome and Firefox) are launched and controller using selenium-webdriver (which at least theoretically should be allow us to support also Edge, but I've been currently unable to make it works on Edge as documented in the Microsoft doc page).

On top of the changes needed on the test suite, there are the following changes:

Print a deprecation warning (only once) when the deprecated sendResponse callback has been called
(we added it because the current behavior, defined and working on Firefox and undefined on Chrome, was confusing, but it is still true that it should be deprecated and removed once it has been removed from the W3C draft and from Firefox)
Reject the promised returned by sendMessage, when a onMessage listener returns a rejected promise (as close as possible to the behavior implemented natively on Firefox)
Test explicitly that the polyfill "existing browser API object" detection is not wrapping the chrome API namespace on Firefox by mistake (which shows practically why the simple change proposed in Different way of checking if browser is defined? #68 / Improve browser detection fix #68 #86 / Add support for browsers which don’t support Promises #114 , in a Firefox content script this !== window and the proposed check is going to make the polyfill to wrap the chrome API object and overwrite the native browser API object with the polyfill one)

james-cnz · 2018-05-15T05:21:39Z

src/browser-polyfill.js

+                fileName: error.fileName,
+                lineNumber: error.lineNumber,
+              },
+            });


I suggest replacing the above 11 lines with:
// Send a JSON representation of the error.
sendResponse({
__webExtensionPolyfillReject__: true,
message: (typeof error == "object" && error && error.message) ? String(error.message) : "An unexpected error occurred",
});

I don't think there's any point trying to send fileName and lineNumber, because these are non-standard Firefox-only properties?, and Firefox doesn't need the polyfill. (Also, I don't think even Firefox actually sends these back from onMessage?)

I think Firefox relays an error message from any Error-like object--specifically any non-null object with a truthy message property?--and returns the message "An unexpected error occured" otherwise. (The code in the pull request does very nearly, but not quite exactly, this?)

Something that's not an Error might nevertheless not be serialisable?

Since the "error" has to be checked at this end anyway, may as well do all the work at this end?

I personally prefer __mozWebExtPolyfill_isSerializedErrorObject over __webExtensionPolyfillReject__, the reasons are explained here:

Perhaps something like __mozWebExtPolyfill_isSerializedErrorObject could work, because isInstanceOfError is likely to be used by something.

The double-_ prefix is used since it’s somewhat common practice to use __comment in place of real comments in JSON, and a single-_ prefix already denotes an internal varible.

The moz prefix is also used by non-standard Mozilla properties on Web APIs, and WebExtPolyfill identifies this polyfill.

@james-cnz you are right, I've currently opted to simplify it for now and just send the error message (most of the changes applied should be in line with your suggestion), we can try to rethink these "error stack traces" in a follow up, it is particularly tricky to achieve the same/similar result, and the Firefox behavior related to these stack traces is also changing over the last few version).

@ExE-Boss I've renamed it into __mozWebExtensionPolyfillReject__
which follows most of the conventions you suggested.

Thank you both for your help on this! it is very much appreciated! ❤️

james-cnz · 2018-05-15T05:26:19Z

src/browser-polyfill.js

+          // using a generic error object.
+          console.error(error);
+          reject(unexpectedError);
+        }


I suggest replacing the above 13 lines with:
// Convert the JSON representation of the error back to an Error instance.
unexpectedError.message = reply.message;
console.error(unexpectedError);
reject(unexpectedError);
I think the reject response is better this way? I don't know what the console error message is supposed to be like though.

If I understand this comment re console error messages correctly, maybe remove the console error line?

for now I've removed that part (and the console.error).

james-cnz · 2018-05-15T08:21:54Z

src/browser-polyfill.js

        }

        // Let Chrome know that the listener is replying.
        return true;
      };
    });

+    const wrappedSendMessageCallback = ({reject, resolve, unexpectedError}, reply) => {


Should chrome.runtime.lastError be checked in here?

That's a good point, I'm not sure that is going to be effective (and I'm not sure how we could trigger it in one of the test cases), but at least it should not do any harm (chrome should already guarantees that if there is an error set on chrome.runtime.lastError it should be related to the currently executed callback).

@Rob--W do you have any opinion (or specific knowledge) about this?

An error could be triggered if you send a message without recipient. E.g. chrome.tabs.sendMessage(SOME_NON_EXISTENT_TAB_ID, msg, callback);

Note that unlike many other extension APIs, Chrome does not print an unchecked error to the console (but chrome.runtime.lastError is set). (If you try to test this in Chrome and can't confirm this behavior, then you might be affected by this bug: https://bugs.chromium.org/p/chromium/issues/detail?id=836370 )

Rob--W · 2018-05-14T20:44:08Z

.travis.yml

-  TRAVIS_CI=true ./test/run-chrome-smoketests.sh
+  TRAVIS_CI=true ./test/run-browsers-smoketests.sh
+
+sudo: required


Why is this needed?

To be fair I've not yet tried to remove it, but travis doc page related to chrome was suggesting it: https://docs.travis-ci.com/user/chrome

Rob--W · 2018-05-15T09:56:42Z

test/integration/setup.js

+const {Builder, By, until} = require("selenium-webdriver");
+const {Command} = require("selenium-webdriver/lib/command");
+const chrome = require("selenium-webdriver/chrome");
+const firefox = require("selenium-webdriver/firefox");


Is there any reason to not move these requires into the if-blocks in launchBrowser?

👍 I moved the require for the browser-specific ones in the related launchBrowser if-blocks.

Rob--W · 2018-05-15T09:57:50Z

test/integration/setup.js

+    const options = new chrome.Options();
+    options.addArguments([
+      `--load-extension=${extensionPath}`,
+      "--no-sandbox",


Add comment why this is needed, so that the --no-sandbox flag can be removed later if possible.

The comment should say: // See issue #85

Rob--W · 2018-05-15T10:08:48Z

test/integration/setup.js

-    headless: false,
+    for (const extensionDirName of extensions) {
+      it(extensionDirName, async () => {
+        const server = await createHTTPServer(path.join(__dirname, "..", "fixtures"));


The server is never closed explicitly. You can move the server outside the it, and create the server in before and close it in after.

EDIT: This has already been fixed in a later commit.

Rob--W · 2018-05-15T10:11:04Z

test/integration/setup.js

+      .forBrowser("chrome")
+      .setChromeOptions(options)
+      .setChromeService(new chrome.ServiceBuilder(chromedriver.path))
+      .build();


Consider adding a check for HEADLESS, and print a warning that headless Chrome is not supported because it does not support extensions.

uhm, yeah, sounds good to me

Rob--W · 2018-05-15T11:01:09Z

src/browser-polyfill.js

+      // Create an error object here, so that the original caller is part of the
+      // stack trace (similarly to the stack of the error raised on Firefox in
+      // this scenario).
+      const unexpectedError = new Error("An unexpected error occurred");


I'm missing a test case that checks this behavior.

This particular line is gone in the updated version.

A test case for the "An unexpected error occurred" is in the current set of tests but like I briefly explain in the inline comment, Firefox seems to present a different behavior in version 59 (where the error has the "An unexpected error occurred" message) and 60 (where it seems that it is currently "undefined"), and so the test cases is not currently asserting the error message (it just checks that an instance of Error is rejected in that case).

Rob--W · 2018-05-15T11:07:29Z

src/browser-polyfill.js

+          // If the original error wasn't an error instance, let's try to keep
+          // the behavior as close as possible to the Firefox behavior, e.g.
+          // print the error object as received in the console and then reject
+          // using a generic error object.


This is not Firefox's behavior, and even if it was, then it would be too implementation-specific.

Test case (Firefox Nightly 61, buildID 20180504220115):

browser.runtime.onMessage.addListener(()=>Promise.reject({message:"what"}))

// If doing this, no error printed: browser.runtime.sendMessage('ee').catch(()=>{}); // If doing this, then error is printed (because it's an uncaught promise rejection): browser.runtime.sendMessage('ee');

👍 I was also pretty unsure about these console errors, to be fair I'm still a bit worried that the stack traces that the extension is going to be able to access directly from their code could be not that helpful (at least in some of the scenarios), but I've removed all the redundant console messages for now.

Rob--W · 2018-05-15T11:08:22Z

src/browser-polyfill.js

+        const {error} = reply;
+        // Convert the JSON representation of the error back to an Error instance,
+        // otherwise just reject with the error value as it is.
+        if (reply.__webExtensionPolyfillReject__.isErrorInstance) {


Let's only forward the message. One cannot rely on fileName / lineNumber (and it is not even documented).

I've made a suggestion for this, see: onMessage side and sendMessage side.

👍 done (as briefly described in one of the above comments)

Rob--W · 2018-05-15T11:09:23Z

test/fixtures/runtime-messaging-extension/background.js

+      return Promise.reject(new Error("rejected-error-value"));
+
+    case "test - sendMessage with returned rejected Promise with non-Error value":
+      return Promise.reject("rejected-non-error-value");


Try Promise.reject({message: "rejected-non-error-value"}) instead.

If you wish, keep the existing test case, but check that no useful error message is passed through.

If keeping the existing test case, maybe check specifically for the message "An unexpected error occurred"? (EDIT: I guess it's kind of an implementation detail, but I think it might be easier to do it this way.)

For what it's worth, I think this mistake was already present in the code, not introduced by the pull request.

I added both the cases, to be fair I think that the behavior that Firefox have when an extension code rejects a plain object with a message property is actually an involuntary side-effect of the behavior we wanted when an API implementation rejects that kind of plain objects:

https://searchfox.org/mozilla-central/search?q=Promise.reject(%7Bmessage%3A&case=false&regexp=false&path=

Anyway, the polyfill is currently matching this behavior and I added an explicit test case for it.

@james-cnz Like I described in one of the above comments I'm not asserting the "An unexpected error occurred" error message because Firefox 59 and Firefox 60 have different behaviors (and so the test was failing on travis only on Firefox).

Rob--W · 2018-05-15T11:16:02Z

test/fixtures/tape-standalone.js

+}
+
+// Export as a global a wrapped test function which enforces a timeout by default.
+window.test = (desc, fn) => {


To make it clear that test is not a regular tap test, use a different name, e.g. describeBrowserTest.

uhm... not sure that it is worth the additional verbosity in the test files, in the end it is just a wrapper around tape which ensure that the test will timeout (because it wasn't and I would really prefer that it does timeout at some point if no message is being received, so that we can more easily guess where the test was failing directly from the failures on travis).

If it was a real test from tap, then test.skip would also work. But we don't use it yet, so if you prefer, we can keep test.

rpl · 2018-05-18T16:54:58Z

@ExE-Boss @james-cnz @Rob--W Thank you so much for all your review comments, you have been really really helpful!

This should not be ready for another round of review.

Rob--W

Just a few typos, good to go.

Have you also tried to run Chrome with --enable-features=NativeCrxBindings ? Native bindings is the future in Chrome, and if at some point native bindings are used by default, then the tests should still complete successfully.
(don't block landing this PR on my question, it is just a question)

Rob--W · 2018-06-01T19:51:39Z

test/fixtures/multiple-onmessage-listeners-extension/content.js

+  if (navigator.userAgent.includes("Firefox/")) {
+    t.deepEqual(res, undefined, "Got an undefined value as expected");
+  } else {
+    // NOTE: When an onMessage listener send `undefined` in a response,


s/send/sends/

Rob--W · 2018-06-01T19:54:53Z

test/fixtures/runtime-messaging-extension/content.js

-    // Unfortunately Firefox currently reject an error with an undefined
-    // message, in the meantime we just check that the object rejected is
-    // an instance of Error.
+    // Unfortunately Firefox currently reject an error with an "undefined"


s/reject/rejects/

Rob--W · 2018-06-01T19:55:50Z

test/fixtures/runtime-messaging-extension/content.js

+    t.equal(reply, undefined, "Unexpected successfully reply");
+  } catch (err) {
+    // Firefox currently converts any rejection with a message property into an error instance
+    // with that property valua as the error message.


Rob--W · 2018-06-01T20:04:25Z

test/integration/setup.js

@@ -101,8 +101,8 @@ async function runExtensionTest(t, server, driver, extensionDirName) {

    // Merge tap results from the connected browser.
    const el = await driver.wait(until.elementLocated(By.id("test-results")), 10000);
-    const tests = (await el.getAttribute("textContent")).split("\n");
-    console.log(tests.filter(row => !row.startsWith("TAP version")).join("\n"));
+    const testResults = (await el.getAttribute("textContent"));


Unnecessary braces around await ....

Rob--W · 2018-06-01T20:08:57Z

test/fixtures/tape-standalone.js

+}
+
+// Export as a global a wrapped test function which enforces a timeout by default.
+window.test = (desc, fn) => {


If it was a real test from tap, then test.skip would also work. But we don't use it yet, so if you prefer, we can keep test.

…or the first time

…eturns a rejected promise.

…d Firefox This commit introduces tape as the test framework used to define the tests in the test extension contexts and send them to the nodejs script that orchestrate the test run. The nodejs script has also been migrated from mocha to tape, it uses the custom test helpers provided to setup the test environment (e.g. create a temporary dir for the test extension, copy the last polyfill build, bundle tape to be used in the test extension, start the browser which run the test extension and finally collect the results of the test extension) and then it merges all the tap logs collected from every test extension into a single "per browser" test suite. - updated travis nodejs environment to nodejs 8 - uses tape to collect test results from inside the test extension - added test case to check polyfill 'existing browser API object' detection - added test for expected rejection on tabs.sendMessage with an invalid tabId - added test with multiple listeners which resolves to undefined and null - optionally run chrome smoketests with --enable-features=NativeCrxBindings

ExE-Boss · 2018-06-04T20:14:16Z

Something in this broke the #114 build, but in a way which isn’t caused by #114.

rpl requested a review from Rob--W May 14, 2018 19:10

james-cnz mentioned this pull request May 15, 2018

An attempt at handling errors in message passing (perhaps not the best approach) #112

Closed

james-cnz reviewed May 15, 2018

View reviewed changes

Rob--W reviewed May 15, 2018

View reviewed changes

rpl mentioned this pull request May 31, 2018

Test with node 6, latest LTS, and latest stable #69

Closed

rpl added the needs: docs label Jun 1, 2018

Rob--W approved these changes Jun 1, 2018

View reviewed changes

rpl added 3 commits June 2, 2018 20:49

fix: print a deprecation warning when sendResponse callback is used f…

451af2d

…or the first time

feat: Reject sendMessage returned promise when a onMessage listener r…

4c4b635

…eturns a rejected promise.

rpl force-pushed the test/cross-browsers-smoke-tests branch from 354ebc9 to b16ee4f Compare June 2, 2018 18:56

rpl merged commit 5d186ba into mozilla:master Jun 2, 2018

rpl mentioned this pull request Jun 2, 2018

Support multiple runtime.onMessage listeners that do not return results #23

Closed

ExE-Boss mentioned this pull request Jun 4, 2018

Release 0.3.0 #88

Closed

ExE-Boss mentioned this pull request Jun 16, 2018

Numerous deprecation warnings on install mozilla/web-ext#1334

Closed

rpl mentioned this pull request Jun 16, 2018

Add support for browsers which don’t support Promises #114

Closed

This was referenced Jun 17, 2018

Improve browser detection fix #68 #86

Closed

Different way of checking if browser is defined? #68

Closed

Correct .travis.yml #129

Merged

rpl deleted the test/cross-browsers-smoke-tests branch March 15, 2021 10:25

test: Add cross browsers smoke tests and add support for sendMessage rejected promise behavior #115

test: Add cross browsers smoke tests and add support for sendMessage rejected promise behavior #115

Conversation

rpl commented May 14, 2018 • edited Loading

james-cnz May 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

james-cnz May 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

james-cnz May 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpl commented May 18, 2018

Rob--W left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ExE-Boss commented Jun 4, 2018

rpl commented May 14, 2018 •

edited

Loading

james-cnz May 15, 2018 •

edited

Loading

james-cnz May 15, 2018 •

edited

Loading

james-cnz May 15, 2018 •

edited

Loading