Closed Bug 1003788 Opened 11 years ago Closed 10 years ago

[marionette-apps] switchToApp method doesn't wait for the app is rendered on screen.

Categories

(Testing Graveyard :: JSMarionette, defect)

Product:
Testing Graveyard
Component:
JSMarionette
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(b2g-v2.0 fixed)

Status:
RESOLVED FIXED
Milestone:
2.0 S3 (6june)
Tracking Flags:
Tracking Status
b2g-v2.0 --- fixed

People

(Reporter: evanxd, Assigned: evanxd)

References

Details

(Whiteboard: [priority])

Attachments

(2 files, 1 obsolete file)

Link to Github pull-request: https://github.com/mozilla-b2g/marionette-apps/pull/30
(deleted), text/x-github-pull-request
jlal
: review+
alive
: feedback+
kgrandon
: feedback+
Details
PR for gaia-node-modules
(deleted), text/x-github-pull-request
Details
We should let `switchToApp` method wait for the app's frame is on the transition-state="opened" state. The state means the app is already rendered, and ready to be controlled by marionette. Currently we just check the frame element have the `displayed` attribute, see it at https://github.com/mozilla-b2g/marionette-apps/blob/master/lib/waitforapp.js#L42. It would cause issues, for example Bug 950673.
It is a rough patch to show the logic.b
Hi all, How do you guys think about this bug?
Flags: needinfo?(jlal)
Flags: needinfo?(gaye)
Flags: needinfo?(mike)
The patch of Bug 950673 delays animationend event until active app loaded. It causes some test cases to fail intermittently. We should change the logic in switchToApp to fix the issue. We might also might be change logic of some test cases.
Blocks: 1003795
Is there a spec somewhere for this transition-state property?
Flags: needinfo?(gaye)
Hi Alive, Could you answer gaye's question at Comment 4? Thanks.
Flags: needinfo?(alive)
http://alivedise.github.io/gaia-system-jsdoc/AppTransitionController.html Not spec-ed but explaining. We change the attribute in changeTransitionState function().
Flags: needinfo?(alive)
Blocks: 950673
Hi Gareth, I think we could also remove this line https://github.com/mozilla-b2g/marionette-apps/blob/master/lib/launch.js#L37. We don't need to wait for homescreen app here, right? For the python implementation at https://github.com/mozilla-b2g/gaia/blob/master/tests/atoms/gaia_apps.js#L232-L269, it launches apps without waiting homescreen app.
Flags: needinfo?(gaye)
How do you think we just remove the homescreen logic.
Assignee: nobody → evanxd
Whiteboard: [priority]
Target Milestone: --- → 1.5 S1 (9may)
After we fix this bug, we should find a long term solution to deal with changes of System app for the marionette-apps module.
Blocks a blocker.
blocking-b2g: --- → 1.4+
Hi Alive, Could you provide the bug number of app.launch queue things here? Thanks.
Flags: needinfo?(alive)
Comment on attachment 8417311 [details] Link to Github pull-request: https://github.com/mozilla-b2g/marionette-apps/pull/30 Hi Gareth, Could you help me to review the patch. We remove the async driver support for two reasons: 1. We don't do the async things for marionette tests in Gaia anymore. 2. We don't need to have sync and async version of the workaround at https://github.com/evanxd/marionette-apps/blob/13318c182ba2f1908bed165c1181583a5ba9cd64/lib/launch.js#L28-L43. Thanks.
Attachment #8417311 - Flags: review?(gaye)
Comment on attachment 8417311 [details] Link to Github pull-request: https://github.com/mozilla-b2g/marionette-apps/pull/30 Hi Alive, Could you give feedback for the System app logic at https://github.com/evanxd/marionette-apps/blob/bug-1003788/lib/waitforapp.js#L34-L63? It would be helpful for the review process. Thanks.
Attachment #8417311 - Flags: feedback?(alive)
Comment on attachment 8417311 [details] Link to Github pull-request: https://github.com/mozilla-b2g/marionette-apps/pull/30 Yes, thank you very much.
Attachment #8417311 - Flags: feedback?(alive) → feedback+
Comment on attachment 8417311 [details] Link to Github pull-request: https://github.com/mozilla-b2g/marionette-apps/pull/30 James expressed some interest in taking a look!
Attachment #8417311 - Flags: review?(gaye) → review?(jlal)
Flags: needinfo?(gaye)
Clearing the blocking flag - we don't block on automation enhancements, as they not part of the build.
blocking-b2g: 1.4+ → ---
Hi Jason, This bug is blocking a 1.4+ bug(Bug 950673).
(In reply to Evan Tseng [:evanxd][:愛聞插低] from comment #19) > Hi Jason, > This bug is blocking a 1.4+ bug(Bug 950673). Right, although this is technically NPOTB, so we don't need to track this under the blocking b2g flag.
Got you. Thanks.
Hi James, Could you have time to take a look for review? This bug is blocking a 1.4+ bug(Bug 950673). Thanks.
(In reply to Jason Smith [:jsmith] from comment #20) > (In reply to Evan Tseng [:evanxd][:愛聞插低] from comment #19) > > Hi Jason, > > This bug is blocking a 1.4+ bug(Bug 950673). > > Right, although this is technically NPOTB, so we don't need to track this > under the blocking b2g flag. This is not an enhancement. Once bug 950673 landed we will need this to fix plenty testt failures(but not real fail because it's a test framework design issue.)
Comment on attachment 8417311 [details] Link to Github pull-request: https://github.com/mozilla-b2g/marionette-apps/pull/30 Hi Kevin, Could you have time to take a look? Thanks.
Attachment #8417311 - Flags: review?(kgrandon)
Flags: needinfo?(mike)
Flags: needinfo?(jlal)
Comment on attachment 8417311 [details] Link to Github pull-request: https://github.com/mozilla-b2g/marionette-apps/pull/30 So I haven't tested the code or anything, but it looks relatively sane to me. James - could you do the final review?
Attachment #8417311 - Flags: review?(kgrandon) → feedback+
Hi James, Could you have time to take a look? Thanks.
Flags: needinfo?(jlal)
Blocks: 994017
Target Milestone: 1.5 S1 (9may) → 1.5 S2 (23may)
I'd like to add here that this issue is hanging up a *bunch* of perf regression fixes. When this lands, we can then land the fix for Bug 950673 which is also a possible fix for Bug 987994.
Blocks: 987994
Hi James, I updated the patch for you comments. Thanks.
Hi Kevin, How do you think about https://github.com/mozilla-b2g/marionette-apps/pull/30/files#r12625589? Some failures will be happened after we remove the waiting homescreen logic.
Flags: needinfo?(kgrandon)
I don't why the URL could not show correctly here. We could try this URL https://github.com/mozilla-b2g/marionette-apps/pull/30/files#diff-8e402bf78d4c5f8a4fb797709e5bc7a3R33.
Yes, it works. Please check it out. Thanks.
I responded on github. I think the ni? was just for the homescreen setting? Let's track this in bug 1007352.
Flags: needinfo?(kgrandon)
Tanks, Kevin.
Hi James, How do you think about the comments https://github.com/mozilla-b2g/marionette-apps/pull/30/files#diff-b55e0795ab38b3bb56061e860c4d3ec8R44?
Flags: needinfo?(jlal)
Blocks: 1007352
Flags: needinfo?(jlal)
Tests pass so I am going to land/unblock this: https://github.com/evanxd/marionette-apps/commit/6d7e702d748261939c64dfec103138706dbff7b3 ^ I am not 100% sure about the approach here tying even more closely in with the nonstandard aspects of the system app but that seems inevitable right now.
Flags: needinfo?(jlal)
Attachment #8417311 - Flags: review?(jlal) → review+
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Thanks, James. And we should find a standard approach without the System app dependencies in the future.
Attached file PR for gaia-node-modules (deleted) —
FYI - I backed this out of gaia-node-modules only to keep the package.json inline with gaia master. Please re-land whenever we are ready to update gaia master again. Sorry about that.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Hi Kevin, Thanks for the help. I think we should re-land the patch now, and change the revision in gaia to skip more wrong usage of switchToApp method. Thanks.
Hi Kevin, How do you think about Comment 41?
Flags: needinfo?(kgrandon)
Sure, my assumption was that this could only land in master with disabling or fixing of tests. I'm fine with this going into gaia-node-modules whenever it's ready to go into master.
Flags: needinfo?(kgrandon)
No longer blocks: 1003795
Depends on: 1003795
Target Milestone: 2.0 S2 (23may) → 2.0 S3 (6june)
It is ready to go into gaia master. See the travis job[1]. [1] https://travis-ci.org/mozilla-b2g/gaia/builds/26279453
Status: REOPENED → RESOLVED
Closed: 11 years ago → 10 years ago
Resolution: --- → FIXED
Mass modify - set status-b2g-v2.0 fixed for fixed bugs under vertical homescreen dependency tree.
status-b2g-v2.0: --- → fixed
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: