Several functions in gaia_test.py switch context to the system app unnecessarily

RESOLVED WONTFIX

Status

Firefox OS
Gaia::UI Tests
RESOLVED WONTFIX
3 years ago
2 years ago

People

(Reporter: Martijn Wargers (dead), Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(URL)

(Reporter)

Description

3 years ago
While working on bug 1109203, I noticed there are several other functions in gaia_test.py that switch context to the system app, where you wouldn't expect it.
It's better that the context doesn't change, when it's not necessary.

I think get_permission, set_permission, set_permission_by_url, displayed_app, is_app_installed, running_apps, _get_pref, _set_pref, bluetooth_enable, bluetooth_disable, connect_to_cell_data, disable_cell_data, enable_wifi, disable_wifi, connect_to_wifi, forget_all_networks, is_wifi_connected, delete_all_sms, get_all_sms, send_sms and is_locked are cases where you wouldn't expect the context to be changed to the system app, but where it happens.
(Reporter)

Comment 1

3 years ago
Perhaps a bit unclear worded. Those functions switch context, otherwise they wouldn't work.
I propose to switch context to what it was before, because consumers of these functions shouldn't expect the context to be switched as a side-effect of these functions.
This has come up before. There's no existing reliable way to switch back to the correct frame. In some cases switching back to the displayed app is satisfactory, however if the test previously had the focus of a nested frame, you will have problems. It's also not possible to restore into a nested frame without also storing a reference to all parent frames. The current scenario of methods changing (and leaving) focus to the system app - although undesirable - was considered the best option. This comes with caution to the test writer, which perhaps should be emphasised (in documentation?)

Of course we could revisit this decision if there is a good proposal for a solution, however I suspect not a lot of time will be invested in this.
(Reporter)

Comment 3

3 years ago
Dave, can't we use self.marionette.get_active_frame() to keep a reference to the old frame and then switch back to it after we're done? (like I used in the patch in bug 1109203)
Flags: needinfo?(dave.hunt)
(In reply to Martijn Wargers [:mwargers] (QA) from comment #3)
> Dave, can't we use self.marionette.get_active_frame() to keep a reference to
> the old frame and then switch back to it after we're done? (like I used in
> the patch in bug 1109203)

Only if the active frame is not a nested one, otherwise you won't be able to switch directly to it from the system app frame - you'd first need to switch to it's ancestor available from the system app and then through each of it's ancestors until you can reach it.
Flags: needinfo?(dave.hunt)
(Reporter)

Comment 5

3 years ago
Hmm, that means self.marionette.switch_to_frame(random_frame) doesn't always work?
(Reporter)

Comment 6

3 years ago
(In reply to Martijn Wargers [:mwargers] (QA) from comment #5)
> Hmm, that means self.marionette.switch_to_frame(random_frame) doesn't always
> work?
davehunt: if the frame is not available in the current context you will get an exception

davehunt: mwargers: I think perhaps the most complete solution would be to identify not just the active frame but the route to that frame from top
davehunt: then that route can be repeated when switching back
davehunt: alternatively if we could find an alternative to switching to the system app that would work too
QA Whiteboard: [fxosqa-auto-backlog+]
(Reporter)

Comment 7

3 years ago
From https://github.com/mozilla-b2g/gaia/pull/29160/files#r27468877 :
"


This actually makes us less consistent. In some places we assume that we're in the correct frame, in other places we switch to the system frame but do not attempt to switch back, and here we switch to the system frame and attempt to switch back.

The problem with switching frame is that the test author is not likely to expect this, and may need to account for it. Whilst we can attempt to switch back to the displayed app, this is only an attempt to restore the original frame context - if we were previously in a nested frame, we're going to end up in a different frame. I would favour consistency, and we should either remove the dependency on being in the system frame (where possible), determine if there's a way to somehow fork a process to do the switch and carry out the script, or fall back to some consistent behaviour. Having a decorator for this sounds like a nice approach, and will keep our solution in a central location.

To be clear, I'm r+ this if it works and does not cause regressions, but would like to see a followup that improves our consistency and maintenance.

"
(Reporter)

Updated

3 years ago
See Also: → bug 1148267
(Reporter)

Updated

2 years ago
Assignee: martijn.martijn → nobody
(Reporter)

Comment 8

2 years ago
Marking WONTFIX, sorry for the bug spam. If somebody still wants to work on this, please file a new bug for it.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.