Closed Bug 1049489 Opened 7 years ago Closed 7 years ago
Experiment with executing Gaia
Data methods inside the CHROME context instead of content/System app context
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.
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.
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' ;)
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!
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 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.
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?
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.
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 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?
Attachment #8471870 - Flags: review?(dave.hunt) → review-
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.
I am only touching the MozContacts API for now, FWIW.
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?
(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?
yes, I'll start on a new pull with just that part.
+smoketest adhoc job running here: http://jenkins1.qa.scl3.mozilla.com/job/flame.b2g-inbound.ui.adhoc/20/console
Comment on attachment 8473638 [details] [review] github pr Works for me as a temporary solution until we can do something more elegant.
Attachment #8473638 - Flags: review?(dave.hunt) → review+
Comment on attachment 8473638 [details] [review] github pr Passing this to Rob now.
Attachment #8473638 - Flags: review?(bob.silverberg) → review?(rwood)
Comment on attachment 8473638 [details] [review] github pr LGTM, just a couple of questions in the PR.
Attachment #8473638 - Flags: review?(rwood) → review+
Merged: https://github.com/mozilla-b2g/gaia/commit/863626285a67f54e795d31adaf8de5b9fd030629 Ehsan you're unblocked, sorry it took some time.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Thanks, Zac! I'll give this a go.
You need to log in before you can comment on or make changes to this bug.