Closed Bug 915884 Opened 7 years ago Closed 6 years ago

Add tests for Inter-App Communication API

Categories

(Core :: DOM: Device Interfaces, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla31
Tracking Status
firefox31 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: airpingu, Assigned: airpingu)

References

()

Details

Attachments

(5 files, 15 obsolete files)

193 bytes, text/html
squib
: review+
ferjm
: review+
Details
11.33 KB, image/png
Details
35.59 KB, patch
ferjm
: review+
airpingu
: checkin+
Details | Diff | Splinter Review
14.86 KB, patch
airpingu
: review+
airpingu
: checkin+
Details | Diff | Splinter Review
11.64 KB, patch
airpingu
: review+
airpingu
: checkin+
Details | Diff | Splinter Review
We need to add tests for Inter-App Communication API. This is definitely the first priority.
Flags: in-testsuite+
This needs fixing fast. And I though in-testsuite+ was set after we actually had tests. Correct?
Thanks for bringing this to my attention. I've been kept interrupted by other V1.3 features in the past month. Mark this as V1.3 to push myself to finish this...
blocking-b2g: --- → 1.3?
(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #2)
> Thanks for bringing this to my attention. I've been kept interrupted by
> other V1.3 features in the past month. Mark this as V1.3 to push myself to
> finish this...

Tests don't ever block the release. However, please definitely look into this at a high priority. We definitely need to have basic mochitests for every new WebAPI introduced.

Clearing in-testsuite+ as that's only set after a patch lands with tests.
blocking-b2g: 1.3? → ---
Flags: in-testsuite+
Will work out marionette test to test API level as well.
Attachment #8345823 - Attachment is obsolete: true
Attachment #8345827 - Attachment is obsolete: true
Add tests for getConnections() and cancel().
Attachment #8345837 - Attachment is obsolete: true
Attachment #8340922 - Attachment is obsolete: true
Attachment #8397073 - Flags: review?(nsm.nikhil)
Attachment #8343638 - Attachment is obsolete: true
Attachment #8397074 - Flags: review?(nsm.nikhil)
Attached patch Part 3, clean up the alignment (obsolete) — Splinter Review
Attachment #8346341 - Attachment is obsolete: true
Attachment #8397075 - Flags: review?(nsm.nikhil)
Attachment #8346340 - Attachment is obsolete: true
Attachment #8397078 - Flags: review?(squibblyflabbetydoo)
Attachment #8397078 - Flags: feedback?(nsm.nikhil)
Hi Nikhil,

Part 1-3 patches are aimed to test the the main functions in InterAppCommService. To do so, I extract the InterAppCommService.jsm which can be applied on the xpcshell tests. This test can cover most of the main logic in receiveMessage() to ensure no one will mess up any existing internal logic in IAC.

To test the complete API usage, IPC flow and apps' manifest registration, I make the part 4 patch based on the Gaia UI Marionette API tests which can be tested on the try server as well.
Hi Jim,

I make the part 4 patch based on the Gaia UI Marionette tests which can verify the correctness of IAC APIs, including connect(), getConnections() and cancel(). This test will run on a real UI which cannot only be automatically tested by Marionette but can also be tested manually.

Please refer to the screen shot. The users can type some messages in the field and send it from the publisher app. Then it will wake up another subscriber app (via system message) which will directly echo the same message back to the publisher. The python test will check if the returned message matches the original one we just sent.

Could you please take the review or pass it to someone who is suitable to review this? Thanks!
I don't think I'm the right person to review these tests. I don't have any IAC experience as an implementor or developer. Maybe :ferjm or someone on Gaia actually using it?
Sure. Thanks Nikhil and sorry for delaying this for a long time.
Comment on attachment 8397073 [details] [diff] [review]
Part 1, create InterAppCommService.jsm

Please see comment #18.
Attachment #8397073 - Flags: review?(nsm.nikhil) → review?(ferjmoreno)
Comment on attachment 8397074 [details] [diff] [review]
Part 2, xpcshell tests for InterAppCommService.jsm

Please see comment #18.
Attachment #8397074 - Flags: review?(nsm.nikhil) → review?(ferjmoreno)
Comment on attachment 8397075 [details] [diff] [review]
Part 3, clean up the alignment

Please see comment #18.
Attachment #8397075 - Flags: review?(nsm.nikhil) → review?(ferjmoreno)
Comment on attachment 8397078 [details]
Part 4, Gaia UI Marionette tests for APIs

Please see comment #19.
Attachment #8397078 - Flags: feedback?(nsm.nikhil) → review?(ferjmoreno)
Sorry for the delay here, I've been traveling the whole week. I'll get to this on Monday.
Comment on attachment 8397073 [details] [diff] [review]
Part 1, create InterAppCommService.jsm

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

Thanks Gene!

I was wondering if we could clean this up a little more and get rid of nsIInterAppCommService and InterAppCommService.js. I can't see the reason for it to be an XPCOM component. We can just use InterAppCommService.jsm and lazy get it from Webapps.jsm. It seems that we are only using it there.

::: dom/apps/src/InterAppCommService.js
@@ +3,5 @@
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
>  const { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components;

'Cc' and 'Cr' are not used anymore.

::: dom/apps/src/InterAppCommService.jsm
@@ +42,5 @@
>                    "child-process-shutdown"];
>  
> +this.InterAppCommService = {
> +
> +// For the convenience of review, will align init() by a follow-up patch.

Thanks :)
Attachment #8397073 - Flags: review?(ferjmoreno)
Attachment #8397075 - Flags: review?(ferjmoreno) → review+
Comment on attachment 8397074 [details] [diff] [review]
Part 2, xpcshell tests for InterAppCommService.jsm

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

::: dom/apps/tests/unit/test_inter_app_comm_service.js
@@ +1,4 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;

'Cr' is never used.

@@ +3,5 @@
> +
> +const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
> +
> +Cu.import("resource://gre/modules/InterAppCommService.jsm");
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");

Do we need to import 'XPCOMUtils'?

@@ +195,5 @@
> +  create_message_port_pair(MESSAGE_PORT_ID,
> +                           KEYWORD,
> +                           PUB_APP_MANIFEST_URL,
> +                           SUB_APP_MANIFEST_URL);
> +  

nit: white space

@@ +204,5 @@
> +  do_check_eq(PUB_APP_PAGE_URL, messagePortPair.publisher.pageURL);
> +  do_check_eq(SUB_APP_PAGE_URL, messagePortPair.subscriber.pageURL);
> +
> +  do_check_true(targets.pubTarget === messagePortPair.publisher.target);
> +  do_check_true(targets.subTarget === messagePortPair.subscriber.target);

I think you can use do_check_eq also here.

@@ +225,5 @@
> +                === undefined);
> +
> +  clear_message_port_pairs();
> +  run_next_test();
> +});

Can we also test registering/unregistering a message port with wrong ID/manifestURL?
Attachment #8397074 - Flags: review?(ferjmoreno) → review+
Comment on attachment 8397078 [details]
Part 4, Gaia UI Marionette tests for APIs

Thanks Gene!

The test looks good.

It would be great if we could play a little bit with the connection 'rules' in both publisher and subscriber. We can probably add different types of apps (probably with 'role': 'system' for the ones that don't require UI) and check that the connection success/fails depending on the connection rules. Right now we are only testing the successful connection path.
Attachment #8397078 - Flags: review?(ferjmoreno) → feedback+
Attachment #8397073 - Attachment is obsolete: true
Attachment #8406777 - Flags: review?(ferjmoreno)
(In reply to Fernando Jiménez Moreno [:ferjm] from comment #28)
> > +  do_check_true(targets.pubTarget === messagePortPair.publisher.target);
> > +  do_check_true(targets.subTarget === messagePortPair.subscriber.target);
> 
> I think you can use do_check_eq also here.

do_check_eq(a, b) is using "==" to compare. I hope to have a stricter comparison. ;)
Fixed comment #28 and add two more tests:

test_failToRegisterMessagePort
test_failToUnregisterMessagePort

Carry on r=ferjm. Thank you for the review!
Attachment #8397074 - Attachment is obsolete: true
Attachment #8406793 - Flags: review+
Carry on r=ferjm. Thanks for the review!
Attachment #8397075 - Attachment is obsolete: true
Attachment #8406794 - Flags: review+
No longer depends on: inter-app-comm-api
Comment on attachment 8406777 [details] [diff] [review]
Part 1, create InterAppCommService.jsm, V2

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

Yay! No XPCOM :) Thanks Gene!
Attachment #8406777 - Flags: review?(ferjmoreno) → review+
Comment on attachment 8397078 [details]
Part 4, Gaia UI Marionette tests for APIs

This seems good to me. However, you should probably remove the unnecessary permissions for the test apps. r=me with that change.
Attachment #8397078 - Flags: review?(squibblyflabbetydoo) → review+
Hi Fernando, please the e-mail for why I add the IDL back. ;)
Attachment #8406777 - Attachment is obsolete: true
Attachment #8408826 - Flags: review?(ferjmoreno)
Rebased.
Attachment #8406793 - Attachment is obsolete: true
Attachment #8408827 - Flags: review+
Rebased.
Attachment #8406794 - Attachment is obsolete: true
Attachment #8408828 - Flags: review+
Attachment #8408826 - Flags: review?(ferjmoreno) → review+
Flags: in-testsuite+
Comment on attachment 8397078 [details]
Part 4, Gaia UI Marionette tests for APIs

Add more tests for the error path of doing IAC connection.
Attachment #8397078 - Flags: feedback+ → review?(ferjmoreno)
Depends on: 1003689
Comment on attachment 8397078 [details]
Part 4, Gaia UI Marionette tests for APIs

Thanks Gene!
Attachment #8397078 - Flags: review?(ferjmoreno) → review+
Attachment #8408826 - Flags: checkin+
Attachment #8408827 - Flags: checkin+
Attachment #8408828 - Flags: checkin+
Whiteboard: Only need to land part-4 patch from Gaia's PR
Master: https://github.com/mozilla-b2g/gaia/commit/d86bd639cedf32cb6b82e99f175381bf26fc0f81
Status: NEW → RESOLVED
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: Only need to land part-4 patch from Gaia's PR
Target Milestone: --- → mozilla31
Depends on: 1008824
Depends on: 1009482
You need to log in before you can comment on or make changes to this bug.