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(integration): Add support for Edge to the integration test suite #163
Conversation
@@ -0,0 +1,29 @@ | |||
const shell = require("shelljs"); | |||
|
|||
// set -eo pipefail |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this comment, as it is not doing anything.
@@ -275,6 +275,14 @@ if (typeof browser === "undefined" || Object.getPrototypeOf(browser) !== Object. | |||
return value; | |||
} | |||
|
|||
// Use defineProperty to assign the prop value to the cache object, | |||
// Edge throws when the __proto__ property is assigned in strict mode. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where are we relying on spoofing __proto__
?
I think that a cleaner solution is to have logic along the lines of:
if (prop === "__proto__") {
return Object.getPrototypeOf(target);
}
|
||
if (navigator.userAgent.includes("Firefox/")) { | ||
// Check that the polyfill didn't create a polyfill wrapped browser API object on Firefox. | ||
t.equal(browser, originalAPIObjects.browser, "browser API object should not be changed on Firefox"); | ||
// On Firefox, window is not the global object for content scripts, and so we expect window.browser to not | ||
// be defined. | ||
t.equal(window.browser, undefined, "window.browser is expected to be undefined on Firefox"); | ||
} else if (navigator.userAgent.includes("Edge/")) { | ||
// On chrome, window is the global object and so the polyfilled browser API should |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"On chrome" -> "On Edge"?
}, | ||
"content_scripts": [ | ||
{ | ||
"matches": [ | ||
"http://localhost/*" | ||
"http://*/*" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could it be that Edge requires a port too? So would changing the pattern to http://localhost:*/*
have the desired result?
(If that is the case, then both Edge and Chrome recognize ports in URL patterns, and we should consider recognizing ports in Firefox too - https://bugzilla.mozilla.org/show_bug.cgi?id=1362809 )
const destExtensionDir = path.join(edgeLocalStatePath, path.basename(extensionPath) + "-" + Date.now()); | ||
|
||
const shell = require("shelljs"); | ||
console.log("Copying test extension from", extensionPath, "to", destExtensionDir); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use template literals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
console.log("Copying test extension from", extensionPath, "to", destExtensionDir); | |
console.log(`Copying test extension from ${extensionPath} to ${destExtensionDir}`); |
new Promise((resolve, reject) => { | ||
timeout = setTimeout(() => reject(new Error(`test timeout after ${TEST_TIMEOUT}`)), TEST_TIMEOUT); | ||
}), | ||
]); | ||
} finally { | ||
clearTimeout(timeout); | ||
if (driver) { | ||
if (exit === false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is exit
explicitly set to false
? It doesn't seem to be used this PR.
Even if you do end up using exit
, I think that the server
cleanup logic below should run too (currently it does not due to the use of a never-resolving Promise.
By the construction of this logic, it looks like auxilary debugging code that helped during development, but shouldn't be checked in.
if (skip === true) { | ||
tt.skip("Test extension skipped"); | ||
return; | ||
} else if (skip === browser || skip.includes(browser)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This adds three ways to disable a test: true
, string, array of strings. Onle the string value is used (skip: "edge"
). If there is no use for the other two, let's keep it simple for now and only use the string value.
Or remove this alltogether and skip the test in the description inside the test file itself.
if (navigator.userAgent.includes("Firefox/")) { | ||
t.deepEqual(res, undefined, "Got an undefined value as expected"); | ||
} else { | ||
if (navigator.userAgent.includes("Chrome/") && !navigator.userAgent.includes("Edge/")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is because Edge pretends to be Chrome, right?
The intent might be a bit clearer if you do UA sniffing in one location (e.g. test/fixtures/tape-standalone.js ) and then export the environment type to a variable (e.g. TEST_BROWSER_NAME
or something like that). I'm using a variable instead of a property so that if you mistakenly use this global variable in a context where it is not defined, that a ReferenceError is triggered instead of getting the undefined
value.
await cleanup(); | ||
}; | ||
driver.quit = async () => { | ||
// await new Promise(() => {}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stray comment?
`); | ||
|
||
if (process.platform == "win32" && process.env.EDGE_DRIVER_PATH) { | ||
process.env.PATH += `;${process.env.EDGE_DRIVER_PATH}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this environment variable documented? If it is only used as a convenience to set the location of the Edge driver, shouldn't this be in the test-integration implementation?
If the line is to be kept, let's move this line into the block with the same condition at the end of this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const destExtensionDir = path.join(edgeLocalStatePath, path.basename(extensionPath) + "-" + Date.now()); | ||
|
||
const shell = require("shelljs"); | ||
console.log("Copying test extension from", extensionPath, "to", destExtensionDir); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
console.log("Copying test extension from", extensionPath, "to", destExtensionDir); | |
console.log(`Copying test extension from ${extensionPath} to ${destExtensionDir}`); |
@@ -138,23 +174,40 @@ const bundleTapeStandalone = async (destDir) => { | |||
|
|||
const stream = b.bundle(); | |||
const onceStreamEnd = awaitStreamEnd(stream); | |||
stream.pipe(fs.createWriteStream(bundleFileName)); | |||
const destFileStream = fs.createWriteStream(bundleFileName); | |||
const onceWritten = new Promise(resolve => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const onceWritten = new Promise(resolve => { | |
const onceWritten = new Promise((resolve, reject) => { | |
destFileStream.on("error", reject); |
Closing as wontfix. See #114 (comment) for a rationale. |
This PR applies the changes needed to be able to run the entire integration test suite on Edge.
It is true that most windows CI services can't currently run Edge because it can't be installed on Windows server systems, which is quite unfortunate, but to be able to realistically "support the webextension-polyfill on Edge (even on a best-effort basis)"... the bare minimum is to at least ensure that all the existing tests can be run successfully on a regular Windows system).
And so this PR is propaedeutic to be able to complete and merge the PR #114, which is meant to allow the polyfill to support Edge.
In this PR that are also the following small changes:
on the polyfill sources, to fix another small issue when the polyfill implementation is running on Edge (which has not been caught yet in Add support for browsers which don’t support Promises #114, I noticed it while running the entire test suite on Edge)
define new npm scripts (to make it easier to run the integration tests on a single browser (e.g. "npm run test-integration:edge" will run the tests on just Edge)
rewritten one bash script into a nodejs script (to make it easier to run the integration tests across the supported browsers on windows, without the need to have a bash compatible shell available on the windows systems
fixed minor incompatibilities of the test extensions' manifest.json files (e.g. Edge will not run the extension if the manifest doesn't have an author property, or the persistent property is missing in the background page manifest section, and the content scripts are not running on the test server listening on localhost without changing the origin permission to "http:///")
Allow to skip an entire test extension (e.g. on some browsers)
Make it a bit easier to investigate issues in the test extensions (by temporarily asking the integration test suite to keep the controlled browser opened after the test is completed)