Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from Jun 2, 2018

Conversation

rpl
Copy link
Member

@rpl rpl commented May 14, 2018

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)

fileName: error.fileName,
lineNumber: error.lineNumber,
},
});
Copy link

@james-cnz james-cnz May 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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! ❤️

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

@james-cnz james-cnz May 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should chrome.runtime.lastError be checked in here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 )

.travis.yml Outdated
TRAVIS_CI=true ./test/run-chrome-smoketests.sh
TRAVIS_CI=true ./test/run-browsers-smoketests.sh

sudo: required
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment should say: // See issue #85

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:+1 done

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

.forBrowser("chrome")
.setChromeOptions(options)
.setChromeService(new chrome.ServiceBuilder(chromedriver.path))
.build();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uhm, yeah, sounds good to me

// 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");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

// 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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');

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 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.

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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@james-cnz james-cnz May 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

rpl commented May 18, 2018

@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.

Copy link
Member

@Rob--W Rob--W left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/send/sends/

// 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"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/reject/rejects/

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

valua?

@@ -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"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary braces around await ....

}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 added 3 commits June 2, 2018 20:49
…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
@rpl rpl force-pushed the test/cross-browsers-smoke-tests branch from 354ebc9 to b16ee4f Compare June 2, 2018 18:56
@rpl rpl merged commit 5d186ba into mozilla:master Jun 2, 2018
@ExE-Boss ExE-Boss mentioned this pull request Jun 4, 2018
@ExE-Boss
Copy link
Contributor

ExE-Boss commented Jun 4, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants