Closed Bug 981249 Opened 6 years ago Closed 6 years ago

Add a test that installs an app and launches it

Categories

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

x86_64
Linux
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 31

People

(Reporter: marco, Assigned: marco)

References

Details

Attachments

(1 file, 3 obsolete files)

I think this would be a really useful test, because it would expose errors with the installation (for example if we set wrong permissions to one of the files) that the installation tests may not expose.

We should probably write such a test as the tests in bug 898647.

I'm making bug 898647 block this because in bug 898647 we're introducing the first tests for the WebappsInstaller module.
Attached patch Patch (obsolete) — Splinter Review
The hosted test is simpler, the packaged one is more complicated.

There's a server component (app.sjs) that handles requests from an app (?appreq) and from the test code (?testreq).
The app loads app.sjs?appreq, the test code verifies (by loading app.sjs?testreq) that a appreq had been handled.

In the hosted case, we just need to set the launch_path to app.sjs?appreq.

In the packaged case, we need the app in the webapps registry too, otherwise AppsUtils.jsm doesn't know where the package is. The app.sjs?appreq is loaded through an iframe.

The points I'd like to improve are:
- The mochitest http server url is being hardcoded (127.0.0.1:8888), I'd like a better way to know what the url is.
- With bug 906223, we'd be able to avoid having the packaged app in the webapps registry (but I guess this could be done in a followup bug).
Assignee: nobody → mar.castelluccio
Status: NEW → ASSIGNED
Attachment #8392128 - Flags: feedback?(myk)
I've just noticed that the 127.0.0.1:8888 url is already hardcoded in a lot of our tests, so I guess we can avoid the first point too.
Priority: -- → P1
Comment on attachment 8392128 [details] [diff] [review]
Patch

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

(In reply to Marco Castelluccio [:marco] from comment #1)

> There's a server component (app.sjs) that handles requests from an app
> (?appreq) and from the test code (?testreq).
> The app loads app.sjs?appreq, the test code verifies (by loading
> app.sjs?testreq) that a appreq had been handled.

This seems reasonable, or at least I can't think of a better way to communicate the successful launch back to the test script.


> In the hosted case, we just need to set the launch_path to app.sjs?appreq.
> 
> In the packaged case, we need the app in the webapps registry too, otherwise
> AppsUtils.jsm doesn't know where the package is. The app.sjs?appreq is
> loaded through an iframe.

Wouldn't we want the hosted app to be in the webapp registry too?  This seems like an integration test of the whole system, and part of the process of installing and running an app is to record it in the registry.


> The points I'd like to improve are:
> - The mochitest http server url is being hardcoded (127.0.0.1:8888), I'd
> like a better way to know what the url is.

Let's use whatever the best practice is, which sounds like a hardcoded URL.


> - With bug 906223, we'd be able to avoid having the packaged app in the
> webapps registry (but I guess this could be done in a followup bug).

Indeed!

::: dom/apps/src/AppsUtils.jsm
@@ +30,1 @@
>  }

Over in Webapps.jsm we do this by default ifdef DEBUG, and we might want to do the same here:

http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.jsm?rev=e9bff15a8d19#57

::: toolkit/webapps/tests/app.sjs
@@ +26,5 @@
> +  if ("testreq" in query) {
> +    response.setHeader("Content-Type", "text/plain", false);
> +
> +    let appreq = getState("appreq");
> +    if (!appreq) {

This works because |"true" == true|, but since |"false" == true| too, this should check that |appreq == "true"| or even set appreq to a string value that doesn't resemble a boolean (so it won't be mistaken for one, as in this case).

@@ +27,5 @@
> +    response.setHeader("Content-Type", "text/plain", false);
> +
> +    let appreq = getState("appreq");
> +    if (!appreq) {
> +      appreq = "false";

Can you initialize appreq to this state at script evaluation?

::: toolkit/webapps/tests/chrome.ini
@@ +1,5 @@
>  [DEFAULT]
>  support-files =
>    head.js
> +  app.sjs
> +  data/app.zip

We need a way to recreate this ZIP file if the tests change.  At the very least, that means committing the source files and instructions for using them to recreate the ZIP file.  (Ideally we would generate the ZIP file at build/test time, so we never have to regenerate it when the source files change.)

@@ +10,5 @@
>  skip-if = os == "mac"
> +[test_hosted_launch.xul]
> +skip-if = os == "mac"
> +[test_packaged_launch.xul]
> +skip-if = os == "mac"

What are the issues on Mac?

::: toolkit/webapps/tests/test_hosted_launch.xul
@@ -258,5 @@
>  
> -  // Reinstall application
> -  info("Test reinstallation");
> -  yield nativeApp.install(manifest);
> -  while (!WebappOSUtils.isLaunchable(app)) {

Since you've removed this conditional, I'd add a test to ensure WebappOSUtils.isLaunchable() still returns true for the app.

@@ +212,5 @@
> +    appClosed = true;
> +  }
> +
> +  appProcess.init(exeFile)
> +  appProcess.runAsync([], 0, observer);

Nit: observer could be a simple lambda:

  appProcess.runAsync([], 0, () => appClosed = true);

::: webapprt/Startup.jsm
@@ +146,5 @@
>  
>      WebappRT.startUpdateService();
> +  }).then(null, function (aError) {
> +    dump("Error: " + aError + "\n");
> +    Services.startup.quit(Ci.nsIAppStartup.eAttemptQuit);

Instead of intercepting the error here, we should let CommandLineHandler handle it with a promise rejection handler.  That way we don't tease callers by returning a promise that will never actually call its reject handler because we intercept errors here.

::: webapprt/WebappRT.jsm
@@ +29,5 @@
>                                    "nsIAppsService");
>  
>  this.WebappRT = {
>    get launchURI() {
> +    return this.localeManifest.fullLaunchPath();

Nice simplification!
Attachment #8392128 - Flags: feedback?(myk) → feedback+
Attached patch Patch (obsolete) — Splinter Review
(In reply to Myk Melez [:myk] [@mykmelez] from comment #3)
> ::: dom/apps/src/AppsUtils.jsm
> @@ +30,1 @@
> >  }
> 
> Over in Webapps.jsm we do this by default ifdef DEBUG, and we might want to
> do the same here:
> 
> http://mxr.mozilla.org/mozilla-central/source/dom/apps/src/Webapps.
> jsm?rev=e9bff15a8d19#57

I've removed that debugging code, we may consider adding it in another bug.

> @@ +27,5 @@
> > +    response.setHeader("Content-Type", "text/plain", false);
> > +
> > +    let appreq = getState("appreq");
> > +    if (!appreq) {
> > +      appreq = "false";
> 
> Can you initialize appreq to this state at script evaluation?

I've switched to "done", so this is a bit different now. Anyway I couldn't initialize "appreq" at script evaluation, I don't know why.

> ::: toolkit/webapps/tests/chrome.ini
> @@ +1,5 @@
> >  [DEFAULT]
> >  support-files =
> >    head.js
> > +  app.sjs
> > +  data/app.zip
> 
> We need a way to recreate this ZIP file if the tests change.  At the very
> least, that means committing the source files and instructions for using
> them to recreate the ZIP file.  (Ideally we would generate the ZIP file at
> build/test time, so we never have to regenerate it when the source files
> change.)

I'm now generating the ZIP file using nsIZipWriter at the beginning of the test.

> 
> @@ +10,5 @@
> >  skip-if = os == "mac"
> > +[test_hosted_launch.xul]
> > +skip-if = os == "mac"
> > +[test_packaged_launch.xul]
> > +skip-if = os == "mac"
> 
> What are the issues on Mac?

Like the other toolkit/webapps/ tests, we can't install apps on our Mac try machines because we lack the necessary privileges. I wish we could find a workaround because these tests are really essential.

> ::: toolkit/webapps/tests/test_hosted_launch.xul
> @@ -258,5 @@
> >  
> > -  // Reinstall application
> > -  info("Test reinstallation");
> > -  yield nativeApp.install(manifest);
> > -  while (!WebappOSUtils.isLaunchable(app)) {
> 
> Since you've removed this conditional, I'd add a test to ensure
> WebappOSUtils.isLaunchable() still returns true for the app.

There's still a call to |isLaunchable|. Probably I shouldn't use |hg cp| because it sometimes makes code more difficult to review!
I've copied the install/reinstall/update tests and removed the reinstall/update parts.
Attachment #8395983 - Flags: review?(myk)
Attachment #8392128 - Attachment is obsolete: true
Comment on attachment 8395983 [details] [diff] [review]
Patch

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

(In reply to Marco Castelluccio [:marco] from comment #4)

> > We need a way to recreate this ZIP file if the tests change.  At the very
> > least, that means committing the source files and instructions for using
> > them to recreate the ZIP file.  (Ideally we would generate the ZIP file at
> > build/test time, so we never have to regenerate it when the source files
> > change.)
> 
> I'm now generating the ZIP file using nsIZipWriter at the beginning of the
> test.

Awesome, that's ideal!


> > What are the issues on Mac?
> 
> Like the other toolkit/webapps/ tests, we can't install apps on our Mac try
> machines because we lack the necessary privileges. I wish we could find a
> workaround because these tests are really essential.

Ah, right.  Perhaps bug 791810 is that workaround, at least to some extent (it'd still be useful to test admin-privileged install, but fixing that bug would at least enable us to test install).  In the meantime, I've confirmed that the tests pass on Mac.


> There's still a call to |isLaunchable|. Probably I shouldn't use |hg cp|
> because it sometimes makes code more difficult to review!
> I've copied the install/reinstall/update tests and removed the
> reinstall/update parts.

Ah, no worries.  Now that I know what you did, it's actually easier to review, because I have more context from the code you're copying!
Attachment #8395983 - Flags: review?(myk) → review+
(In reply to Myk Melez [:myk] [@mykmelez] from comment #5) 
> > > What are the issues on Mac?
> > 
> > Like the other toolkit/webapps/ tests, we can't install apps on our Mac try
> > machines because we lack the necessary privileges. I wish we could find a
> > workaround because these tests are really essential.
> 
> Ah, right.  Perhaps bug 791810 is that workaround, at least to some extent
> (it'd still be useful to test admin-privileged install, but fixing that bug
> would at least enable us to test install).  In the meantime, I've confirmed
> that the tests pass on Mac.

We could use the workaround just for tests, but we'd probably need to add some test-specific code. It isn't good to add test-specific code, but it's better than nothing.
Keywords: checkin-needed
The patch is just adding a new javascript test, without any new C code. So I guess the bug was already there, the new test just uncovered it.
The failure is in the webapprt executable:

> 02:46:02     INFO -      #134 0x4598fc in _start (/home/cltbld/.test_desktop_hosted_launch-412e200b589392a505d82e3f80c66c1d/webapprt-stub+0x4598fc)
Attached patch Patch (obsolete) — Splinter Review
Given that the failure predates this patch, I think we can just skip the test on ASAN builds.
I'll file another bug to fix the ASAN failure.
Attachment #8395983 - Attachment is obsolete: true
Attached patch PatchSplinter Review
Fixed commit message.
Attachment #8398073 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/73e20031bb0f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Depends on: 989569
(In reply to Myk Melez [:myk] [@mykmelez] from comment #5)
> Ah, right.  Perhaps bug 791810 is that workaround, at least to some extent
> (it'd still be useful to test admin-privileged install, but fixing that bug
> would at least enable us to test install).  In the meantime, I've confirmed
> that the tests pass on Mac.

I've tried to install apps in a directory that doesn't require admin privileges. The tests work correctly on 10.8, but fail in 10.6 because isLaunchable always returns false. If I remove the |isLaunchable| check, the test passes on 10.6 too.
So probably on 10.6 apps installed in other directories aren't detected.

Ultimately, I think we can use this workaround and disable the tests only on 10.6, at least temporarily.
Flags: needinfo?(myk)
(In reply to Marco Castelluccio [:marco] from comment #11)

> Given that the failure predates this patch, I think we can just skip the
> test on ASAN builds.
> I'll file another bug to fix the ASAN failure.

Can you file that followup bug and reference it here?


(In reply to Marco Castelluccio [:marco] from comment #15)

> Ultimately, I think we can use this workaround and disable the tests only on
> 10.6, at least temporarily.

Indeed!  Can you file a followup bug for this as well?
Flags: needinfo?(myk) → needinfo?(mar.castelluccio)
(In reply to Myk Melez [:myk] [@mykmelez] from comment #16)
> (In reply to Marco Castelluccio [:marco] from comment #11)
> 
> > Given that the failure predates this patch, I think we can just skip the
> > test on ASAN builds.
> > I'll file another bug to fix the ASAN failure.
> 
> Can you file that followup bug and reference it here?

I already did, it's bug 989569.

> 
> (In reply to Marco Castelluccio [:marco] from comment #15)
> 
> > Ultimately, I think we can use this workaround and disable the tests only on
> > 10.6, at least temporarily.
> 
> Indeed!  Can you file a followup bug for this as well?

Sure! Filed bug 993690.
Flags: needinfo?(mar.castelluccio)
Flags: in-testsuite+
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.