914604 - Add test for app reinstall and redirects when installing an app via the app actor

Status: RESOLVED FIXED

(DevTools Graveyard :: WebIDE, defect)

Description

[Alexandre Poirot [:ochameau]]

So far, testing app installation was challenging as xpcshell doesn't allow to create a mozbrowser frame... But thanks to some tweaks made to mochitest, like bug 894990, we should be able to write a mochitest against the webapps actor and test all its features!

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Convert webapps xpcshell test to mochitest-plain and cover app reinstall and redirects manifest property

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: Updated after modification during review of bug 912475 and 894990

Comment 3

[Alexandre Poirot [:ochameau]]

Comment on attachment 803013 [details] [diff] [review]
Updated after modification during review of bug 912475 and 894990

I haven't got a review for bug 914633, but receive positive feedback, so hopefully this patch won't have any other modification due to its dependencies.

I would like to land this patch in FF26 in order to finally have good unit tests for the app actor!

Comment 4

[Alexandre Poirot [:ochameau]]

https://tbpl.mozilla.org/?tree=Try&rev=d24568da7aa3

Comment 5

[[:fabrice] Fabrice Desré]

Comment on attachment 803013 [details] [diff] [review]
Updated after modification during review of bug 912475 and 894990

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

::: toolkit/devtools/apps/tests/debugger-protocol-helper.js
@@ +67,5 @@
> +// Install a test packaged webapp from data folder
> +addMessageListener("install", function (aMessage) {
> +  let url = aMessage.url;
> +  let appId = aMessage.appId;
> + 

nit: whitespace

::: toolkit/devtools/apps/tests/redirect.sjs
@@ +1,1 @@
> +var gBasePath = "tests/dom/apps/tests/";

Unused.

::: toolkit/devtools/apps/tests/test_webapps_actor.html
@@ +41,5 @@
> +    ok(false, "Caught exception", ex);
> +  }
> +}
> +
> +function go() {

I would call that start(), but up to you.

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: Adressed comments and removed redirects test

I fixed review comments, but also had to remove redirects test that triggers an assertion.
I'll open a bug to investigate that in a followup.

Comment 7

[Alexandre Poirot [:ochameau]]

https://tbpl.mozilla.org/?tree=Try&rev=78a611c17daf

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/integration/fx-team/rev/c63bb927cf78

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Comment 10

[Martijn Wargers (dead)]

You're doing great work adding tests and features to SpecialPowers.
But instead of using SpecialPowers.addPermission, you should use SpecialPowers.pushPermissions, which automatically removes the permissions when the test file is done (similar to pushPrefEnv).

+  function() {
+    ok(true, "all done!\n");
+    SpecialPowers.popPrefEnv(finish);

This shouldn't be necessary, as prefs would be automatically be undone after the test file has been run with pushPrefEnv. When you would test it standalone, there is bug 914890, when that bug is fixed, it would also be fixed for standalone tests.

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

This needed build peer review...

Comment 12

[Alexandre Poirot [:ochameau]]

Comment on attachment 805045 [details] [diff] [review]
Adressed comments and removed redirects test

Oops, this patch already landed but we forgot to flag a build peer during review.

Mike, could you do a buildwise review?

Comment 13

[Mike Hommey [:glandium]]

Comment on attachment 805045 [details] [diff] [review]
Adressed comments and removed redirects test

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

::: toolkit/devtools/apps/tests/Makefile.in
@@ +7,5 @@
> +srcdir          = @srcdir@
> +VPATH           = @srcdir@
> +relativesrcdir = @relativesrcdir@
> +
> +include $(DEPTH)/config/autoconf.mk

Please remove this boilerplate, and the rules.mk include.