1301454 - [FlyWeb] Implement integration tests

Status: RESOLVED WONTFIX

(Core Graveyard :: DOM: Flyweb, defect)

Description

[Justin D'Arcangelo [:justindarc]]

We need to implement a set of basic integration tests around the FlyWeb functionality to prevent accidental regressions from happening like the one in Bug 1301091.

Comment 1

[Kannan Vijayan [:djvj]]

Attachment: flyweb-test-suite.patch

Incomplete draft patch for flyweb test suite.

Comment 2

[Kannan Vijayan [:djvj]]

Attachment: flyweb-test-suite.patch

Still a preliminary patch, draft status, but does all the right things (goes through a full lifecycle of:

- Open server tab to server page
- Have server tab call publishServer
- Automatically allow the user prompt for publishServer
- Discover the published service
- Pair with the discovered service
- Open a client tab that connects to and receives a page from the service

Also, this patch includes a fix (that needs to be factored out into a separate patch) for a shutdown leak that was discovered during testing.

Comment 3

[Kannan Vijayan [:djvj]]

Comment on attachment 8791762 [details] [diff] [review]
flyweb-test-suite.patch

Review of attachment 8791762 [details] [diff] [review]:
-----------------------------------------------------------------

Hey conley,

Could you take look at this patch (ignore the stuff in FlyWebService.cpp, that's a leak-fixing change that I've moved over to bug 1303856).

The patch still needs a bit of cleanup, but I wanted to confirm that the overall structure of it is worthwhile.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8791762 [details] [diff] [review]
flyweb-test-suite.patch

Review of attachment 8791762 [details] [diff] [review]:
-----------------------------------------------------------------

I think this is a reasonable start, and is going in the right direction. I think there are some built-in utilities that you might want to take advantage of, along with some alternatives for knowing when things occur (like when the permission request is shown, and how to know when content has successfully published the server).

::: dom/flyweb/FlyWebService.cpp
@@ +907,5 @@
>  FlyWebService::GetOrCreate()
>  {
>    if (!gFlyWebService) {
>      gFlyWebService = new FlyWebService();
> +    ClearOnShutdown(&gFlyWebService);

Fragments of bug 1303865, I imagine. Probably best to get this out of here.

::: dom/flyweb/test/browser_flyweb_publishserver.js
@@ +1,5 @@
> +
> +/*XPCOMUtils.defineLazyModuleGetter(this, "Promise",
> +  "resource://gre/modules/Promise.jsm");*/
> +XPCOMUtils.defineLazyModuleGetter(this, "Task",
> +  "resource://gre/modules/Task.jsm");

Dead-code (Promise.jsm import).

You don't need to import Task.jsm - add_task is given to you for free.

@@ +7,5 @@
> +
> +const gHttpTestRoot = "http://example.com/browser/dom/flyweb/test/";
> +
> +function resourceURL(name) {
> +  return gHttpTestRoot + name;

Might as well just set the URL as a constant, like

const TEST_PAGE = "http://example.com/browser/dom/flyweb/test/file_flyweb_hello_world.html";

above.

@@ +30,5 @@
> +  info("getPublishServerPopupNotification: FlyWeb child node not found!");
> +  return null;
> +}
> +
> +function waitForAndAllowPublishServerPopupNotification() {

Instead of polling, I think you can wait for the "popupshowing" event to fire on PopupNotifications.panel. Something like:

```JavaScript

function* waitForAndAllowPublishServerRequest() {
  yield BrowserTestUtils.waitForEvent(PopupNotifications.panel, "popupshowing");
  Assert.equal(PopupNotifications.panel.childNodes, 1,
               "Should be showing a notification.");
  let notification = PopupNotifications.panel.childNodes[0].notificiation;
  let popupHidden = BrowserTestUtils.waitForEvent(PopupNotifications.panel, "popuphidden");
  notification.button.click();
  yield popupHidden;
}

```

That's going to click on the Allow button, which is the main action by default. If you want to click on Deny, check out these tests I've got under review, since they do something similar: https://reviewboard.mozilla.org/r/75074/diff/4#1 (specifically, "clickSecondaryAction").

@@ +48,5 @@
> +  }
> +  return new Promise(helper);
> +}
> +
> +function waitForEventOnElement(elem, eventName, message) {

This sort of thing is available in BrowserTestUtils. Give BrowserTestUtils.jsm a read - lots of useful utilities in there (with documentation!) you might want to use.

@@ +100,5 @@
> +  });
> +}
> +
> +add_task(function* () {
> +  waitForExplicitFinish();

Not necessary if you're using add_task.

@@ +103,5 @@
> +add_task(function* () {
> +  waitForExplicitFinish();
> +
> +  info("Setting pref dom.flyweb.enabled to true");
> +  Services.prefs.setBoolPref("dom.flyweb.enabled", true);

Generally speaking, it's a good idea to use SpecialPowers.pushPrefEnv, which will pop the pref off once the test exits (even if it throws). This is a good way of making sure that prefs don't accidentally stick around from test to test too.

SpecialPowers.pushPrefEnv returns a Promise, so you can use:

```JavaScript

yield SpecialPowers.pushPrefEnv({
  set: [
    ["dom.flyweb.enabled", true],
  ]
});

```

@@ +116,5 @@
> +  info("Waiting for popup notification for publishServer.");
> +  yield waitForAndAllowPublishServerPopupNotification();
> +  info("Received and allowed popup notification for publishServer.");
> +
> +  info("KVKV: SERVER DOC=" + serverBrowser.contentDocument);

Accessing contentDocument directly is going to go over CPOWs, which we want to try to avoid in newer tests.

If you need to muck about with contentDocument or contentWindow, a ContentTask is better. Example:

```JavaScript

yield ContentTask.spawn(serverTab.linkedBrowser, null, function*() {
  // This function is serialized and sent down to the browser argument's
  // content over the message manager, and then executed. You have chrome-privileges
  // here, but no access to the outer scope.
  content.document.getElementById("ready").addEventListener("click"... //etc
});


```

The button click thing seems more like a hack though. Maybe better to do a postMessage from within file_flyweb_hello_world.html, and then addEventListener("message"...) in a ContentTask?

@@ +133,5 @@
> +  let clientTab = yield loadNewTab(pairedSvc.uiUrl);
> +
> +  yield new Promise((resolve, reject) => {
> +    info("Waiting 20 seconds to see services.");
> +    setTimeout(resolve, 20000);

Not sure what this 20s timeout is for. It doesn't look like we're doing anything except waiting to close those tabs?

@@ +140,5 @@
> +  //yield new Promise((resolve, reject) => {
> +  //  waitForServerPageReady(serverTab.linkedBrowser.contentDocument, resolve, reject)
> +  //});
> +
> +  gBrowser.removeTab(clientTab);

You should use yield BrowserTestUtils.removeTab(clientTab); so that the tabs are really fully closed (otherwise their cleanup stuff might still be underway and leak into the next test, etc).

@@ +145,5 @@
> +  gBrowser.removeTab(serverTab);
> +
> +  // Nothing for now.
> +  ok(true, "Ran test.");
> +  finish();

Not necessary if you're using add_task.

Comment 5

[Kannan Vijayan [:djvj]]

Attachment: flyweb-browser-test.patch

Thanks for the suggestions, mike.  This is an updated patch with comments addressed, and random debugging crap taken out.  This should be good for r? now.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8793444 [details] [diff] [review]
flyweb-browser-test.patch

Review of attachment 8793444 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great - just some cosmetic / documentation stuff. Thanks for the test!

::: dom/flyweb/test/browser_flyweb_publishserver.js
@@ +1,1 @@
> +

Please add "use strict"; at the top.

Also, underneath that, please add a docstring that describes what sorts of thing the tests in this file should be testing.

@@ +1,2 @@
> +
> +const gHttpTestRoot = "http://example.com/browser/dom/flyweb/test/";

Seems a bit odd to have these two different variable naming conventions. Maybe make this first one TEST_ROOT instead.

@@ +1,5 @@
> +
> +const gHttpTestRoot = "http://example.com/browser/dom/flyweb/test/";
> +const TEST_PAGE = gHttpTestRoot + "file_flyweb_hello_world.html";
> +
> +function loadNewTab(url) {

Each of these helper functions needs documentation describing their function, inputs, outputs, and potential side-effects.

@@ +8,5 @@
> +
> +function* waitForAndAllowPublishServerRequest() {
> +  yield BrowserTestUtils.waitForEvent(PopupNotifications.panel, "popupshowing");
> +  Assert.equal(PopupNotifications.panel.childNodes.length, 1,
> +               "PopupNotifications should be sowing a single notification.");

typo: "sowing" -> "showing"

@@ +9,5 @@
> +function* waitForAndAllowPublishServerRequest() {
> +  yield BrowserTestUtils.waitForEvent(PopupNotifications.panel, "popupshowing");
> +  Assert.equal(PopupNotifications.panel.childNodes.length, 1,
> +               "PopupNotifications should be sowing a single notification.");
> +  let notification = PopupNotifications.panel.childNodes[0]; //.notification;

Accidentally left that comment? If not, I don't think it adds much and should be removed anyways.

@@ +18,5 @@
> +  yield popupHidden;
> +}
> +
> +function* waitForPostMessageOnBrowser(browser, messageName) {
> +  yield ContentTask.spawn(browser, {messageName}, function*({messageName}) {

Nit: please put spaces on either side of the variable when doing { foo }, like:

{ messageName }, function*({ messageName }) {

@@ +24,5 @@
> +      let timeoutId;
> +
> +      let listener = (event) => {
> +        if (event.data == messageName) {
> +          if (timeoutId) { clearTimeout(timeoutId); }

Please break this up over several lines. I know it's a one-liner, but generally we like to have them on separate lines anyways.

@@ +33,5 @@
> +        content.removeEventListener("message", listener);
> +        reject("Timed out waiting for message.");
> +      };
> +  
> +      timeoutId = setTimeout(timer, 10000);

I don't actually believe setTimeout is available in ContentTasks... normally you have to import, like:

Cu.import("resource://gre/modules/Timer.jsm");

Are you certain this code is running?

@@ +56,5 @@
> +                return;
> +              }
> +              donePairing = true;
> +              info("Pairing succeeded: " + JSON.stringify(pairedSvc));
> +              setTimeout(() => {

Why is the setTimeout required, out of curiosity?

@@ +79,5 @@
> +    });
> +  });
> +}
> +
> +add_task(function* () {

Please add some documentation above this add_task describing what exactly this individual test does.

Like:

/**
 * Tests that foo the bar lorem ipsum. Also makes sure that the frizbats
 * and the flibbity bop makes sure the etc.
 */

And please also give the function a name, like:

add_task(function* test_basic_pairing() {
  // ...
});

@@ +81,5 @@
> +}
> +
> +add_task(function* () {
> +  info("Setting pref dom.flyweb.enabled to true");
> +  yield new Promise(resolve =>

Thankfully, SpecialPowers.pushPrefEnv already returns a Promise when you don't supply a callback, so you can just do:

yield SpecialPowers.pushPrefEnv(...

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Oh, and a green try push across Linux, Windows and OS X would be excellent. :)

Comment 8

[Kannan Vijayan [:djvj]]

Pushed one yesterday: https://treeherder.mozilla.org/#/jobs?repo=try&revision=43e42ae7623e&selectedJob=27830714

Getting timeouts on Linux64 and OSX.

Comment 9

[Kannan Vijayan [:djvj]]

(In reply to Mike Conley (:mconley) - (needinfo me!) from comment #6)
> Comment on attachment 8793444 [details] [diff] [review]
> I don't actually believe setTimeout is available in ContentTasks... normally
> you have to import, like:
> 
> Cu.import("resource://gre/modules/Timer.jsm");
> 
> Are you certain this code is running?

Nope.  Mostly I added it as a debug mechanism that would tell me if that part of the code was taking too long, but I removed it entirely in the latest patch.

> @@ +56,5 @@
> > +                return;
> > +              }
> > +              donePairing = true;
> > +              info("Pairing succeeded: " + JSON.stringify(pairedSvc));
> > +              setTimeout(() => {
> 
> Why is the setTimeout required, out of curiosity?

Calling FlyWebDiscoveryManager.stopDiscovery() inside a callback initiated with a startDiscovery() can sometimes crash.  I think we need a judicious use of a Runnable somewhere, but I'm not sure.  We just make sure that stopDiscovery() happens outside of the callback's dynamic scope (which is the normal case since stopDiscovery is called by the UI when the FlyWeb menu is dismissed).

Comment 10

[Kannan Vijayan [:djvj]]

Attachment: flyweb-browser-test.patch

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8793913 [details] [diff] [review]
flyweb-browser-test.patch

Review of attachment 8793913 [details] [diff] [review]:
-----------------------------------------------------------------

Code looks good - just some cosmetic stuff, mostly documentation related. Good job on the test!

Happy to review again if you feel like it needs it.

::: dom/flyweb/test/browser_flyweb_publishserver.js
@@ +1,1 @@
> +

Remove the newline at the top.

@@ +5,5 @@
> +const TEST_PAGE = TEST_ROOT + "file_flyweb_hello_world.html";
> +
> +/**
> + * Loads the given url in a new tab, and returns a Promise that
> + * resolves to the object for that tab.

Probably also best to mention that url is a string.

@@ +30,5 @@
> +  yield popupHidden;
> +}
> +
> +/**
> + * Given a browser object, and a message name, 

Trailing white space.

@@ +31,5 @@
> +}
> +
> +/**
> + * Given a browser object, and a message name, 
> + * call, then allows it.

Lines 36-38 don't make tooooo much sense. Copy paste?

@@ +51,5 @@
> +}
> +
> +/**
> + * Uses FlyWebDiscoveryManager to look for the service published
> + * by the server page, and estbalishes a connection to it.

typo: "estbalishes" -> "establishes"

@@ +73,5 @@
> +                return;
> +              }
> +              donePairing = true;
> +              info("Pairing succeeded: " + JSON.stringify(pairedSvc));
> +              setTimeout(() => {

Can you document why the setTimeout is necessary here in the bug, perhaps reference a bug number for using a runnable in the future or something?

@@ +84,5 @@
> +                return;
> +              }
> +              donePairing = true;
> +              info("Pairing failed: " + error);
> +              setTimeout(() => {

Same as above here. It's just kinda mysterious for future hackers.

::: dom/flyweb/test/file_flyweb_hello_world.html
@@ +1,5 @@
> +<html>
> +<head>
> +  <title>FlyWeb HELLO WORLD.</title>
> +  <script type="text/javascript">
> +

If you have a second, would be good to have documentation in this file as well as to what this file does exactly.

Comment 12

[Kannan Vijayan [:djvj]]

Ok.  The way we were handling popupnotification in the previous patch is not correct, and prone to races where the popup event happens before the handlers are installed.  I have a really debug-spew-infested updated version of the patch that fixes this.  Will clean it up and post it today.

Comment 13

[Kannan Vijayan [:djvj]]

Attachment: flyweb-browser-test.patch

Updated test with better PopupNotification handling.  Try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4af3dda3ba7246f73bc8448113f55e6f19969612