Vicamo Yang [:vicamo][:vyang] Reporter
	Description • 11 years ago

All MarionetteJSTestCase instances are now running in chrome process.  It will be very handy to be able to run the same test script with both chrome and content process for those testing WebAPIs supporting both modes.

	Vicamo Yang [:vicamo][:vyang] Reporter
	Comment 1 • 11 years ago

Like MarionetteTestCase, which is for test scripts written in python, the idea here is to push two test case instances to test suite.  One for OOP, another for legacy chrome process.  Script body paring code is moved outside |runTest| so that we only parse input once.

	Vicamo Yang [:vicamo][:vyang] Reporter
	Comment 2 • 11 years ago

This patch introduces new in-script config variable MARIONETTE_OOP and implements necessary steps to create an remote iframe that parse js script will be executed.

	Vicamo Yang [:vicamo][:vyang] Reporter
	Comment 3 • 11 years ago

This patch is an example to patch existing test scripts.  However, it always fails because permission 'sms' is not granted in that remote iframe.  Don't quite know what's happening here.

	Jonathan Griffin (:jgriffin) Assignee
	Comment 4 • 11 years ago

We probably need to set the mozapp property on the remote iframe to a manifest, like https://github.com/mozilla-b2g/gaia/blob/master/test_apps/test-container/manifest.webapp.  We could try making this manifest a data url to avoid having to get it hosted by the webserver, which would introduce some requirements about device and host machine being on the same network that people have resisted before.

	Jonathan Griffin (:jgriffin) Assignee
	Comment 5 • 11 years ago

Regarding specialpowers in remote frames, it needs to be specifically hooked up; we have code in mochitest to do this here:  http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/b2g_start_script.js#53

We'd need to add similar code to marionette_test where you initialize the remote frame.

	Jonathan Griffin (:jgriffin) Assignee
	Comment 6 • 11 years ago

(In reply to Jonathan Griffin (:jgriffin) from comment #5)
> Regarding specialpowers in remote frames, it needs to be specifically hooked
> up; we have code in mochitest to do this here: 
> http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/
> b2g_start_script.js#53
> 
> We'd need to add similar code to marionette_test where you initialize the
> remote frame.

Hmm, looking at your code, it appears that this isn't necessary after all.

	Jonathan Griffin (:jgriffin) Assignee
	Comment 7 • 11 years ago

I've been trying to get these perms to work, but to no avail.  We may have to do what mochitest does...launch the test-container app (or other test app) and do the work inside that.

	Jonathan Griffin (:jgriffin) Assignee
	Comment 8 • 11 years ago

I got this working locally by running the tests inside the test-container app, after hacking that app to have the sms permissions.

My patch is quite hacky so I'm going to clean it up before posting it.

	Vicamo Yang [:vicamo][:vyang] Reporter
	Comment 9 • 11 years ago

(In reply to Jonathan Griffin (:jgriffin) from comment #8)
> I got this working locally by running the tests inside the test-container
> app, after hacking that app to have the sms permissions.
> 
> My patch is quite hacky so I'm going to clean it up before posting it.

Awesome! Thanks!

	Jonathan Griffin (:jgriffin) Assignee
	Comment 10 • 11 years ago

This approach works well, but requires the sms permission be added to gaia's test-container app, which I'll do in a separate patch.  It follows a different model than your patch, Vicamo...if you flag tests as oop=true in the manifest, then if you invoke runtests.py with --type b2g+oop, it will run only oop=true tests in OOP mode.  If you specify just --type b2g, it will run all the tests in in-process (default) mode.

We _could_ add support for OOP and in-process tests in the same run, like you were trying to do, but it would require us to add another test app to gaia that would be blacklisted from running OOP.  Let me know what you think.

	Vicamo Yang [:vicamo][:vyang] Reporter
	Comment 11 • 11 years ago

Thank you!

(In reply to Jonathan Griffin (:jgriffin) from comment #10)
> Created attachment 825373 [details] [diff] [review]
> Support OOP webapi tests
> 
> This approach works well, but requires the sms permission be added to gaia's
> test-container app, which I'll do in a separate patch.

But I want to run nearly all RIL Marionette test cases in both OOP and non-OOP mode, which follows we have to add all RIL related permissions to Gaia test-container app.  Is it really a good idea?  Judging by it's a "test-container" app, it sounds reasonable enough to add all permissions.

So what if some code want's to test without a certain permission?  For example, we might want the ultimate API hiding mechanism to base on app permissions (and platform support of course).  In this case we can't have a test case for such validity checking which runs in OOP mode.  Is this a important use-case?  I can't really tell.

test-container sounds like a new direction for me.  But etching required permissions in Gaia, not even in Gecko, is a good motivation for me to search for another way.

> It follows a
> different model than your patch, Vicamo...if you flag tests as oop=true in
> the manifest, then if you invoke runtests.py with --type b2g+oop, it will
> run only oop=true tests in OOP mode.  If you specify just --type b2g, it
> will run all the tests in in-process (default) mode.
> 
> We _could_ add support for OOP and in-process tests in the same run, like
> you were trying to do, but it would require us to add another test app to
> gaia that would be blacklisted from running OOP.  Let me know what you think.

I think that's easy.  It should only take a few changes to support running the same case in both OOP and non-OOP mode.  Just use the trick I had -- add the same script twice to test suite.

	Jonathan Griffin (:jgriffin) Assignee
	Comment 12 • 11 years ago

> But I want to run nearly all RIL Marionette test cases in both OOP and non-OOP mode, which follows we 
> have to add all RIL related permissions to Gaia test-container app.  Is it really a good idea?  
> Judging by it's a "test-container" app, it sounds reasonable enough to add all permissions.

Yes, I think it's fine.

> So what if some code want's to test without a certain permission?  For example, we might want the 
> ultimate API hiding mechanism to base on app permissions (and platform support of course).  In this 
> case we can't have a test case for such validity checking which runs in OOP mode.  Is this a 
> important use-case?  I can't really tell.

Ideally, the harness would dynamically generate a test app and manifest based on the test's requirements.  I haven't been able to figure out the right magic to get this to work yet, but it's something that could be easily plugged into this architecture later.

> I think that's easy.  It should only take a few changes to support running the same case in both OOP 
> and non-OOP mode.  Just use the trick I had -- add the same script twice to test suite.

It's a little more complicated than that.  Right now, OOP tests are run inside a gaia app, but non-OOP tests are run in a way that basically unloads gaia entirely.  We can't switch back and forth between modes in the same run.  We could do this if we changed non-OOP tests to run in a (different) gaia app, though.

	Jonathan Griffin (:jgriffin) Assignee
	Comment 13 • 11 years ago

> It's a little more complicated than that.  Right now, OOP tests are run inside a gaia app, but non-OOP 
> tests are run in a way that basically unloads gaia entirely.  We can't switch back and forth between 
> modes in the same run.  We could do this if we changed non-OOP tests to run in a (different) gaia app, 
> though.

Another thing we could do is run all of the OOP tests first, then all of the non-OOP tests.  We could do this in the same test run.  (We just can't switch back and forth between the two modes...but we can switch once from OOP to non-OOP.)

	Jonathan Griffin (:jgriffin) Assignee
	Comment 14 • 11 years ago

Pointer to Github pull-request

	Jonathan Griffin (:jgriffin) Assignee
	Comment 15 • 11 years ago

Comment on attachment 826754 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/13349

This adds the SMS perms to the test container app.  I don't *think* this should cause any problems with mochitests.

	Jonathan Griffin (:jgriffin) Assignee
	Comment 16 • 11 years ago

https://github.com/mozilla-b2g/gaia/commit/048bb99a642060241f813ab7181df7fdd901c69e

Will watch the tree for problems.

	Jonathan Griffin (:jgriffin) Assignee
	Comment 17 • 11 years ago

WIP.  This itereation allows OOP and non-OOP tests to be run in the same run; it orders all non-OOP tests first, then runs any OOP tests second.

It makes a change to emulator.py to disable the FTU and lockscreen.  I don't think anything depends on these, but will run this on try to test it.  It may impact gaia-ui-tests, which aren't yet running in TPBL on the emulator.

	Jonathan Griffin (:jgriffin) Assignee
	Comment 18 • 11 years ago

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

	Dave Hunt [:davehunt] [he/him] ⌚BST
	Comment 19 • 11 years ago

Comment on attachment 827634 [details] [diff] [review]
Support OOP webapi tests

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

::: testing/marionette/client/marionette/emulator.py
@@ +354,5 @@
> +window.wrappedJSObject.asyncStorage.setItem('ftu.enabled', false);
> +""")
> +        marionette.execute_async_script("""
> +window.addEventListener('mozbrowserloadend', function loaded(aEvent) {
> +  if (aEvent.target.src.indexOf('ftu') != -1 || aEvent.target.src.indexOf('homescreen') != -1) {

Would this be 'ftu' if the FTU is disabled?

::: testing/marionette/client/marionette/marionette_test.py
@@ +402,5 @@
> +let origin = window.wrappedJSObject.WindowManager.getDisplayedApp();
> +if (origin == 'app://test-container.gaiamobile.org') {
> +    marionetteScriptFinished(true);
> +}
> +let appsReq = navigator.mozApps.mgmt.getAll();

In bug 924565 this was identified as a performance issue and will likely be replaced with window.wrappedJSObject.Applications.installedApps.

	Jonathan Griffin (:jgriffin) Assignee
	Comment 20 • 11 years ago

(In reply to Dave Hunt (:davehunt) from comment #19)
> Comment on attachment 827634 [details] [diff] [review]
> Support OOP webapi tests
> 
> Review of attachment 827634 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: testing/marionette/client/marionette/emulator.py
> @@ +354,5 @@
> > +window.wrappedJSObject.asyncStorage.setItem('ftu.enabled', false);
> > +""")
> > +        marionette.execute_async_script("""
> > +window.addEventListener('mozbrowserloadend', function loaded(aEvent) {
> > +  if (aEvent.target.src.indexOf('ftu') != -1 || aEvent.target.src.indexOf('homescreen') != -1) {
> 
> Would this be 'ftu' if the FTU is disabled?

No, it would be 'homescreen' then.  You're right, the way the tests are currently written it will never be 'ftu' so I can remove it.

> 
> ::: testing/marionette/client/marionette/marionette_test.py
> @@ +402,5 @@
> > +let origin = window.wrappedJSObject.WindowManager.getDisplayedApp();
> > +if (origin == 'app://test-container.gaiamobile.org') {
> > +    marionetteScriptFinished(true);
> > +}
> > +let appsReq = navigator.mozApps.mgmt.getAll();
> 
> In bug 924565 this was identified as a performance issue and will likely be
> replaced with window.wrappedJSObject.Applications.installedApps.

Thanks, I'll update it accordingly.

	Jonathan Griffin (:jgriffin) Assignee
	Comment 21 • 11 years ago

Updated with review comments

	Jonathan Griffin (:jgriffin) Assignee
	Comment 22 • 11 years ago

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

	Jonathan Griffin (:jgriffin) Assignee
	Comment 23 • 11 years ago

Comment on attachment 828181 [details] [diff] [review]
Support OOP webapi tests

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

Try looks good; asking for review.

	Vicamo Yang [:vicamo][:vyang] Reporter
	Comment 24 • 11 years ago

Comment on attachment 828181 [details] [diff] [review]
Support OOP webapi tests

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

Hi Jonathan, we have already bug 926277 for enabling OOP in RIL tests, maybe we can move mobilemessage manifest changes to that bug and let me have a chance to scan all RIL marionette test cases.  Maybe you want a simple unit test script for Marionette instead?

	Andrew Halberstadt [:ahal]
	Comment 25 • 11 years ago

Comment on attachment 828181 [details] [diff] [review]
Support OOP webapi tests

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

::: testing/marionette/client/marionette/emulator.py
@@ +346,5 @@
>          self._run_adb(['shell', 'setprop', 'net.dns1', '10.0.2.3'])
>  
> +    def wait_for_homescreen(self, marionette, disable_lockscreen=True):
> +        print 'waiting for homescreen...'
> +        marionette.start_session()

In the b2g emulator unittest case, we might want to wait for the homescreen the second time around (after having restarted the b2g process). This will cause a failure because the marionette session already exists at that point. I fixed it locally by doing:

created_new_session = False
if not marionette.session:
    created_new_session = True
    marionette.start_session()
...
if created_new_session:
    marionette.delete_session()

::: testing/marionette/client/marionette/marionette.py
@@ +491,4 @@
>              self.emulator.setup(self,
>                                  gecko_path=gecko_path,
>                                  busybox=busybox)
> +            self.emulator.wait_for_homescreen(self)

I'd prefer if this wasn't implicitly called by marionette.py. For emulator unittests, we start the emulator and then immediately restart the b2g process so this causes us to wait for the homescreen unnecessarily.

	Jonathan Griffin (:jgriffin) Assignee
	Comment 26 • 11 years ago

addressed review comments and pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=36abdfb3074f

	Jonathan Griffin (:jgriffin) Assignee
	Comment 27 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/76ddaf7db5bf

	Carsten Book [:Tomcat]
	Comment 28 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/76ddaf7db5bf

	Vicamo Yang [:vicamo][:vyang] Reporter
	Comment 29 • 11 years ago

(In reply to Jonathan Griffin (:jgriffin) from comment #26)
> addressed review comments and pushed to try:
> https://tbpl.mozilla.org/?tree=Try&rev=36abdfb3074f

Looks like the landed one differs from the one reviewed in comment 24 a lot.  I can no longer runs oop and non-oop test cases in the same run :(  The reason we want oop support in marionette is to deploy oop test cases on tbpl, but now oop ability is *always* disabled because that |oop| variable in BaseMarionetteTestRunner is always evaluated to 'false' now.

	Vicamo Yang [:vicamo][:vyang] Reporter
	Comment 30 • 11 years ago

(In reply to Vicamo Yang [:vicamo][:vyang] (PTO Dec. 21 ~ Jan. 5) from comment #29)

See bug 926277 for further discussion/work.

	Jonathan Griffin (:jgriffin) Assignee
	Comment 31 • 11 years ago

(In reply to Vicamo Yang [:vicamo][:vyang] (PTO Dec. 21 ~ Jan. 5) from comment #29)
> (In reply to Jonathan Griffin (:jgriffin) from comment #26)
> > addressed review comments and pushed to try:
> > https://tbpl.mozilla.org/?tree=Try&rev=36abdfb3074f
> 
> Looks like the landed one differs from the one reviewed in comment 24 a lot.
> I can no longer runs oop and non-oop test cases in the same run :(  The
> reason we want oop support in marionette is to deploy oop test cases on
> tbpl, but now oop ability is *always* disabled because that |oop| variable
> in BaseMarionetteTestRunner is always evaluated to 'false' now.

Sorry about that; I'll re-open this bug to take care of this.

	Jonathan Griffin (:jgriffin) Assignee
	Comment 32 • 11 years ago

Nm, I see you already have a patch in bug 926277.