926280 - Support OOP in MarionetteJSTestCase

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Vicamo Yang [:vicamo][:vyang]]

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.

Comment 1

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 1/3: refactor MarionetteJSTestCase

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.

Comment 2

[Vicamo Yang [:vicamo][:vyang]]

Attachment: part 2/3: new variable MARIONETTE_OOP : WIP

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.

Comment 3

[Vicamo Yang [:vicamo][:vyang]]

Attachment: test.example

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.

Comment 4

[Jonathan Griffin (:jgriffin)]

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.

Comment 5

[Jonathan Griffin (:jgriffin)]

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.

Comment 6

[Jonathan Griffin (:jgriffin)]

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

Comment 7

[Jonathan Griffin (:jgriffin)]

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.

Comment 8

[Jonathan Griffin (:jgriffin)]

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.

Comment 9

[Vicamo Yang [:vicamo][:vyang]]

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

Comment 10

[Jonathan Griffin (:jgriffin)]

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

Comment 11

[Vicamo Yang [:vicamo][:vyang]]

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.

Comment 12

[Jonathan Griffin (:jgriffin)]

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

Comment 13

[Jonathan Griffin (:jgriffin)]

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

Comment 14

[Jonathan Griffin (:jgriffin)]

Attachment: Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/13349

Pointer to Github pull-request

Comment 15

[Jonathan Griffin (:jgriffin)]

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.

Comment 16

[Jonathan Griffin (:jgriffin)]

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

Will watch the tree for problems.

Comment 17

[Jonathan Griffin (:jgriffin)]

Attachment: Support OOP webapi tests

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.

Comment 18

[Jonathan Griffin (:jgriffin)]

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

Comment 19

[Dave Hunt [:davehunt] [he/him] ⌚BST]

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.

Comment 20

[Jonathan Griffin (:jgriffin)]

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

Comment 21

[Jonathan Griffin (:jgriffin)]

Attachment: Support OOP webapi tests

Updated with review comments

Comment 22

[Jonathan Griffin (:jgriffin)]

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

Comment 23

[Jonathan Griffin (:jgriffin)]

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

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

Try looks good; asking for review.

Comment 24

[Vicamo Yang [:vicamo][:vyang]]

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?

Comment 25

[Andrew Halberstadt [:ahal]]

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.

Comment 26

[Jonathan Griffin (:jgriffin)]

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

Comment 27

[Jonathan Griffin (:jgriffin)]

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

Comment 28

[Carsten Book [:Tomcat]]

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

Comment 29

[Vicamo Yang [:vicamo][:vyang]]

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

Comment 30

[Vicamo Yang [:vicamo][:vyang]]

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

See bug 926277 for further discussion/work.

Comment 31

[Jonathan Griffin (:jgriffin)]

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

Comment 32

[Jonathan Griffin (:jgriffin)]

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