test(integration): Add support for Edge to the integration test suite #163

rpl · 2018-11-26T14:39:20Z

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)

Rob--W · 2018-11-26T15:42:32Z

scripts/run-browsers-smoketests.js

@@ -0,0 +1,29 @@
+const shell = require("shelljs");
+
+// set -eo pipefail


Remove this comment, as it is not doing anything.

Rob--W · 2018-11-26T15:54:49Z

src/browser-polyfill.js

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


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

Rob--W · 2018-11-26T15:55:33Z

test/fixtures/detect-existing-browser-api-object/content.js


  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


"On chrome" -> "On Edge"?

Rob--W · 2018-11-26T15:58:28Z

test/fixtures/detect-existing-browser-api-object/manifest.json

  },
  "content_scripts": [
    {
      "matches": [
-        "http://localhost/*"
+        "http://*/*"


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 )

Rob--W · 2018-11-26T16:00:29Z

test/integration/setup.js

+    const destExtensionDir = path.join(edgeLocalStatePath, path.basename(extensionPath) + "-" + Date.now());
+
+    const shell = require("shelljs");
+    console.log("Copying test extension from", extensionPath, "to", destExtensionDir);


Use template literals.

Suggested change

console.log("Copying test extension from", extensionPath, "to", destExtensionDir);

console.log(`Copying test extension from ${extensionPath} to ${destExtensionDir}`);

Rob--W · 2018-11-26T16:18:32Z

test/integration/setup.js

          new Promise((resolve, reject) => {
            timeout = setTimeout(() => reject(new Error(`test timeout after ${TEST_TIMEOUT}`)), TEST_TIMEOUT);
          }),
        ]);
      } finally {
        clearTimeout(timeout);
        if (driver) {
+          if (exit === false) {


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.

Rob--W · 2018-11-26T16:30:03Z

test/integration/setup.js

+        if (skip === true) {
+          tt.skip("Test extension skipped");
+          return;
+        } else if (skip === browser || skip.includes(browser)) {


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.

Rob--W · 2018-11-26T16:33:53Z

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

-  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/")) {


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.

Rob--W · 2018-11-26T16:35:41Z

test/integration/setup.js

+      await cleanup();
+    };
+    driver.quit = async () => {
+      // await new Promise(() => {});


Stray comment?

Rob--W · 2018-11-26T16:39:13Z

scripts/run-browsers-smoketests.js

+`);
+
+if (process.platform == "win32" && process.env.EDGE_DRIVER_PATH) {
+  process.env.PATH += `;${process.env.EDGE_DRIVER_PATH}`;


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.

ExE-Boss

GitHub suggestions for @Rob--W’s comments.

(using the new suggestion syntax:

```suggestion
…
```

ExE-Boss · 2019-01-06T08:45:50Z

test/integration/setup.js

+    const destExtensionDir = path.join(edgeLocalStatePath, path.basename(extensionPath) + "-" + Date.now());
+
+    const shell = require("shelljs");
+    console.log("Copying test extension from", extensionPath, "to", destExtensionDir);


Suggested change

console.log("Copying test extension from", extensionPath, "to", destExtensionDir);

console.log(`Copying test extension from ${extensionPath} to ${destExtensionDir}`);

ExE-Boss · 2019-01-06T08:48:21Z

test/integration/setup.js

@@ -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 => {


Suggested change

const onceWritten = new Promise(resolve => {

const onceWritten = new Promise((resolve, reject) => {

destFileStream.on("error", reject);

rpl · 2019-01-07T12:47:46Z

Closing as wontfix. See #114 (comment) for a rationale.

test(integration): Add support for Edge to the integration test suite.

a7a8e41

rpl requested a review from Rob--W November 26, 2018 14:39

Rob--W requested changes Nov 26, 2018

View reviewed changes

ExE-Boss reviewed Jan 6, 2019

View reviewed changes

rpl mentioned this pull request Jan 7, 2019

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

Closed

rpl added platform: edge state: wontfix labels Jan 7, 2019

rpl closed this Jan 7, 2019

ExE-Boss mentioned this pull request Jan 8, 2019

test(integration): Use node for running test scripts #167

Merged

rpl deleted the test/run-integration-test-on-edge branch March 15, 2021 10:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(integration): Add support for Edge to the integration test suite #163

test(integration): Add support for Edge to the integration test suite #163

rpl commented Nov 26, 2018

Rob--W Nov 26, 2018

Rob--W Nov 26, 2018

Rob--W Nov 26, 2018

Rob--W Nov 26, 2018

Rob--W Nov 26, 2018

ExE-Boss Jan 6, 2019

Rob--W Nov 26, 2018

Rob--W Nov 26, 2018

Rob--W Nov 26, 2018

Rob--W Nov 26, 2018

Rob--W Nov 26, 2018

ExE-Boss left a comment

ExE-Boss Jan 6, 2019

ExE-Boss Jan 6, 2019

rpl commented Jan 7, 2019

		@@ -0,0 +1,29 @@
		const shell = require("shelljs");

		// set -eo pipefail

	console.log("Copying test extension from", extensionPath, "to", destExtensionDir);
	console.log(`Copying test extension from ${extensionPath} to ${destExtensionDir}`);

	const onceWritten = new Promise(resolve => {
	const onceWritten = new Promise((resolve, reject) => {
	destFileStream.on("error", reject);

test(integration): Add support for Edge to the integration test suite #163

test(integration): Add support for Edge to the integration test suite #163

Conversation

rpl commented Nov 26, 2018

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

ExE-Boss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpl commented Jan 7, 2019