733631 - Unit test infrastructure for the webapp runtime

Status: RESOLVED FIXED

(Firefox Graveyard :: Webapp Runtime, defect, P1)

Description

[:Felipe Gomes (needinfo for replies!)]

We need to figure out a way to test the webapp runtime in continuous integration. I discussed this with ctalbert and we came up with some possibilities:

- the runtime would have a mode (possibly activated by a argv --flag) in which it tests itself and then returns an exit code to tell if the tests were successful or not.  The runtime can be invoked from xpcshell using nsIProcess and it's easy to get the return code from it.

- alternatively, some simple logging API can be exposed at the runtime, like is(), ok(), and log(), and these would log out to a file which the xpcshell will read after the runtime finishes running its tests. We can ifdef out this api from release builds

- other possibility that will be available soon (but is not right now) is to use Marionette on both sides to communicate between the test being run on xpcshell and the runtime.


I'm curious to what are the things that we need to test from the actual runtime instead of from Firefox. I believe that making sure the right page is opened according to the manifest, that the profile can be found, etc.. What else comes to mind?
Things like being properly installed, checking for registry keys, etc., can be done directly from xpcshell/mochitest

Comment 1

[Jason Smith [:jsmith]]

Note: Evolving test cases document needed to be automated will be documented here - https://docs.google.com/spreadsheet/ccc?key=0AiZoGR-iOAlUdDdfMHo5aTI2WjVIeWxnNWxvV0ZVOWc#gid=0

Comment 2

[Jason Smith [:jsmith]]

One part of the infrastructure that we're going to need is an "app directory" of test cases to have an easy location to install apps for different native test cases (e.g. easy place to do manual/automated testing against). Likely this will involve the following:

- Deployment of application test cases & associated manifests on different subdomains (e.g. testcase1.stage-myapps.mozillalabs.com)
- Create of an app directory that references each of the app test cases hosted

This will be need to be maintained overtime as new test cases arrive.

Thoughts? Does this idea make sense for the infrastructure?

Comment 3

[Jason Smith [:jsmith]]

Test plan being documented here:

https://wiki.mozilla.org/Web_Apps_integration/TestPlan

Comment 4

[Jason Smith [:jsmith]]

Re-evaluating what I said now in comment 2 in an effort to simplify the infrastructure. A simple approach to test using apps could just require turning mozqa.com/data/firefox into a web app. Then, we can run test cases within there easily.

However, this only addresses the use part of apps, not the other areas.

Comment 5

[Jason Smith [:jsmith]]

Rewording. This bug is going to track the implementation of the unit test infrastructure, not the functional test infrastructure. Will use other bugs to track this.

Comment 6

[Jennifer Arguello :ticachica]

Sounds like something that for sure is necessary, but not a blocker especially if things can be tested in a manual manner. Of course not ideal, but not impossible. :)

Comment 7

[Jason Smith [:jsmith]]

As a note to the upcoming triage - We really should get this implemented soon, as I feel a bit nervous that we have an implementation with a decent amount of code with no automation coverage.

Comment 8

[Drew Willcoxon :adw]

Myk asked me to work on this after Felipe was pulled away for other work.

As I understand it, there are two types of tests we need: tests for runtime chrome, and tests for content running in the runtime.

It seems to me that chrome tests are like browser-chrome mochitests, and content tests are like plain mochitests.  So I've been looking at modifying the mochitest framework to accommodate webapps.  I made a proof of concept for chrome tests by modifying testing/mochitest/runtests.py and the harness files related to browser-chrome tests.

Content tests are a little trickier since each test file that you write would basically need to be installed as a webapp.  I'm imagining that you'd give your mochitest HTML file to the harness, and the harness would create a profile directory, write a mock webapp.json to it, and launch the stub executable passing -profile (and an arg called -firefox-path that pointed to a place where the executable could find a GRE).  Once XRE_main starts, the runtime tries to read webapp.json in AppRegD and fails.  So then it's basically in testing mode and looks in the profile directory, and finding webapp.json there, it installs the mochitest app and starts it.  (So the app is registered in the same profile that the runtime is using, which I'm not 100% sure is not a problem but so far doesn't look like it.)

There are lots of details I need to work out, like how the mochitest HTML file is provided to the runtime, e.g., it's packaged in the extension that mochitest installs in the profile, or it's served by the test server, etc.

Comment 9

[Jason Smith [:jsmith]]

FYI to Drew - During specification of my functional test automation framework, we discovered that we cannot rely on any testing framework that relies on add-ons to operate, as web apps do not support add-ons.

Comment 10

[Drew Willcoxon :adw]

mochitest installs an extension that contains test-related files into the profile directory and it seems to work fine with the webapp runtime if the runtime is started with that profile.

Comment 11

[David Burns :automatedtester]

@Drew

The ateam is working on a addon-less test[1] framework that can be used within our infrastructure, for B2G originally, but will also be used on desktop. It's called Marionette[1].

Since this is going to be using items out of MozBase[2] to drop things where we need it. If having an addon installed is acceptable, and for the main unit tests I am sure it would be, then mochitest is a good tool. 

If we need to have it work for end to end tests without addons, which is what Jason asked for in a meeting with the Ateam,  then marionette is a better bet.

Let me know if I can help either way

[1] https://wiki.mozilla.org/Auto-tools/Projects/Marionette
[2] https://wiki.mozilla.org/Auto-tools/Projects/MozBase

Comment 12

[Drew Willcoxon :adw]

Thanks David.

> If we need to have it work for end to end tests without addons, which is
> what Jason asked for in a meeting with the Ateam,  then marionette is a
> better bet.

My understanding is that this bug is just about unit tests, specifically tests for runtime chrome and content running in the runtime.

Comment 13

[Myk Melez [:myk] [@mykmelez]]

We plan to disable addons in the runtime (bug 748967 is about one aspect of this), but we might still be able to enable them for testing purposes, depending on how we end up disabling them.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #13)
> We plan to disable addons in the runtime (bug 748967 is about one aspect of
> this), but we might still be able to enable them for testing purposes,
> depending on how we end up disabling them.

This bug is about globally installed add-ons. What about add-ons in the profile?

Comment 15

[Myk Melez [:myk] [@mykmelez]]

(In reply to Henrik Skupin (:whimboo) from comment #14)
> (In reply to Myk Melez [:myk] [@mykmelez] from comment #13)
> > We plan to disable addons in the runtime (bug 748967 is about one aspect of
> > this), but we might still be able to enable them for testing purposes,
> > depending on how we end up disabling them.
> 
> This bug is about globally installed add-ons. What about add-ons in the
> profile?

We're going to disable those too.

Comment 16

[Drew Willcoxon :adw]

Uh can we not do that?  Or whoever does do it please design for a backdoor for tests?

Comment 17

[Jason Smith [:jsmith]]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #15)
> (In reply to Henrik Skupin (:whimboo) from comment #14)
> > (In reply to Myk Melez [:myk] [@mykmelez] from comment #13)
> > > We plan to disable addons in the runtime (bug 748967 is about one aspect of
> > > this), but we might still be able to enable them for testing purposes,
> > > depending on how we end up disabling them.
> > 
> > This bug is about globally installed add-ons. What about add-ons in the
> > profile?
> 
> We're going to disable those too.

Is that currently implemented? If not, then we should probably get a bug filed on that.

Comment 18

[Drew Willcoxon :adw]

Attachment: chrome and content mochitests proof-of-concept

This adds a self-install mode to the (Mac) stub executable and runtime.  You pass the executable an -install-url, the runtime navigates to it, the page there calls mozApps.install, and once installed the runtime navigates to the new app.

This also adds --webapprt-content and --webapprt-chrome modes to mochitest's runtests.py.  In both cases you have a toy webapp you want to test.  runtests.py installs your webapp using the self-install mode, and then your tests test it.  In the content case, your toy webapp and your test are the same thing.

Content tests are mochitest plain tests.  Pass the file that installs your test as --test-path:

python obj-debug/_tests/testing/mochitest/runtests.py --appname obj-debug/dist/NightlyDebug.app/Contents/MacOS/webapprt-stub --xre-path obj-debug/dist/NightlyDebug.app/Contents/MacOS/ --browser-arg -firefox-path --browser-arg /Users/adw/m-c/obj-debug/dist/NightlyDebug.app --webapprt-content --test-path webapprtContent/test_install.html

Chrome tests are basically browser-chrome tests.  This time pass your test as --test-path and your installation page as -install-url:

python obj-debug/_tests/testing/mochitest/runtests.py --appname obj-debug/dist/NightlyDebug.app/Contents/MacOS/webapprt-stub --xre-path obj-debug/dist/NightlyDebug.app/Contents/MacOS/ --browser-arg -firefox-path --browser-arg /Users/adw/m-c/obj-debug/dist/NightlyDebug.app --browser-arg -install-url --browser-arg file:///Users/adw/m-c/obj-debug/_tests/testing/mochitest/webapprtChrome/foo_install.html --webapprt-chrome --test-path browser_foo.js

There are still some kinks to work out, like waiting for the installation to finish in the chrome case, but it basically works.

Comment 19

[Myk Melez [:myk] [@mykmelez]]

(In reply to Drew Willcoxon :adw from comment #16)
> Uh can we not do that?  Or whoever does do it please design for a backdoor
> for tests?

I chatted with ticachica, who is ok with making an exception for a unit test extension (although she would like to see us remove that dependency in a future version of the unit test framework, which it sounds like is also a goal of Marionette).


(In reply to Drew Willcoxon :adw from comment #18)
> Created attachment 623389 [details] [diff] [review]
> chrome and content mochitests proof-of-concept
> 
> This adds a self-install mode to the (Mac) stub executable and runtime.

Ooh!  How close is this proof-of-concept to being ready for review and integration?

Comment 20

[Myk Melez [:myk] [@mykmelez]]

(In reply to Jason Smith [:jsmith] from comment #17)
> Is that currently implemented? If not, then we should probably get a bug
> filed on that.

Filed as bug 754915.

Comment 21

[Drew Willcoxon :adw]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #19)
> Ooh!  How close is this proof-of-concept to being ready for review and
> integration?

These tests need a home, so I need to add a tests directory to webapprt/ and add targets to whatever webapprt/ makefiles necessary that execute runtests.py with the right arguments.  I also need to write some realistic tests to shake out problems with the proof of concept, particularly problems due to the novel kink that WebappRT tests must install a webapp first.  (I mentioned one such problem in comment 18, and I've seen others.)

Beyond that, the Windows stub executable needs to be made to support self-install mode like the Mac's, and the larger Mozilla integration infrastructure needs to be told about WebappRT tests so they can be run along with all the various other types of Mozilla tests.  I think these are discrete tasks and can wait for follow-up patches.

Comment 22

[Drew Willcoxon :adw]

Attachment: chrome and content mochitests work in progress

This adds webapprt/test/content and webapprt/test/chrome directories and webapprt-test-content and webapprt-test-chrome make targets.  There is a sample test for each.  So you can run them like this:

make -C objdir webapprt-test-content TEST_PATH=webapprt/test
make -C objdir webapprt-test-chrome TEST_PATH=webapprt/test

This is close to review, but there are two remaining problems I know of and need to figure out, both related to integration within the larger Mozilla testing infrastructure, and both due to the fact that WebappRT content tests are just mochitest plain tests.  (1) The mochitest-plain make target picks up the WebappRT content tests, and (2) the webapprt-test-content make target picks up all the mochitest plain tests in the tree if you don't specify a TEST_PATH.

Comment 23

[Drew Willcoxon :adw]

Attachment: chrome and content mochitests

Felipe, could you review the changes this patch makes to WebappRT, along with the overall approach?  This modifies the stub executable and runtime to accept a -content-url command-line argument, which tells the runtime to navigate to a specific URL instead of some webapp installed on the system like normal.  The content can then install apps.  There are sample chrome and content tests so you can see how they work.

This only changes the Mac executable.  I'm planning on doing the Windows executable later if this approach is OK.

Comment 24

[Drew Willcoxon :adw]

Comment on attachment 628506 [details] [diff] [review]
chrome and content mochitests

Ted, could you review the changes that this patch makes to the mochitest framework and testsuite-targets.mk, or point me to someone who could?  This adds two new types of tests, WebappRT chrome and content tests.  Chrome tests are basically browser-chrome mochitests, and content tests are basically plain mochitests.  They just run in the WebappRT runtime instead of the browser.  I had to use a new "webapprt_" filename prefix for content tests because otherwise the plain mochitest framework would pick up WebappRT content tests and vice versa.

Comment 25

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 628506 [details] [diff] [review]
chrome and content mochitests

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

Thanks Drew, that's a great patch!

I'm going through reviewing all the webapprt/ changes, but just wanted to post a quick note ahead of time: please take a look at bug 749029 and bug 752666 as they touch things that you are using here. The former is changing when Webapps.jsm gets loaded (although in practice it's just a code move, but I still haven't figured out why you need it to only happen after webapprt-command-line, so that might be relevant), and the latter is getting rid of most of the ContentPolicy code, so it would be good to coordinate with Myk if you need any of that to be kept.

Also I don't know much about the webapprt.mm so Myk should take a look at it.

Comment 26

[Drew Willcoxon :adw]

(In reply to Felipe Gomes (:felipe) from comment #25)
> The former is changing when Webapps.jsm gets loaded (although
> in practice it's just a code move, but I still haven't figured
> out why you need it to only happen after webapprt-command-line,

In short, because WebappRT.config isn't ready before that point.

In long: Webapps.jsm init()s DOMApplicationRegistry as soon as the file is evaluated.  init() gets the "WebappRegD" directory, which is handled by webapprt's DirectoryProvider.js.  DirectoryProvider.getFile() accesses WebappRT.config in that case.  The WebappRT.config() getter tries to load and use the webapp.json file, but it doesn't exist, so the getter throws an exception.  By waiting for the "webapprt-command-line" notification, WebappRT.config can be safely accessed because the webapprt-command-line observer in WebappRT.jsm will have set it to the right value.

Comment 27

[Drew Willcoxon :adw]

Comment on attachment 628506 [details] [diff] [review]
chrome and content mochitests

Myk, Felipe suggested that you look at the webapprt.mm changes here.  Also, I want to make sure that the framework implemented in this patch is what you had in mind when you asked me to work on this.  Let me know if you have any questions.

Comment 28

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 628506 [details] [diff] [review]
chrome and content mochitests

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

(In reply to Drew Willcoxon :adw from comment #26)
> (In reply to Felipe Gomes (:felipe) from comment #25)
> > The former is changing when Webapps.jsm gets loaded (although
> > in practice it's just a code move, but I still haven't figured
> > out why you need it to only happen after webapprt-command-line,
> 
> In short, because WebappRT.config isn't ready before that point.

Is this needed due to how WebappRT.config is being frozen to not be changed later? I'm wondering if removing that restriction would remove the need for this.  Or maybe it's needed because ContentPolicy had a whitelist of allowed domains, which is going away and would also make this unnecessary. Basically I'm trying to figure out a way to not have to add this back-and-forth of observers during start-up as it will ma
ke things a bit hard to follow.
But I realize that this might also be necessary for the webapps-ask-install/did-install flow..

::: webapprt/DirectoryProvider.js
@@ +23,1 @@
>  

Could the same observer in WebappRT.jsm set a property for this so that you can just check WebappRT.inContentURLMode below and not have to have this observer here?

Also same question for the observer in ContentPolicy.js

::: webapprt/WebappRT.jsm
@@ +21,5 @@
> +  return DOMApplicationRegistry;
> +});
> +
> +// If -content-url was given on the command line, observe webapps-ask-install so
> +// the content can install apps.

I think there's a bug open about allowing Marketplace functionality from the webapp runtime. That will make mozApps.install do something from inside the runtime as well, instead of doing nothing as it currently does (just fires the notification)

Have you given any thoughts about that? Maybe we will have to use a custom notification to install instead of using mozApps.install. this could make it a bit easier to not have to do the ask/did-install dance too. (could be done as a follow-up)

The need for actually installing a webapp in a test (vs. just loading a url) is it to ultimately call DOMApplicationRegistry.confirmInstall to set up all registry and then be able to test things like mozApps.getSelf, right?

Comment 29

[Drew Willcoxon :adw]

(In reply to Felipe Gomes (:felipe) from comment #28)
> > In short, because WebappRT.config isn't ready before that point.
> 
> Is this needed due to how WebappRT.config is being frozen to not be changed
> later? I'm wondering if removing that restriction would remove the need for
> this.  Or maybe it's needed because ContentPolicy had a whitelist of allowed
> domains, which is going away and would also make this unnecessary. 

It's needed because the WebappRT.config() getter would go looking for a file it won't find, which would end up throwing an exception that would ruin the runtime's startup.  There's no webapp.json in the -content-url case.

Maybe it would help to explain that CommandLineHandler.handle() isn't called until after a bunch of stuff has already happened, like WebappRT.config being accessed.  I wish that handle() were called the first thing on startup because it would make the patch simpler.  Instead of command-line flags, I thought about using environment variables or some other form of communication that would let me test for a content URL immediately on startup, but I don't like the fact that environment variables would apply to any webapprt executable you run in that environment.  I want the test-specific communication to be targeted to a specific execution of a specific executable.

> ::: webapprt/DirectoryProvider.js
> @@ +23,1 @@
> >  
> 
> Could the same observer in WebappRT.jsm set a property for this so that you
> can just check WebappRT.inContentURLMode below and not have to have this
> observer here?

I think so, but I think the observer approach is a better design.  It's flexible, scalable, and not too narrowly fit to the problems I'm trying to solve in this bug.  With the command-line notification, anybody in the whole runtime who wants to know what command line flags were passed can find out, and maybe we'll add more flags and observers in the future.

> Also same question for the observer in ContentPolicy.js

If ContentPolicy is going away, that's great for me since I only had to modify it so that URLs with the same origin as -content-url would be allowed to load.

> I think there's a bug open about allowing Marketplace functionality from the
> webapp runtime. That will make mozApps.install do something from inside the
> runtime as well, instead of doing nothing as it currently does (just fires
> the notification)

Oh, hmm.  Well, the did-install observers only kick in if -content-url was passed on the command line, so I don't think it would be a problem.

> The need for actually installing a webapp in a test (vs. just loading a url)
> is it to ultimately call DOMApplicationRegistry.confirmInstall to set up all
> registry and then be able to test things like mozApps.getSelf, right?

Right.

Comment 30

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 628506 [details] [diff] [review]
chrome and content mochitests

(In reply to Drew Willcoxon :adw from comment #29)
> Maybe it would help to explain that CommandLineHandler.handle() isn't called
> until after a bunch of stuff has already happened, like WebappRT.config
> being accessed.  I wish that handle() were called the first thing on startup
> because it would make the patch simpler.  Instead of command-line flags, I
> thought about using environment variables or some other form of
> communication that would let me test for a content URL immediately on
> startup, but I don't like the fact that environment variables would apply to
> any webapprt executable you run in that environment.  I want the
> test-specific communication to be targeted to a specific execution of a
> specific executable.

Ok yeah, it's unfortunate that handle() is called so late, but I also prefer that vs. env variables

> > I think there's a bug open about allowing Marketplace functionality from the
> > webapp runtime. That will make mozApps.install do something from inside the
> > runtime as well, instead of doing nothing as it currently does (just fires
> > the notification)
> 
> Oh, hmm.  Well, the did-install observers only kick in if -content-url was
> passed on the command line, so I don't think it would be a problem.

Hmm but after there's app install support from the runtime the did-install would also kick in by itself.. But in that case we could make it not do anything in -content-url mode. In that regard, do you think we should keep the generic name of "-content-url" or should we name it specifically to "-test-url"?



r=felipe to all the js code in webapprt/, with checking if the changes to ContentPolicy are still needed


Also I think that at some point it will be good to have start-up code in its own function instead of stuffing things into CommandLineHandler.handle().. Maybe now it's a good chance to change it?  (moving the Webapps.jsm import and openWindow out of it)

Comment 31

[:Gavin Sharp [email: [e-mail]]]

(In reply to Felipe Gomes (:felipe) from comment #30)
> Also I think that at some point it will be good to have start-up code in its
> own function instead of stuffing things into CommandLineHandler.handle()..
> Maybe now it's a good chance to change it?  (moving the Webapps.jsm import
> and openWindow out of it)

Yeah, this is a good idea. I think we'll need something like this for bug 759502, as well.

Comment 32

[:Gavin Sharp [email: [e-mail]]]

(though to be clear, I don't think it needs to block this bug from landing!)

Comment 33

[cmtalbert]

Comment on attachment 628506 [details] [diff] [review]
chrome and content mochitests

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

I focused on the test framework pieces, but I glanced over the webapp specific files. So, I tried running this on windows. It didn't work. :( 

Both make targets failed with a dialog stating that webapp.ini was not found.

Running from python directly: python --webapp-content produced nothing. No tests no pages, nothing. Adding --test-path=webapprt and also webapprt/content/tests/webapprt_sample.html also didn't work.

Running the chrome test from python: python --webapp-chrome died with a javascript error: chrome://mochikit/content/browser-harness.xul line 200: testWin is null.

So, one thing to keep in mind is that buildbot is going to run using the python style not the make targets (the test slaves don't have a build tree). So both methods need to work.

The patches look good, but since I couldn't get it to run, I'm going to have to r-. I'm going to be away from the 8th to the 17th so if you need another review, I recommend Ted or Joel (jmaher).

I'll file the follow on bug for the buildbot changes to get this running in production.

::: testing/mochitest/browser-test.js
@@ +130,5 @@
>        if (win != window && !win.closed &&
>            win.document.documentElement.getAttribute("id") != "browserTestHarness") {
>          let type = win.document.documentElement.getAttribute("windowtype");
> +        if (!type) {
> +          type = "unknown window type";

I'm not sure this is important - it depends on what we have in place for parsing of hangs and error messages on TBPL, but this code is not equivalent to what it is replacing. In particular, if you are running browser-chrome tests then type in your code will be "navigator:browser" but originally it would have been "browser window". In order not to break any parsers that might be depending on that (if there are any) I think it'd be simple to just maintain that functionality. And if we want to change that in the future, let's do it in a follow on bug where we can evaluate the logparsers.

::: testing/mochitest/runtests.py
@@ +181,5 @@
> +
> +    self.add_option("--webapprt-chrome",
> +                    action = "store_true", dest = "webapprtChrome",
> +                    help = "run WebbappRT chrome tests")
> +    defaults["webapprtChrome"] = False

I'd like to see something in the verifyOptions method that ensures you get an error if you set both these options to true.

::: webapprt/test/chrome/Makefile.in
@@ +14,5 @@
> +_BROWSER_TEST_FILES = \
> +  head.js \
> +  install.html \
> +  browser_sample.js \
> +    sample.webapp \

Nit: use the same indentation here. These can just be spaces, not tabs

::: webapprt/test/chrome/browser_sample.js
@@ +8,5 @@
> +       appConfig.app.manifest.name,
> +       "Window title should be webapp name");
> +    finish();
> +  });
> +}

I applaud the use of a simple sample test. Thanks!

::: webapprt/test/chrome/head.js
@@ +41,5 @@
> +
> +  let content = document.getElementById("content");
> +  content.addEventListener("load", function onLoad(event) {
> +    if (!appConfig)
> +      return;

These early returns are dangerous. If you look back at your sample test. We are 'waitingForExplicitFinish()'. So, when we do this early return and the next one we do not call the callback, which means that 'finish()' is never called in your test, which means that the test will hang with no other indication of failure. 

I like your test format, so ideally, we need some way to throw an error and call finish() here. Or, we call the callback and the callback is required to treat a null argument as a failure. I'm fine with solving it either way as long as it's solved. Silent failures resulting in timeouts are the bane of mochitests.

::: webapprt/test/chrome/sample.html
@@ +16,5 @@
> +  self.onsuccess = function () {
> +    msg.textContent = "Webapp getSelf OK: " + self.result;
> +  };
> +  self.onerror = function () {
> +    msg.textContent = "Webapp getSelf failed: " + self.error.name;

Suggestion: Is anything in the mochitest sample_test able to detect this failure? That would be a cool thing to showcase in your sample test. I would think that the callback is running inside the loaded app so it should be able to do document.getElementById("msg") and get this text right? That might make your sample test a little more interesting.

Comment 34

[Drew Willcoxon :adw]

(In reply to Felipe Gomes (:felipe) from comment #30)
> Hmm but after there's app install support from the runtime the did-install
> would also kick in by itself.. But in that case we could make it not do
> anything in -content-url mode. In that regard, do you think we should keep
> the generic name of "-content-url" or should we name it specifically to
> "-test-url"?

Oh, I see what you mean.  If we end up having to insert exceptions all over the place for content URL mode, yeah, I think -test-url would be better.  But it would be better not to have to insert exceptions all over the place... hmm.

I guess there will end up being two separate installation paths, one for tests, one for normal installations?  The test path installs apps into the runtime's profile and newly installed apps take over the runtime, the normal path installs them into the original Firefox profile and the app in the runtime keeps on running?  How would the runtime even get at the app registry in the Firefox profile though?  It could read and write the files, but it wouldn't go through DOMApplicationRegistry.

Comment 35

[cmtalbert]

Filed bug 762744 for the changes necessary to buildbot infrastructure to turn these on as well as to downstream applications (tbpl, orangefactor, try chooser etc).

One question for the folks on this bug. You might want to check my list of trees to run these tests on over in 762744. In particular, should the fx-team branch also be running these tests by default?  You might mention it over there if so.

See y'all in a week.

Comment 36

[Drew Willcoxon :adw]

(In reply to Clint Talbert Not reading bugmail June 8 - 18 2012( :ctalbert ) from comment #33)
> I focused on the test framework pieces, but I glanced over the webapp
> specific files. So, I tried running this on windows. It didn't work. :( 

Yeah, only OS X is supported now.  I'll add Windows support if this patch passes all the other reviews.

I was planning on landing what I have here with the tests turned off -- i.e., so that the buildbot scripts don't yet run the new make targets.  But as part of this patch I should probably have the make targets fail gracefully on non-OS X so that the tests can be turned on even without Windows support in place.

> ::: webapprt/test/chrome/Makefile.in
> @@ +14,5 @@
> > +_BROWSER_TEST_FILES = \
> > +  head.js \
> > +  install.html \
> > +  browser_sample.js \
> > +    sample.webapp \
> 
> Nit: use the same indentation here. These can just be spaces, not tabs

This is just to show that sample.webapp is a companion to the main browser_sample.js script.  They're part of the same "package."

Comment 37

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 628506 [details] [diff] [review]
chrome and content mochitests

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

(In reply to Drew Willcoxon :adw from comment #27)
> Comment on attachment 628506 [details] [diff] [review]
> chrome and content mochitests
> 
> Myk, Felipe suggested that you look at the webapprt.mm changes here.  Also,
> I want to make sure that the framework implemented in this patch is what you
> had in mind when you asked me to work on this.  Let me know if you have any
> questions.

The webapprt.mm changes look correct to me.  And yes, this is what I had in mind!

As I see it, there are roughly five areas of functionality that need testing in desktop Firefox's webapps feature implementation stack:

1. navigator.mozApps API
2. native installers
3. stub executable launchers
4. xulapp shell
5. web platform from within runtime

We've landed (plain?) mochitests for #1, which seem to work well; and chrome/browser-chrome mochitests seem sufficient for testing #2.  So that leaves the last three, and this seems like a fine solution for #4 (webapprt-test-chrome) and #5 (webapprt-test-content).

I don't know what to do about #3, but I suspect we can extend the chrome/browser-chrome installer tests to also test launching installed apps.  So that isn't something that this infrastructure needs to take into account.

I'm a bit concerned about the test-specific codepaths in this approach, in particular the different location of the registry, and the way apps get installed and loaded from within a runtime process.  But I don't have better ideas at the moment.

Perhaps the test files in the source repository could include pre-constructed app bundles and datadirs, but that seems brittle and hard to maintain/add tests.  Alternately, perhaps the test harness could start Firefox and invoke its native installers to install the test apps before launching them and running tests, but that seems complex and fragile.

Thus the current approach seems best.

::: testing/mochitest/runtests.py
@@ +175,5 @@
>      defaults["browserChrome"] = False
>  
> +    self.add_option("--webapprt-content",
> +                    action = "store_true", dest = "webapprtContent",
> +                    help = "run WebbappRT content tests")

Nit: WebbappRT -> WebappRT

@@ +832,5 @@
>      if not os.path.isdir(os.path.join(self.SCRIPT_DIRECTORY, jarDir)):
>        self.automation.log.warning("TEST-UNEXPECTED-FAIL | invalid setup: missing mochikit extension")
>        return None
>  
>      # Support Firefox (browser), B2G (shell) and SeaMonkey (navigator).

Nit: add "Webapp Runtime (webapp)" to this list.

Comment 38

[Drew Willcoxon :adw]

Attachment: chrome and content mochitests

Clint, this unbitrots the previous patch and addresses your comment 33.  Again, all of this is only supported on OS X for now, and I've defined the make targets only for OS X.  I'll add Windows support in a follow-up bug.

Comment 39

[Jason Smith [:jsmith]]

(In reply to Drew Willcoxon :adw from comment #38)
> Created attachment 635134 [details] [diff] [review]
> chrome and content mochitests
> 
> Clint, this unbitrots the previous patch and addresses your comment 33. 
> Again, all of this is only supported on OS X for now, and I've defined the
> make targets only for OS X.  I'll add Windows support in a follow-up bug.

Curious, how much work will it take to add Windows support to this? On Marco's side also (he's the guy working on the linux side of things), how much work will it take to add Linux support to this?

Comment 40

[Drew Willcoxon :adw]

Good question.  The main part specific to OS X was to patch webapprt/mac/webapprt.mm to accept the -profile command-line argument and to look for the Firefox binary in the same bundle as the stub executable.  Stub executables for other platforms will need to be updated similarly.  (The other part specific to OS X was patching some chrome modification code in webapprt/content/webapp.js, but that's not needed on other platforms.)

Comment 41

[cmtalbert]

Comment on attachment 635134 [details] [diff] [review]
chrome and content mochitests

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

So these changes look good. Everything is being packaged properly for buildbot, assuming that the webapprt-stub binaries will be packaged with the browser binary, which I assume they will.

However, in running it, there is a notification panel that displays during the installation of the webapp (from both content and chrome) which halts the test. And I don't see any code to deal with that dialog panel.  The panel is being launched from here:
http://mxr.mozilla.org/mozilla-central/source/browser/modules/webappsUI.jsm#136

This stops the test. If I manually click "install" then the test completes successfully. If I click "deny install" then the test hangs indefinitely.
So, there are two further items that need to be added:
1. You need to handle this dialog in some way so that the installation is allowed
2. If the installation fails, you need to be certain that the test does not hang.

r+ with those two changes, happy to re-review those specific changes if required, but I think it will be pretty minimal. You can get around the pop up warning with a callback, and you just need to be certain that there is a mochitest check (an ok(false, 'msg'); ) capturing the case that installation fails and then calls simpletest.finish().

Comment 42

[cmtalbert]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #37)
> Comment on attachment 628506 [details] [diff] [review]
> chrome and content mochitests
> 
> Review of attachment 628506 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to Drew Willcoxon :adw from comment #27)
> > Comment on attachment 628506 [details] [diff] [review]
> > chrome and content mochitests
> > 
> > Myk, Felipe suggested that you look at the webapprt.mm changes here.  Also,
> > I want to make sure that the framework implemented in this patch is what you
> > had in mind when you asked me to work on this.  Let me know if you have any
> > questions.
> 
> The webapprt.mm changes look correct to me.  And yes, this is what I had in
> mind!
> 
> As I see it, there are roughly five areas of functionality that need testing
> in desktop Firefox's webapps feature implementation stack:
> 
> 1. navigator.mozApps API
> 2. native installers
> 3. stub executable launchers
> 4. xulapp shell
> 5. web platform from within runtime
> 
I agree with Myk. Also, the Automation team is working with Jason and other QA stakeholders to build a Marionette based tool to aid with automating this process and performing OS level checks to verify (among other things) that the stub exectuable launchers are working properly, that apps are showing up in all the right places on the user's computer etc.

Marionette is a new automation framework we developed for B2G that allows us to automate any Gecko executable, and drive it by its exposed UI or from chrome-space. So, this should solve point number 3 without doing more unnatural things to Mochitest.

It's good to have both sets of tests to unblock the apps team now while the Marionette apps framework is developed because the Mochitests already have buildbot support, and Marionette doesn't yet (though that is landing for B2G). 

The Marionette framework will be more versatile and will have more hooks to verify OS level changes (this is one of the requirements QA asked for) than Mochitest does.

Comment 43

[Drew Willcoxon :adw]

Thanks, Clint.

(In reply to Clint Talbert ( :ctalbert ) from comment #41)
> However, in running it, there is a notification panel that displays during
> the installation of the webapp (from both content and chrome) which halts
> the test. And I don't see any code to deal with that dialog panel.  The
> panel is being launched from here:
> http://mxr.mozilla.org/mozilla-central/source/browser/modules/webappsUI.
> jsm#136

I think this is bug 738832, which landed after I posted the newest patch.  (The panel is opened from webapprt/WebappsHandler.jsm, though.)  I'll have to update the patch again.

Comment 44

[Drew Willcoxon :adw]

Attachment: chrome and content mochitests

Myk, would you mind looking at this one last time?  It makes some changes to the startup path in CommandLineHandler.js in light of bug 738832.  In particular it suppresses WebappsHandler while in test mode.  I want to make sure I'm not messing anything up or missing an obviously better way of doing things.

It also renames the -test-url command-line flag (which was previously named -content-url) to -test-mode.  Since ContentPolicy was removed, the flag no longer needs a URL argument in the case of chrome tests.  (But it still does for content tests.)

I'll push this to tryserver to make sure it doesn't have any unexpected side effects.

Comment 45

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 637249 [details] [diff] [review]
chrome and content mochitests

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

> Myk, would you mind looking at this one last time?  It makes some changes to
> the startup path in CommandLineHandler.js in light of bug 738832.  In
> particular it suppresses WebappsHandler while in test mode.  I want to make
> sure I'm not messing anything up or missing an obviously better way of doing
> things.

The degree of complexity of the startup code (not only as a result of this patch) concerns me, but I don't know of a better way of doing things.  We will want to test WebappsHandler too at some point, but this unblocks most of the test writing we want to do, so let's land it and then iterate as needed.

r=myk with the additional check in the command line handler.

::: webapprt/CommandLineHandler.js
@@ +25,5 @@
> +    if (testModeIdx >= 0) {
> +      let testURL = null;
> +      if (testModeIdx + 1 < cmdLine.length) {
> +        testURL = cmdLine.getArgument(testModeIdx + 1);
> +        cmdLine.removeArguments(testModeIdx + 1, testModeIdx + 1);

Before this code removes and validates the argument after the -test-mode flag, it needs to make sure that next argument isn't itself a flag, since otherwise the code will throw incorrectly when -test-mode is followed by another flag.

(It's too bad nsICommandLine doesn't have a handleFlagWithOptionalParam method.)

Comment 46

[Drew Willcoxon :adw]

Yet another problem: each chrome test "leaks" two DOMWindows:

> [chrome://mochitests/content/webapprtChrome/webapprt/test/chrome/browser_sample.js]
>   1 window(s) [url = http://mochi.test:8888/webapprtChrome/webapprt/test/chrome/install.html?manifestPath=%22sample.webapp%22]
>   1 window(s) [url = http://mochi.test:8888/webapprtChrome/webapprt/test/chrome/sample.html]

I saw this before but ignored it since I didn't understand it and it doesn't prevent the test from passing.  In preparation for landing I noticed it again and thought I should check it out.

If there are > 7 leaks, the framework's automatic leak test (build/automationutils.py) fails, and I think that each chrome test you run will produce similar "leaks" (one for install.html and one for the test webapp), so as soon as there are 4 or more chrome tests, the leak test will fail.

They aren't meaningful leaks.  Each test just navigates to two URLs in the runtime's content browser and then ends.  The DOMWindows for the URLs don't go away by the time each test ends, so the harness sees them as leaks.  Of course the DOMWindows do go away when the suite ends and the chrome window closes.

I don't know what to do about it.  I wonder what normal browser chrome tests do when they open pages.  Do they always open them in new tabs and then close them when done?

Comment 47

[(not currently active) Ted Mielczarek]

CCing some folks who have done leak-checking work on the browser tests.

Comment 48

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Drew Willcoxon :adw from comment #46)
> I don't know what to do about it.  I wonder what normal browser chrome tests
> do when they open pages.  Do they always open them in new tabs and then
> close them when done?

AFAIK, yes. Tests are supposed to clean up after themselves and reset the browser state as far as possible. Do the leaks go away if you replace |content.loadURI(installURL)| with

> gBrowser.addTab(installURL);

in head.js? You can then attach the load listener to newTab.linkedBrowser. Don't forget to remove the tab at the end of the test.

Comment 49

[:Gavin Sharp [email: [e-mail]]]

These tests are run in the web app runtime, which doesn't have tabs. For the moment, we should probably just find a way to disable the leak checking for these tests specifically, or work around it some other way.

Comment 50

[Drew Willcoxon :adw]

Turning off the leak detector for chrome tests is as easy as changing one line in runtests.py:

>     # it's a debug build, we can parse leaked DOMWindows and docShells
> #     if Automation.IS_DEBUG_BUILD:
>     if Automation.IS_DEBUG_BUILD and not options.webapprtChrome:
>       logger = ShutdownLeakLogger(self.automation.log)
>     else:
>       logger = None

Do people think that's kosher?

I also tried going back in the browser's history to the page that was open before the test started.  It's flaky and doesn't always work.

Another possibility is to open a new runtime chrome window for each test and close it when done.  Obviously it's more complex than reusing the same window, but in theory it should work.  I'm experimenting with that now.

Comment 51

[:Gavin Sharp [email: [e-mail]]]

(In reply to Drew Willcoxon :adw from comment #50)
> Turning off the leak detector for chrome tests is as easy as changing one
> line in runtests.py:
> 
> >     # it's a debug build, we can parse leaked DOMWindows and docShells
> > #     if Automation.IS_DEBUG_BUILD:
> >     if Automation.IS_DEBUG_BUILD and not options.webapprtChrome:
> >       logger = ShutdownLeakLogger(self.automation.log)
> >     else:
> >       logger = None
> 
> Do people think that's kosher?

I think that's fine for now. We can revisit that in a followup and investigate other options for making it work.

Comment 52

[Drew Willcoxon :adw]

(In reply to Drew Willcoxon :adw from comment #50)
> Another possibility is to open a new runtime chrome window for each test and
> close it when done.  Obviously it's more complex than reusing the same
> window, but in theory it should work.  I'm experimenting with that now.

This doesn't seem to work.  The "--DOMWINDOW" log line for the new runtime chrome window isn't printed when the window closes at the end of the test or even after waiting 10 seconds after the end of the test.  It's printed waaaaay after when the suite ends.  So it leaks.

Comment 53

[:Felipe Gomes (needinfo for replies!)]

Maybe you've already tried this, but: is it possible to try to run GC a few times when the last test finishes? We run 1 schedulePreciseGC and 3 garbageCollect() in browser-chrome tests before counting the DOMWINDOWs left:

http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/browser-test.js#264

Comment 54

[Drew Willcoxon :adw]

Attachment: landed patch

(In reply to Felipe Gomes (:felipe) from comment #53)
> Maybe you've already tried this, but: is it possible to try to run GC a few
> times when the last test finishes?

I didn't, thanks for the suggestion.  It didn't help unfortunately.  Something must be keeping the window alive.  Probably my own leak-riddled code now that I think about it.  But I already landed with the leak detector turned off for chrome WebappRT tests.

https://tbpl.mozilla.org/?tree=Try&rev=d3f4e808bcea
https://hg.mozilla.org/integration/mozilla-inbound/rev/df17f2f4ab63

Comment 55

[Jason Smith [:jsmith]]

Drew - Could you summarize what followup items need to be taken care of from this patch and file bugs associated with each one? I know there's followup items to add support for windows and linux, the leak issue, but what else?

Comment 56

[Drew Willcoxon :adw]

This is why I wasn't seeing --DOMWINDOW for the new chrome window until the end of the suite.  From the changes I made to webapprt/content/webapp.js:

>   if (cmdLineArgs && cmdLineArgs.hasKey("test-mode")) {
>     Services.obs.addObserver(function observe(subj, topic, data) {
>       // The observer is present for the lifetime of the runtime.
>       initWindow(false);
>     }, "webapprt-test-did-install", false);

Each runtime chrome window adds this observer but never removes it, which is by design when all the tests use the same window.  If the window instead assumes that it'll only be used for one test and removes the observer the first time it's triggered, then the window isn't leaked.

There's one thing I still don't understand though.  Even when the observer removes itself, the --DOMWINDOW for the new chrome window still happens after

> INFO TEST-END | chrome://mochitests/content/webapprtChrome/webapprt/test/chrome/browser_sample.js | finished in 930ms

even though I close the window and even trigger GC before I call finish().  The reason the framework doesn't consider the window leaked is because the --DOMWINDOW comes before

> INFO TEST-START | Shutdown

I don't know if there's a race between "Shutdown" and the --DOMWINDOW and I'm getting lucky or what.

Comment 57

[Drew Willcoxon :adw]

(In reply to Drew Willcoxon :adw from comment #56)
> If the window instead assumes that it'll only be used for one test and removes
> the observer the first time it's triggered, then the window isn't leaked.

Actually the window should just remove it on unload or close or whatever, which would be the right thing to do in any case.

Comment 58

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/df17f2f4ab63

Comment 59

[Dão Gottwald [:dao]]

(In reply to Tim Taubert [:ttaubert] from comment #48)
> (In reply to Drew Willcoxon :adw from comment #46)
> > I don't know what to do about it.  I wonder what normal browser chrome tests
> > do when they open pages.  Do they always open them in new tabs and then
> > close them when done?
> 
> AFAIK, yes.

No, many tests just load stuff in the current tab, without leaking all those inner DOM windows until shutdown.

Comment 60

[Jason Smith [:jsmith]]

Per comment 55 - Could someone please file followup bugs for anything that needs to be taken care of as a result of this patch that is known? Such as windows support?

Comment 61

[Drew Willcoxon :adw]

(In reply to Dão Gottwald [:dao] from comment #59)
> No, many tests just load stuff in the current tab, without leaking all those
> inner DOM windows until shutdown.

Because browser-test.js removes the current tab after all tests are done but before Shutdown is logged:

http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/browser-test.js#262

So the simplest thing to do for WebappRT tests, other than disabling the leak logger, is to close the window(s) that the tests run in before Shutdown.  I'll move this to a new bug.

Comment 62

[Dão Gottwald [:dao]]

(In reply to Drew Willcoxon :adw from comment #61)
> (In reply to Dão Gottwald [:dao] from comment #59)
> > No, many tests just load stuff in the current tab, without leaking all those
> > inner DOM windows until shutdown.
> 
> Because browser-test.js removes the current tab after all tests are done but
> before Shutdown is logged:
> 
> http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/browser-test.
> js#262

It does this for tests replacing the original tab with a new one, not for tests loading pages in the original tab.

> So the simplest thing to do for WebappRT tests, other than disabling the
> leak logger, is to close the window(s) that the tests run in before
> Shutdown.  I'll move this to a new bug.

That sounds like it would hide actual leaks when the leaking code lives in that window. What I'd try is to just load about:blank in the content area when all tests are done.

Comment 63

[Drew Willcoxon :adw]

(In reply to Dão Gottwald [:dao] from comment #62)
> It does this for tests replacing the original tab with a new one, not for
> tests loading pages in the original tab.

Are you sure?  I tested with two toy tests, both of which load pages in the one and only tab, and when both are done the tab is closed by the browser-test.js line I linked to.  There's no logic in nextTest() that checks whether the current tab replaced the original.  It just closes the current tab.

> That sounds like it would hide actual leaks when the leaking code lives in
> that window.

Good point.  If each test opened a new window though, leaks would still be caught on the original window when the suite ends.

> What I'd try is to just load about:blank in the content area when all tests
> are done.

That was one of the first things I tried, and it didn't work -- I presume because the DOMWindows are still in bfcache.  In fact when I add an unload listener to one of the pages, it's no longer leaked.

I'm experimenting again with loading about:blank on startup and then when tests are done, going all the way back in history to about:blank and then calling sessionHistory.PurgeHistory().  It seems to work.

Comment 64

[Drew Willcoxon :adw]

(In reply to Drew Willcoxon :adw from comment #63)
> > What I'd try is to just load about:blank in the content area when all tests
> > are done.
> 
> That was one of the first things I tried, and it didn't work -- I presume
> because the DOMWindows are still in bfcache.  In fact when I add an unload
> listener to one of the pages, it's no longer leaked.

Left out one part: loading a new about:blank when the tests are done "leaks" its DOMWindow just like it does for whatever other new page is open when the tests finish, which is why I mention opening it before the tests start.

Comment 65

[Dão Gottwald [:dao]]

(In reply to Drew Willcoxon :adw from comment #63)
> (In reply to Dão Gottwald [:dao] from comment #62)
> > It does this for tests replacing the original tab with a new one, not for
> > tests loading pages in the original tab.
> 
> Are you sure?  I tested with two toy tests, both of which load pages in the
> one and only tab, and when both are done the tab is closed by the
> browser-test.js line I linked to.  There's no logic in nextTest() that
> checks whether the current tab replaced the original.  It just closes the
> current tab.

It does close the current tab, but the motivation is the one I mentioned. See bug 734172.