1049489 - Experiment with executing GaiaData methods inside the CHROME context instead of content/System app context

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::UI Tests, defect, P1)

Description

[Zac C (:zac)]

As developers are closing out the System app's access to various APIs we might find some of our test framework functionality failing.

This is a pretty big change so we need to do some thorough testing on all platforms and perhaps release a version of gaiatest before/after to be nice to the gaiatest users.

Bob's experiment with the @decorator in Bug 1005057 might be a good place to start, otherwise the code might end up being very verbose.

This is a P1 because of the risk and scale rather than the urgency.

Comment 1

[Zac C (:zac)]

I've experimented with the decorator (example in bug 1005057) but it is limited to only certain types of functions and has difficulties when we're passing in various arguments.

Comment 2

[Zac C (:zac)]

I've experimented with this using verbose code just to see if it works. I've done some basic device only testing with the branch (linked below) and it works but I'd want some more thorough testing to be sure that it will not cause regressions in some edge cases.

There are some more methods in the GaiaData class that should probably switch to the chrome context first. They don't use atoms - they just call the APIs directly. If they operate in the chrome context then we mitigate the risk of changes to those APIs breaking tests in the future too.

https://github.com/zacc/gaia/compare/bug_1049489

Bob can you take over with this while I'm off on PTO? See the bug this is blocking for 'context' ;)

Comment 3

[(no longer active)]

If doing a total overhaul here would take a long time, it would be awesome if we could just focus on the contacts tests here and defer the rest of the work to another bug, so that I can land my patch in bug 1043562 sooner.  Thanks!

Comment 4

[Bob Silverberg [:bsilverberg]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/22796

I'm not sure how far Zac got with this, but I made a change that makes it less verbose and I am running an adhoc of that. Let's see how close this gets us.

Comment 5

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8471870 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/22796

The tests run this far seem to indicate that this PR would be ok to merge, pending review. The rerun of the Gaia Try server was green, tests are passing locally, and a adhoc on CI didn't yield any unexpected failures.

Comment 6

[(no longer active)]

Thanks a lot, Bob!  Any chance you could check to see that these tests also pass with my patch in bug 1043562 applied to Gecko?

Comment 7

[Bob Silverberg [:bsilverberg]]

Tbh, I'm not totally sure how to do that, but it should be pretty easy for you to do it the other way around. Just take my branch and apply it to gaia and then run the tests to see if it fixes them.

Comment 8

[(no longer active)]

The problem is I don't know how to run these tests locally, I was seeing the failures on try.  I'd be happy to test it if you can guide me how to do that.  Or if there is a way to test a Gecko and Gaia change together on try then that's the best option.

Comment 9

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

Comment on attachment 8471870 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/22796

I'm not a fan of creating execute_async_script_in_chrome and execute_script_in_chrome methods just to switch context to chrome and back. I'd really prefer something like a decorator or a helper that takes a function.

This could look like:

@chrome
@property
def all_contacts(self):
    return self.marionette.execute_async_script('return GaiaDataLayer.getAllContacts();', special_powers=True)

Or potentially something like:

@property
def all_contacts(self):
    return self.in_chrome_context(self.marionette.execute_async_script('return GaiaDataLayer.getAllContacts();', special_powers=True))

Maybe this is even something the Marionette client could be enhanced for, so it will take care of switching context back and forth.

That said, if this is blocking them I'd be okay with the patch as it is so long as there are no regressions. Do we need to do the same with the GaiaDevice or GaiaApps methods too?

Comment 10

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

After speaking briefly with :ato and :jgraham, we came up with the idea of having a context manager in the Marionette client, which would allow something like with self.marionette.context_chrome() ...

This would require knowing the current context (so it could be switched back). I'm not sure how likely this is to be worked on though given the current priorities, so maybe the more verbose approach will suffice. It may also be worth considering just changing the method calls used by Ehsan.

Comment 11

[(no longer active)]

I am only touching the MozContacts API for now, FWIW.

Comment 12

[Zac C (:zac)]

OK let's see if we can not over-complicate this so we unblock Ehsan.

Dave, the lead time on that sounds a bit long. Do you think we can go with one of the proposed working versions initially and use the Marionette client style when it's completed and released?

Comment 13

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

(In reply to Zac C (:zac) from comment #12)
> OK let's see if we can not over-complicate this so we unblock Ehsan.
> 
> Dave, the lead time on that sounds a bit long. Do you think we can go with
> one of the proposed working versions initially and use the Marionette client
> style when it's completed and released?

Yep, I'm happy with that compromise - it's not even guaranteed that we'd go ahead with adding this to the Python client, but I suspect this would be something not entirely uncommon. Is it worth only changing the MozContacts API though, to limit the impact and still unblock Ehsan?

Comment 14

[Zac C (:zac)]

yes, I'll start on a new pull with just that part.

Comment 15

[Zac C (:zac)]

Attachment: github pr

Comment 16

[Zac C (:zac)]

+smoketest adhoc job running here:
http://jenkins1.qa.scl3.mozilla.com/job/flame.b2g-inbound.ui.adhoc/20/console

Comment 17

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

Comment on attachment 8473638 [details] [review]
github pr

Works for me as a temporary solution until we can do something more elegant.

Comment 18

[Zac C (:zac)]

Comment on attachment 8473638 [details] [review]
github pr

Passing this to Rob now.

Comment 19

[Robert Wood [:rwood]]

Comment on attachment 8473638 [details] [review]
github pr

LGTM, just a couple of questions in the PR.

Comment 20

[Zac C (:zac)]

Merged:
https://github.com/mozilla-b2g/gaia/commit/863626285a67f54e795d31adaf8de5b9fd030629

Ehsan you're unblocked, sorry it took some time.

Comment 21

[(no longer active)]

Thanks, Zac!  I'll give this a go.