We need mochitests for that.

Hi Fabrice, Henry is working on writing test cases for System Message. Do you have any plan in mind to share with Henry?

Fabrice used to mention we should treat it as a blocking bug even if it's just a test. What's the timeline do we prefer?

I'm gonna block any change to this api until we have tests. So if you have blockers, consider them blocked on this one.

Henry, can you upload any work in progress patches? Let me know if you need a hand with this.

Hi Nikhil, I have to be shortly working on other task this week. I hope I can have some WIP by next week. Here's my preliminary idea for the most basic validation: If a system message is defined in manifest.webapp, the handler registered by mozSetMessageHandler MUST (1) be called (2) with the well-wrapped object when gecko send/broadcast a system message at some time later. If an app also defines a page associated with certain message, when receiving that message, the app has to (3) be brought to foreground and (4) go to that page no matter the app is running or not. Since we have to have the system to throw out a system message, I am not pretty sure how to do this in a test app. (maybe SpecialPowers?) Can anyone give me some suggestions? Thanks!

This will be a little tricky. Mochitest has the ability to define test apps with their own manifests, but mochitests cannot verify anything that happens in another app (e.g., in anything other than the mochitest test app). Marionette tests can set up system listeners in chrome or content, but pure Marionette tests can't set up test apps. I think the best tool for this may be gaia-ui-tests; these are Marionette tests that interact with Gaia; you could have a Gaia test app that defines the system message you want to test, and you can use the test to verify behavior of other apps. See examples at https://github.com/mozilla/gaia-ui-tests. We are running gaia-ui-tests on TBPL on b2g desktop builds.

(In reply to Jonathan Griffin (:jgriffin) from comment #6) > This will be a little tricky. > > Mochitest has the ability to define test apps with their own manifests, but > mochitests cannot verify anything that happens in another app (e.g., in > anything other than the mochitest test app). > > Marionette tests can set up system listeners in chrome or content, but pure > Marionette tests can't set up test apps. > > I think the best tool for this may be gaia-ui-tests; these are Marionette > tests that interact with Gaia; you could have a Gaia test app that defines > the system message you want to test, and you can use the test to verify > behavior of other apps. See examples at > https://github.com/mozilla/gaia-ui-tests. > > We are running gaia-ui-tests on TBPL on b2g desktop builds. I do not expect the tests to require one app to interact with another app. But I'd like to be able to kill an app from itself, and then start the same app again. Looks like gaiatest will allow that, and is something I'll look at. jgriffin: is there a way to save state across this 'app restart' during the test?

It depends on what you're saving the state of. You can save the state of things that aren't bound to that particular app, but killing/restarting the app would wipe out any app-specific state, unless you used prefs or something.

https://github.com/nikhilm/gaia/tree/test-systemmessage has a simple application which just exists to provide context for the gaia-ui-test and something that can be killed and restarted. https://github.com/nikhilm/gaia-ui-tests/tree/test-systemmessage adds a unit test file for system messages. Right now it has 3 tests, which use alarms to dispatch system messages. I'm having a small problem with test_app_started(). It works only on a fresh reboot of the b2g desktop emulator, and not running the same test multiple times on an existing instance. The test starts the app, adds a alarm for 1s in the future, kills the app, sleeps for 1.5 seconds, then checks if the app was automatically launched by the system message. jgriffin or davehunt, any idea what could be causing this failure? I'm happy to provide any information.

Thanks for your information! I successfully used interactive python to broadcast system messages. We now can actually send system messages rather than using alarm API. Here's my test commands: >>> from marionette import Marionette >>> marionette = Marionette('localhost', 2828) >>> marionette.start_session() u'10-b2g' >>> marionette.set_context("chrome") True >>> marionette.execute_script(""" ... const { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components; ... Cu.import("resource://gre/modules/XPCOMUtils.jsm"); ... XPCOMUtils.defineLazyGetter(this, "messenger", function() { ... return Cc["@mozilla.org/system-message-internal;1"].getService(Ci.nsISystemMessagesInternal); ... }); ... messenger.broadcastMessage("alarm", {}); ... """) >>> Then there would be an error pops up in Gaia/alarm.js because of the incorrectly passed parameter . It's the evidence that the message was really delivered.

(In reply to Nikhil Marathe [:nsm] from comment #9) > I'm having a small problem with test_app_started(). It works only on a fresh > reboot of the b2g desktop emulator, and not running the same test multiple > times on an existing instance. The test starts the app, adds a alarm for 1s > in the future, kills the app, sleeps for 1.5 seconds, then checks if the app > was automatically launched by the system message. jgriffin or davehunt, any > idea what could be causing this failure? I'm happy to provide any > information. The only thing I can think is that somehow having the previous alarm in the database is preventing the app from launching. When we run these tests on device or desktop builds we remove the entire indexedDB folder between tests. I don't have a lot of experience with using the emulator for Gaia UI tests. I've added some comment on your github commits. Also, I noticed on bug 906175 that Malini mentions: "We should be able to use something like mochitest's test container to create our own test app and trigger a modal from there." which might mean we don't need to commit a test app to the Gaia repository. Flagging Malini for more information here.

gaia-ui-tests seems to meet our requirement. However, during the test, I have to switch context to "chrome". It looks once any command has been executed in content mode, I can no longer correctly execute any script in chrome mode. (is it what https://developer.mozilla.org/en-US/docs/Marionette/Marionette#set_context() said?) For example, I change to chrome mode, execute XPCOMUtils.defineLazyServiceGetter(this, 'messenger', '@mozilla.org/system-message-internal;1', 'nsISystemMessagesInternal'); It reports an error message which implies "this" is undefined. I have to delete and restart a session, switch to chrome mode then execute my chrome command. Is restarting a session the solution to this issue? Or I got something wrong? Thanks :)

(In reply to Henry Chang [:henry] from comment #12) > gaia-ui-tests seems to meet our requirement. However, during the test, I > have to switch context to "chrome". It looks once any command has been > executed in content mode, I can no longer correctly execute any script in > chrome mode. This sounds like a bug to me. Could you raise a bug and attach a minimal test case that reproduces the issue? As far as I know you should be able to switch back and forth between content and chrome contexts.

(In reply to Dave Hunt (:davehunt) from comment #13) > (In reply to Henry Chang [:henry] from comment #12) > > gaia-ui-tests seems to meet our requirement. However, during the test, I > > have to switch context to "chrome". It looks once any command has been > > executed in content mode, I can no longer correctly execute any script in > > chrome mode. > > This sounds like a bug to me. Could you raise a bug and attach a minimal > test case that reproduces the issue? As far as I know you should be able to > switch back and forth between content and chrome contexts. Sure! I will dig it deeper and file a bug once I make sure I am not doing anything wrong. Thanks.

(In reply to Dave Hunt (:davehunt) from comment #11) > (In reply to Nikhil Marathe [:nsm] from comment #9) > > I'm having a small problem with test_app_started(). It works only on a fresh > > reboot of the b2g desktop emulator, and not running the same test multiple > > times on an existing instance. The test starts the app, adds a alarm for 1s > > in the future, kills the app, sleeps for 1.5 seconds, then checks if the app > > was automatically launched by the system message. jgriffin or davehunt, any > > idea what could be causing this failure? I'm happy to provide any > > information. > > The only thing I can think is that somehow having the previous alarm in the > database is preventing the app from launching. When we run these tests on > device or desktop builds we remove the entire indexedDB folder between > tests. I don't have a lot of experience with using the emulator for Gaia UI > tests. > > I've added some comment on your github commits. Also, I noticed on bug > 906175 that Malini mentions: "We should be able to use something like > mochitest's test container to create our own test app and trigger a modal > from there." which might mean we don't need to commit a test app to the Gaia > repository. Flagging Malini for more information here. I wrote that after hearing this suggestion from jgriffin, I have yet to test it out and see what I need to do to get a test app registered and to use it.

(In reply to Dave Hunt (:davehunt) from comment #11) > I've added some comment on your github commits. Also, I noticed on bug > 906175 that Malini mentions: "We should be able to use something like > mochitest's test container to create our own test app and trigger a modal > from there." which might mean we don't need to commit a test app to the Gaia > repository. Flagging Malini for more information here. So Mochitest has a test application called 'Test Container', but I don't think we should use it outside of Mochitest right now, since it relies on a Mochitest server running to get its manifest.webapp file (which sets up the permissions, I think.). I think we'd need to have a new test app checked in, one that doesn't rely on Mochitest running. That being said, I can show you a working example of using that app as a sandbox. I have some example code here: https://github.com/malini/gaia-ui-tests-1/compare/testcontainer?expand=1 My example code is just adding a div above the iframe container used by mochitest. Tomorrow, I'll work on getting a non-mochitest specific test app we can use. It will look like the example I linked above, but will hopefully have a separate, non-webserver based manifest.

(In reply to Dave Hunt (:davehunt) from comment #13) > (In reply to Henry Chang [:henry] from comment #12) > > gaia-ui-tests seems to meet our requirement. However, during the test, I > > have to switch context to "chrome". It looks once any command has been > > executed in content mode, I can no longer correctly execute any script in > > chrome mode. > > This sounds like a bug to me. Could you raise a bug and attach a minimal > test case that reproduces the issue? As far as I know you should be able to > switch back and forth between content and chrome contexts. I figured out what's happening here. After setting context to chrome, I will be using 'this', which I suppose to be the global variable. However, it's 'null'. It's probably due to marionette-server.js::execute() ... if (directInject) { script = aRequest.value; } else { script = "let func = function() {" + aRequest.value + "};" + "func.apply(null, __marionetteParams);"; } ... If I call marionette.execute_script(), 'directInject' will be falsy since it's not passed to execute(). But it raises another issue. How come 'this' would be 'ChromeWindow' just after restarting the session? What is 'this' intended to be in the injected script? I believe something must be affected by GaiaTestCase python package. The attachment TestChrome.py can be used to reproduce that the injected script sees different 'this'. Does the value of 'this' depends on the state of the sandbox or it should be always null? Thanks.

I also attached a text file with all the marionette commands before TestChrome::test_chrome() gets called

The value of 'this' can change depending on what's going on in the privileged context, although I'm not sure why it would ever be undefined. Using a lazy getter here doesn't make much sense; why don't you just: const { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components; let messenger = Cc["@mozilla.org/system-message-internal;1"] .getService(Ci.nsISystemMessagesInternal); messenger.broadcastMessage("alarm", {}); Also, after you use: marionette.set_context("chrome") every command after that is performed in the chrome context, until you switch back via: marionette.set_context("content")

(In reply to Jonathan Griffin (:jgriffin) from comment #21) > The value of 'this' can change depending on what's going on in the > privileged context, although I'm not sure why it would ever be undefined. > But the thing is, in the chrome context, my script will be wrapped to script = "let func = function() {" + aRequest.value + "};" + "func.apply(null, __marionetteParams);"; ^^^^ According to ECMA5 standard, http://www.ecma-international.org/ecma-262/5.1/#sec-15.3.4.3, 'this' should always be 'null'. Am I right? > Using a lazy getter here doesn't make much sense; why don't you just: > > const { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components; > let messenger = Cc["@mozilla.org/system-message-internal;1"] > .getService(Ci.nsISystemMessagesInternal); > messenger.broadcastMessage("alarm", {}); > Agree. I just copied and pasted some codes to test the marionette's capability. > Also, after you use: > > marionette.set_context("chrome") > > every command after that is performed in the chrome context, until you > switch back via: > > marionette.set_context("content") By the way, is gaia-ui-test TBPL running here https://tbpl.mozilla.org/?tree=Cedar ? (Gu) Thanks.

> But the thing is, in the chrome context, my script will be wrapped to > > script = "let func = function() {" + > aRequest.value + > "};" + > "func.apply(null, __marionetteParams);"; > ^^^^ > According to ECMA5 standard, http://www.ecma-international.org/ecma-262/5.1 > /#sec-15.3.4.3, 'this' should always be 'null'. Am I right? From https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/apply: " thisArg The value of this provided for the call to fun. Note that this may not be the actual value seen by the method: if the method is a function in non-strict mode code, null and undefined will be replaced with the global object, and primitive values will be boxed." So, it doesn't appear that anything wrong is happening here, except possibly for 'this' being undefined sometimes. Cc'ing Bobby Holley for confirmation. > By the way, is gaia-ui-test TBPL running here > https://tbpl.mozilla.org/?tree=Cedar ? (Gu) Thanks. Yes, Gu is gaia-ui-tests.

(In reply to Jonathan Griffin (:jgriffin) from comment #23) > > But the thing is, in the chrome context, my script will be wrapped to > > > > script = "let func = function() {" + > > aRequest.value + > > "};" + > > "func.apply(null, __marionetteParams);"; > > ^^^^ > > According to ECMA5 standard, http://www.ecma-international.org/ecma-262/5.1 > > /#sec-15.3.4.3, 'this' should always be 'null'. Am I right? > > From > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/ > Global_Objects/Function/apply: > > " thisArg > The value of this provided for the call to fun. Note that this may not > be the actual value seen by the method: if the method is a function in > non-strict mode code, null and undefined will be replaced with the global > object, and primitive values will be boxed." > > So, it doesn't appear that anything wrong is happening here, except possibly > for 'this' being undefined sometimes. Cc'ing Bobby Holley for confirmation. Yeah, a null/undefined |this| gets unboxed to the global of the callee in non-strict mode.

Attached a simple system message test. It first broadcasts a fake system message "fake-message" in the chrome context. Then check if the testing app is running after the broadcast. We further check if the message delivered by the system message is correctly sent to the testing app by comparing the DOM element. To do this test, you have to also have a test app in gaia. I will attach the archive the test app int the next attachment.

This is the test app archive for testing system message. It has the message permission of "fake-message". It also displays the message delivered by "fake-message".

I just attached 2 WIP patches: the testing app and the test python. It simply tests if a message will be sent and received properly. The testing app should be running after we send the fake system message.

I changed my previous patches to github (not a PR yet) gaia-ui-tests: Adding gaiatest/tests/unit/test_systemmessage.py https://github.com/elefant/gaia-ui-tests/commit/8b3a4eeb9c7b1e1c5732a32ff5f4256d858da0aa gaia: Adding a test app: test-system-message https://github.com/elefant/gaia/commit/f26f1703de9b86a0a1b18fc601b65e8bba3c0b34 gecko mods: attachment 797755 [details] [diff] [review]. Adding 'fake-message' Can anyone comment on this preliminary WIP? By the way, has gaia-ui-test been added to Try server? I see it on the try chooser. But it doesn't seem to work. https://tbpl.mozilla.org/?tree=Try&rev=dda8c080b9d3 The gaia-ui-tests option was added but not run.

What is the time frame to land this?

(In reply to Henry Chang [:henry] from comment #29) > I changed my previous patches to github (not a PR yet) > > gaia-ui-tests: Adding gaiatest/tests/unit/test_systemmessage.py > https://github.com/elefant/gaia-ui-tests/commit/ > 8b3a4eeb9c7b1e1c5732a32ff5f4256d858da0aa > > gaia: Adding a test app: test-system-message > https://github.com/elefant/gaia/commit/ > f26f1703de9b86a0a1b18fc601b65e8bba3c0b34 Instead of creating a new app for this, you may be able to use the 'Test Container' app as done so in Malini's pull request for testing modal dialogs: https://github.com/mozilla/gaia-ui-tests/pull/1265/files > Can anyone comment on this preliminary WIP? I would recommend requesting feedback from Jonathan Griffin > By the way, has gaia-ui-test been added to Try server? I see it on the try > chooser. But it doesn't seem to work. > https://tbpl.mozilla.org/?tree=Try&rev=dda8c080b9d3 No, there's currently no way to run Gaia UI tests on try.

(In reply to Dave Hunt (:davehunt) from comment #31) > (In reply to Henry Chang [:henry] from comment #29) > > I changed my previous patches to github (not a PR yet) > > > > gaia-ui-tests: Adding gaiatest/tests/unit/test_systemmessage.py > > https://github.com/elefant/gaia-ui-tests/commit/ > > 8b3a4eeb9c7b1e1c5732a32ff5f4256d858da0aa > > > > gaia: Adding a test app: test-system-message > > https://github.com/elefant/gaia/commit/ > > f26f1703de9b86a0a1b18fc601b65e8bba3c0b34 > > Instead of creating a new app for this, you may be able to use the 'Test > Container' app as done so in Malini's pull request for testing modal > dialogs: https://github.com/mozilla/gaia-ui-tests/pull/1265/files > Ok. I will take it a look. But is it a must? The use of system message may involve two apps. For example, app 'A' sends an 'activities' (a kind of system message) to app 'B' and we have to check if 'B' receives the message and be brought to the foreground. > > Can anyone comment on this preliminary WIP? > > I would recommend requesting feedback from Jonathan Griffin > > > By the way, has gaia-ui-test been added to Try server? I see it on the try > > chooser. But it doesn't seem to work. > > https://tbpl.mozilla.org/?tree=Try&rev=dda8c080b9d3 > > No, there's currently no way to run Gaia UI tests on try. Got it. Is it a plan to add gaia-ui-test to try server? From what I know gaia-ui-test is only running on Cedar. If we choose to test system message in the gaia-ui-tests framework, what is the work flow you would suggest? (since I cannot find any rule or purpose of Cedar branch...) Thanks :)

(In reply to Nikhil Marathe [:nsm] from comment #30) > What is the time frame to land this? Sorry for the late reply. I was hoping try server was already be able to run gaia-ui-test but sadly it wasn't yet. I guess most the relevant developer hope they can test system message on try. So if try server is not going to have gaia-ui-test, maybe I have to specialize the current running test frameworks on try.

gaia-ui-tests is available on try, running against b2g desktop builds. To trigger them: -b o -p linux64_gecko -u gaia-ui-test -t none

(In reply to Jonathan Griffin (:jgriffin) from comment #34) > gaia-ui-tests is available on try, running against b2g desktop builds. To > trigger them: > > -b o -p linux64_gecko -u gaia-ui-test -t none Note, you cannot make changes to gaia-ui-tests and run *those* on try; you can only run gecko changes against the current version of gaia-ui-tests on try.

Henry, I'm not sure why you needinfo'd me there? If you can run your tests locally and get them reviewed, I think you should land them, since we don't want to wait for gaia-try to be ready.

(In reply to Jonathan Griffin (:jgriffin) from comment #35) > (In reply to Jonathan Griffin (:jgriffin) from comment #34) > > gaia-ui-tests is available on try, running against b2g desktop builds. To > > trigger them: > > > > -b o -p linux64_gecko -u gaia-ui-test -t none > > Note, you cannot make changes to gaia-ui-tests and run *those* on try; you > can only run gecko changes against the current version of gaia-ui-tests on > try. Sorry for the misinformation Henry, when I said there was no way to run on try, this is what I was referring to. Also, it's worth noting that due to TBPL only running against the desktop client not all the tests are run. You can see tbpl-manifest.ini in gaia-ui-tests for details of what's being run. https://github.com/mozilla/gaia-ui-tests/blob/master/gaiatest/tests/tbpl-manifest.ini

I created a pull request https://github.com/mozilla/gaia-ui-tests/pull/1376 which uses test container as the testing app. It requires another 2 changes: For gaia, we have to add "messages": [ { "dummy-message": "/index.html" } ] to manifest.webapp. I am not sure if we can do this here. For gecko, we have to add 'dummy-message' to SystemMessagePermissionChecker.jsm. I filed a bug 919408 to track it up.

I created a pull request https://github.com/mozilla/gaia-ui-tests/pull/1376 which uses test container as the testing app. It requires another 2 changes: For gaia, we have to add "messages": [ { "dummy-message": "/index.html" } ] to manifest.webapp. I am not sure if we can do this here. For gecko, we have to add 'dummy-message' to SystemMessagePermissionChecker.jsm. I filed a bug 919408 to track it up.

Comment on attachment 808429 [details] PR to gaia-ui-tests/master In general, looks good. Zac is the right person for an in-depth review, which he's already doing.

Merged: https://github.com/mozilla-b2g/gaia/commit/63be88e888611c5bf40f042ba10108d1efe7d875 Henry, do you need this on TBPL and/or B2G26/v1.2?

(In reply to Zac C (:zac) from comment #42) > Merged: > https://github.com/mozilla-b2g/gaia/commit/ > 63be88e888611c5bf40f042ba10108d1efe7d875 > > Henry, do you need this on TBPL and/or B2G26/v1.2? Actually we only need this running on try server so that we can run this case every time we are going to commit patch about system message. What would you suggest on this purpose? Thanks :)

Comment on attachment 832889 [details] pull test to b2g26_v1.2 branch The test is failing on v1.2

Henry, we can't get this to pass on v1.2. It must be missing some Gecko components. We've enabled it for TBPL on mozilla-central, though.

Zac, I believe gecko requires this patch 919408. Do you know what gecko remote I should push for b2g26_v1.2? Thanks!

No I don't, sorry.

It doesn't sounds like v1.2 is being updated to support this test, so I am removing the affected flag. If anyone wants to have coverage for this on v1.2 they'll need to figure out what needs to be uplifted in order to make it work.

Can we close this one since you already landed a Gaia UI test for it?

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #50) > Can we close this one since you already landed a Gaia UI test for it? I would say yes and other specific purpose test case will be tracked by other test case. Thanks for the remind.