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

Status: RESOLVED FIXED

(Testing Graveyard :: JSMarionette, defect)

Description

[Evan Tseng [:evanxd]]

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.

Comment 1

[Evan Tseng [:evanxd]]

Attachment: Rough patch: https://github.com/mozilla-b2g/marionette-apps/pull/29

It is a rough patch to show the logic.b

Comment 2

[Evan Tseng [:evanxd]]

Hi all,

How do you guys think about this bug?

Comment 3

[Evan Tseng [:evanxd]]

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.

Comment 4

[Gareth Aye [:gaye] (back from PTO)]

Is there a spec somewhere for this transition-state property?

Comment 5

[Evan Tseng [:evanxd]]

Hi Alive,

Could you answer gaye's question at Comment 4?

Thanks.

Comment 6

[(inactive after 6/18) Alive Kuo [:alive]]

http://alivedise.github.io/gaia-system-jsdoc/AppTransitionController.html
Not spec-ed but explaining. We change the attribute in changeTransitionState function().

Comment 7

[Evan Tseng [:evanxd]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/marionette-apps/pull/30

Comment 8

[Evan Tseng [:evanxd]]

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.

Comment 9

[Evan Tseng [:evanxd]]

How do you think we just remove the homescreen logic.

Comment 10

[Evan Tseng [:evanxd]]

After we fix this bug, we should find a long term solution to deal with changes of System app for the marionette-apps module.

Comment 11

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Blocks a blocker.

Comment 12

[Evan Tseng [:evanxd]]

Hi Alive,

Could you provide the bug number of app.launch queue things here?

Thanks.

Comment 13

[(inactive after 6/18) Alive Kuo [:alive]]

https://bugzilla.mozilla.org/show_bug.cgi?id=999574

Comment 14

[Evan Tseng [:evanxd]]

Comment on attachment 8417311 [details] [review]
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.

Comment 15

[Evan Tseng [:evanxd]]

Comment on attachment 8417311 [details] [review]
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.

Comment 16

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8417311 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/marionette-apps/pull/30

Yes, thank you very much.

Comment 17

[Gareth Aye [:gaye] (back from PTO)]

Comment on attachment 8417311 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/marionette-apps/pull/30

James expressed some interest in taking a look!

Comment 18

[Jason Smith [:jsmith]]

Clearing the blocking flag - we don't block on automation enhancements, as they not part of the build.

Comment 19

[Evan Tseng [:evanxd]]

Hi Jason,
This bug is blocking a 1.4+ bug(Bug 950673).

Comment 20

[Jason Smith [:jsmith]]

(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.

Comment 21

[Evan Tseng [:evanxd]]

Got you.
Thanks.

Comment 22

[Evan Tseng [:evanxd]]

Hi James,

Could you have time to take a look for review?
This bug is blocking a 1.4+ bug(Bug 950673).

Thanks.

Comment 23

[(inactive after 6/18) Alive Kuo [:alive]]

(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 24

[Evan Tseng [:evanxd]]

Comment on attachment 8417311 [details] [review]
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.

Comment 25

[Kevin Grandon :kgrandon]

Comment on attachment 8417311 [details] [review]
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?

Comment 26

[Evan Tseng [:evanxd]]

Hi James,

Could you have time to take a look?

Thanks.

Comment 27

[Dave Huseby [:huseby]]

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.

Comment 28

[Evan Tseng [:evanxd]]

Hi James,

I updated the patch for you comments.

Thanks.

Comment 29

[Evan Tseng [:evanxd]]

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.

Comment 30

[Evan Tseng [:evanxd]]

https://github.com/mozilla-b2g/marionette-apps/pull/30/files#r12625589

Comment 31

[Evan Tseng [:evanxd]]

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.

Comment 32

[Evan Tseng [:evanxd]]

Yes, it works. Please check it out.
Thanks.

Comment 33

[Kevin Grandon :kgrandon]

I responded on github. I think the ni? was just for the homescreen setting? Let's track this in bug 1007352.

Comment 34

[Evan Tseng [:evanxd]]

Tanks, Kevin.

Comment 35

[Evan Tseng [:evanxd]]

Hi James,

How do you think about the comments https://github.com/mozilla-b2g/marionette-apps/pull/30/files#diff-b55e0795ab38b3bb56061e860c4d3ec8R44?

Comment 36

[James Lal [:lightsofapollo]]

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.

Comment 37

[Evan Tseng [:evanxd]]

Thanks, James.
And we should find a standard approach without the System app dependencies in the future.

Comment 38

[Evan Tseng [:evanxd]]

Attachment: PR for gaia-node-modules

Comment 39

[Kevin Grandon :kgrandon]

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.

Comment 40

[Kevin Grandon :kgrandon]

https://github.com/mozilla-b2g/gaia-node-modules/commit/9c5d6338fb2dcda83115cbf423a478dbb2a32d95

Comment 41

[Evan Tseng [:evanxd]]

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.

Comment 42

[Evan Tseng [:evanxd]]

Hi Kevin,
How do you think about Comment 41?

Comment 43

[Kevin Grandon :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.

Comment 44

[Evan Tseng [:evanxd]]

It is ready to go into gaia master. See the travis job[1].

[1] https://travis-ci.org/mozilla-b2g/gaia/builds/26279453

Comment 45

[Kevin Grandon :kgrandon]

Mass modify - set status-b2g-v2.0 fixed for fixed bugs under vertical homescreen dependency tree.