Closed Bug 926280 Opened 6 years ago Closed 6 years ago

Support OOP in MarionetteJSTestCase

Categories

(Testing :: Marionette, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla29

People

(Reporter: vicamo, Assigned: jgriffin)

References

Details

Attachments

(2 files, 5 obsolete files)

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.
Blocks: 926277
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.
Assignee: nobody → vyang
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.
Attachment #816432 - Flags: feedback?
Attached patch test.example (obsolete) — Splinter Review
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.
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.
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.
(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.
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.
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.
(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!
Attached patch Support OOP webapi tests (obsolete) — Splinter Review
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.
Assignee: vyang → jgriffin
Attachment #825373 - Flags: feedback?(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.
> 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.
> 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 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.
Attachment #826754 - Flags: review?(ahalberstadt)
Attachment #826754 - Flags: review?(ahalberstadt) → review+
Attached patch Support OOP webapi tests (obsolete) — Splinter Review
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 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.
(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.
Updated with review comments
Attachment #827634 - Attachment is obsolete: true
Comment on attachment 828181 [details] [diff] [review]
Support OOP webapi tests

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

Try looks good; asking for review.
Attachment #828181 - Flags: review?(vyang)
Attachment #825373 - Attachment is obsolete: true
Attachment #825373 - Flags: feedback?(vyang)
Attachment #816430 - Attachment is obsolete: true
Attachment #816432 - Attachment is obsolete: true
Attachment #816432 - Flags: feedback?
Attachment #816433 - Attachment is obsolete: true
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?
Attachment #828181 - Flags: review?(vyang) → review+
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.
addressed review comments and pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=36abdfb3074f
https://hg.mozilla.org/mozilla-central/rev/76ddaf7db5bf
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
(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.
(In reply to Vicamo Yang [:vicamo][:vyang] (PTO Dec. 21 ~ Jan. 5) from comment #29)

See bug 926277 for further discussion/work.
(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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Nm, I see you already have a patch in bug 926277.
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Blocks: 970804
You need to log in before you can comment on or make changes to this bug.