Closed
Bug 1005057
Opened 10 years ago
Closed 10 years ago
add switch_to_frame() to GaiaApps and GaiaDataLayer methods that require it
Categories
(Firefox OS Graveyard :: Gaia::UI Tests, defect, P4)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: zcampbell, Unassigned)
References
Details
Attachments
(2 files, 2 obsolete files)
Some of these methods that use atoms assume they are called from the system frame.
Let's follow where we have in other cases and make them switch to the frame first.
Reporter | ||
Comment 1•10 years ago
|
||
Rob, would not be surprised if this busts some assumptions in existing tests so we will test with caution (on functional tests side)
Attachment #8416503 -
Flags: review?(rwood)
Attachment #8416503 -
Flags: review?(florin.strugariu)
Attachment #8416503 -
Flags: review?(bob.silverberg)
Updated•10 years ago
|
Attachment #8416503 -
Flags: review?(florin.strugariu) → review+
Comment 2•10 years ago
|
||
Comment on attachment 8416503 [details]
github pr
Fix works great except it breaks the fmradio_play endurance test, after I noticed that I verified that it also breaks the functional fmradio tests (makes sense). I did not test any of the other functional tests.
For the endurance test (https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/tests/endurance/test_endurance_fmradio_play.py), if you could please add a "self.apps.switch_to_displayed_app()" just before line 38 and that will fix it. Thanks Zac!
Attachment #8416503 -
Flags: review?(rwood) → review-
Comment 3•10 years ago
|
||
Comment on attachment 8416503 [details]
github pr
I have added a question and a discussion point to the PR.
Attachment #8416503 -
Flags: review?(bob.silverberg) → review-
Reporter | ||
Comment 4•10 years ago
|
||
:rwood, yeah not surprised it broke something! I'll fix it up later today and do a full functional test run.
Comment 5•10 years ago
|
||
This is an approach that I think might be good for dealing with functions that need to be executed from the system frame. This decorator would first switch to the system frame, then call the decorated function, then switch back to whatever frame was active before the system frame was switched to.
If we use this decorator approach we both save from having to include all the frame switching code in each function, and we also remove the need for a consumer of any of these methods to know which frame they will be in after the function returns. Ideally, if a function is switching away from the current frame it should also switch back.
Attachment #8418041 -
Flags: feedback?(zcampbell)
Attachment #8418041 -
Flags: feedback?(dave.hunt)
Comment 6•10 years ago
|
||
Comment on attachment 8418041 [details]
Link to a branch that shows decorator usage
I like this, however I believe this will fail if we're in a nested frame. If we can either detect and fail early in that scenario or somehow support switching back to the nested frame, this would be a really nice enhancement!
Attachment #8418041 -
Flags: feedback?(dave.hunt) → feedback+
Comment 7•10 years ago
|
||
@davehunt: Will it fail because switch_to_frame() doesn't work with nested frames, or because get_active_frame() doesn't return the correct frame when it is a nested frame? Either of those sound like marionette issues that should be addressed (and maybe there is even an open bug for them).
Flags: needinfo?(dave.hunt)
Comment 8•10 years ago
|
||
(In reply to Bob Silverberg [:bsilverberg] from comment #7)
> @davehunt: Will it fail because switch_to_frame() doesn't work with nested
> frames, or because get_active_frame() doesn't return the correct frame when
> it is a nested frame? Either of those sound like marionette issues that
> should be addressed (and maybe there is even an open bug for them).
I don't recall ever looking into this myself, so :zac might be a better person to ask. I believe the issue is that get_active_frame will simply return the frame element, which may not be available from the top frame. You would somehow need to know a frame's ancestors and methodically switch through each of these.
Flags: needinfo?(dave.hunt) → needinfo?(zcampbell)
Reporter | ||
Comment 9•10 years ago
|
||
(In reply to Dave Hunt (:davehunt) from comment #8)
> (In reply to Bob Silverberg [:bsilverberg] from comment #7)
> > @davehunt: Will it fail because switch_to_frame() doesn't work with nested
> > frames, or because get_active_frame() doesn't return the correct frame when
> > it is a nested frame? Either of those sound like marionette issues that
> > should be addressed (and maybe there is even an open bug for them).
>
> I don't recall ever looking into this myself, so :zac might be a better
> person to ask. I believe the issue is that get_active_frame will simply
> return the frame element, which may not be available from the top frame. You
> would somehow need to know a frame's ancestors and methodically switch
> through each of these.
Yes that's correct. This bump is the reason we have not used this technique sooner throughout gaiatest. Bob I recall you hitting this in bug 937336.
I'll have a look at the commit closer a bit later but as we can't switch into nested frames it seems we'll never entirely avoid some frame switching inside the test case, but we can at least have gaiatest do the majority of it automatically. It's a decision between allowing gaiatest to do some stuff automatically or leaving it entirely to be explicitly done by the test author.
Flags: needinfo?(zcampbell)
Reporter | ||
Updated•10 years ago
|
Priority: -- → P4
Reporter | ||
Comment 10•10 years ago
|
||
Comment on attachment 8418041 [details]
Link to a branch that shows decorator usage
Doesn't really seem that ground breaking but it looks OK, let's give it a shot!
f+
Attachment #8418041 -
Flags: feedback?(zcampbell) → feedback+
Reporter | ||
Comment 11•10 years ago
|
||
Comment on attachment 8418041 [details]
Link to a branch that shows decorator usage
hmmm, one thing that I've just discovered while using this is that when we split GaiaDevice, GaiaApps etc out into separate files we'll need this class in each of the files.
Bob given this context, does it change the way you would implement it?
Flags: needinfo?(bob.silverberg)
Reporter | ||
Comment 12•10 years ago
|
||
Attachment #8416503 -
Attachment is obsolete: true
Attachment #8421815 -
Flags: feedback?(bob.silverberg)
Comment 13•10 years ago
|
||
(In reply to Zac C (:zac) from comment #11)
> Comment on attachment 8418041 [details]
> Link to a branch that shows decorator usage
>
> hmmm, one thing that I've just discovered while using this is that when we
> split GaiaDevice, GaiaApps etc out into separate files we'll need this class
> in each of the files.
> Bob given this context, does it change the way you would implement it?
I still think the implementation would be similar, but we'd have to decide how to package it. For example, we could have the decorator in a file all by itself (e.g., `needs_system_frame_decorator.py`) and then simply import it into any of the other files that need it (i.e., `from .need_system_frame_decorator import needs_system_frame).
Flags: needinfo?(bob.silverberg)
Reporter | ||
Comment 14•10 years ago
|
||
Thanks Bob,
I'm still having some problems - can you have a look at the Travis job to see the error?
Flags: needinfo?(bob.silverberg)
Comment 15•10 years ago
|
||
:zac I had to change some things. Please see the PR for comments and my suggested changes.
Flags: needinfo?(bob.silverberg)
Reporter | ||
Updated•10 years ago
|
Assignee: zcampbell → nobody
Reporter | ||
Updated•10 years ago
|
Attachment #8421815 -
Attachment is obsolete: true
Attachment #8421815 -
Flags: feedback?(bob.silverberg)
Reporter | ||
Comment 16•10 years ago
|
||
Not really keen on importing another package just for this.
Rob if you need to switch to frame in some methods let's just do it on a case-by-case basis.
Reporter | ||
Comment 17•10 years ago
|
||
We could run into problems when we split these files out so will make this depending on the gaiatest packaging.
Depends on: 994176
Reporter | ||
Comment 18•10 years ago
|
||
As per comment #16, let's do this in case-by-case basis.
In this case running without --restart, using set_permission at the start of a test will run it in the context of the homescreen instead of the system frame.
Attachment #8447090 -
Flags: review?(robert.chira)
Comment 19•10 years ago
|
||
Comment on attachment 8447090 [details]
github pr
r+, waiting for Travis before merging.
Attachment #8447090 -
Flags: review?(robert.chira) → review+
Comment 20•10 years ago
|
||
Comment on attachment 8447090 [details]
github pr
It seems there where some issue in the TBPL run with the lockscreen tests. This requires further investigation.
Attachment #8447090 -
Flags: review?(robert.chira)
Attachment #8447090 -
Flags: review?(florin.strugariu)
Attachment #8447090 -
Flags: review+
Updated•10 years ago
|
Attachment #8447090 -
Flags: review?(florin.strugariu) → review+
Reporter | ||
Comment 21•10 years ago
|
||
(In reply to Robert Chira [:RobertC] from comment #20)
> Comment on attachment 8447090 [details]
> github pr
>
> It seems there where some issue in the TBPL run with the lockscreen tests.
> This requires further investigation.
I think there were some problems in the tree at the time.
Re-triggers of the test show it coming up roses (green :P )
Updated•10 years ago
|
Attachment #8447090 -
Flags: review?(robert.chira) → review+
Comment 22•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•