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

RESOLVED FIXED in Firefox 26

Status

RESOLVED FIXED
5 years ago
3 months ago

People

(Reporter: ochameau, Assigned: ochameau)

Tracking

unspecified
Firefox 26
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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!
(Assignee)

Comment 1

5 years ago
Created attachment 802281 [details] [diff] [review]
Convert webapps xpcshell test to mochitest-plain and cover app reinstall and redirects manifest property
(Assignee)

Updated

5 years ago
Depends on: 914633, 894990
(Assignee)

Comment 2

5 years ago
Created attachment 803013 [details] [diff] [review]
Updated after modification during review of bug 912475 and 894990
Attachment #802281 - Attachment is obsolete: true
(Assignee)

Comment 3

5 years ago
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!
Attachment #803013 - Flags: review?(fabrice)
(Assignee)

Updated

5 years ago
Blocks: 912475
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.
Attachment #803013 - Flags: review?(fabrice) → review+
(Assignee)

Comment 6

5 years ago
Created attachment 805045 [details] [diff] [review]
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.
Attachment #803013 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Depends on: 916604
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/c63bb927cf78
Status: NEW → ASSIGNED
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/c63bb927cf78
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 26

Updated

5 years ago
Depends on: 918528
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.
Summary: Add test for app resinstall and redirects when installing an app via the app actor → Add test for app reinstall and redirects when installing an app via the app actor
This needed build peer review...
Flags: needinfo?(poirot.alex)
(Assignee)

Comment 12

5 years ago
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?
Attachment #805045 - Flags: review?(mh+mozilla)
(Assignee)

Updated

5 years ago
Flags: needinfo?(poirot.alex)
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.
Attachment #805045 - Flags: review?(mh+mozilla)

Updated

3 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.