We need to add tests for Inter-App Communication API. This is definitely the first priority.

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...

(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.

Will work out marionette test to test API level as well.

Add tests for getConnections() and cancel().

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.

Comment on attachment 8397074 [details] [diff] [review] Part 2, xpcshell tests for InterAppCommService.jsm Please see comment #18.

Comment on attachment 8397075 [details] [diff] [review] Part 3, clean up the alignment Please see comment #18.

Comment on attachment 8397078 [details] Part 4, Gaia UI Marionette tests for APIs Please see comment #19.

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 :)

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?

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.

(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!

Carry on r=ferjm. Thanks for the review!

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!

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.

Hi Fernando, please the e-mail for why I add the IDL back. ;)

Rebased.

Rebased.

I'd like land the Gecko tests first. Gaia UI Test needs to be polished according comment #29. https://hg.mozilla.org/integration/b2g-inbound/rev/223ae5cffe53 https://hg.mozilla.org/integration/b2g-inbound/rev/991793077944 https://hg.mozilla.org/integration/b2g-inbound/rev/1b88876e6eaf Please leave this bug open.

https://hg.mozilla.org/mozilla-central/rev/223ae5cffe53 https://hg.mozilla.org/mozilla-central/rev/991793077944 https://hg.mozilla.org/mozilla-central/rev/1b88876e6eaf

Comment on attachment 8397078 [details] Part 4, Gaia UI Marionette tests for APIs Add more tests for the error path of doing IAC connection.

Comment on attachment 8397078 [details] Part 4, Gaia UI Marionette tests for APIs Thanks Gene!

Master: https://github.com/mozilla-b2g/gaia/commit/d86bd639cedf32cb6b82e99f175381bf26fc0f81