933590 - [Window Management] Remove 'WindowManager' reference in gaia-ui-tests, update to latest API or do not rely on it.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::UI Tests, defect)

Description

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

WindowManager is going to be deprecated,
but there's still some codes in gaia uitest referring it.

gaiatest/atoms/gaia_apps.js:    let runningApps = window.wrappedJSObject.WindowManager.getRunningApps();
gaiatest/atoms/gaia_apps.js:    let runningApps = window.wrappedJSObject.WindowManager.getRunningApps();
gaiatest/atoms/gaia_apps.js:    let runningApps = window.wrappedJSObject.WindowManager.getRunningApps();
gaiatest/atoms/gaia_apps.js:    let runningApps = window.wrappedJSObject.WindowManager.getRunningApps();
gaiatest/atoms/gaia_apps.js:              window.wrappedJSObject.WindowManager.getRunningApps();
gaiatest/atoms/gaia_apps.js:      window.wrappedJSObject.WindowManager.kill(aOrigin);
gaiatest/atoms/gaia_apps.js:    let runningApps = window.wrappedJSObject.WindowManager.getRunningApps();
gaiatest/atoms/gaia_apps.js:        let windowManager = window.wrappedJSObject.WindowManager;
gaiatest/atoms/gaia_apps.js:    let windowManager = window.wrappedJSObject.WindowManager;

Comment 1

[Bob Silverberg [:bsilverberg]]

Alive: With what should WindowManager be replaced in the above code?

Comment 2

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

I will update here when the change finally confirms.

Comment 3

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

OK, I looked at some code in uitest.
Can't we just remove it? It's really killing me that I need to maintain the API for an user that is not at the same repo with me.
And the test is neither black box nor white box - if window manager changes it just fails.

My proposal:
* To launch an app, use mozApps API
* To locate an app, use iframe[mozapps=MANIFESTURL]
* To locate an active app, use div.appWindow.active

What's the remaining requirement? Please tell me and let's figure out a way not relying on any live JS Object.

Comment 4

[Zac C (:zac)]

Alive, sorry to be a pain but we are in the gaia tree. We are not a separate user. This stuff is the backbone of UI testing on Travis so it *is* important.
Also at least two of our partners use gaiatest and the test repository for their own Firefox OS testing so we have a further dependency, we can't just delete the stuff.

For separate reasons locate an app is being changed to use mozApps API already here:
https://bugzilla.mozilla.org/show_bug.cgi?id=924565 

We definitely still need to be able to kill an app: WindowManager.kill(aOrigin)

and WindowManager.getDisplayedApp() is very important to us in the test flow. We could probably replace that with div.appWindow.active but I'll have to check how well they match each other.

Comment 5

[Dave Hunt [:davehunt] [he/him] ⌚BST]

I've raised bug 935423 to remove the gaia-ui-tests repository as it's no longer used and still causing confusion.

Alive: This is in the gaia repository at https://github.com/mozilla-b2g/gaia/blob/3b5fe429f2414f2a9d7241b311b399033bb10612/tests/python/gaia-ui-tests/gaiatest/atoms/gaia_apps.js

I would be happy to review a patch that either modified that JS file or the Python class that uses it, which is here: https://github.com/mozilla-b2g/gaia/blob/3b5fe429f2414f2a9d7241b311b399033bb10612/tests/python/gaia-ui-tests/gaiatest/gaia_test.py#L56

Any changes would need a large amount of testing, not just the tests in the gaia repo, but any Python packages that depend on gaiatest. This includes Eideticker, b2gpopulate, and b2gperf.

Comment 6

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

(In reply to Zac C (:zac) from comment #4)
> Alive, sorry to be a pain but we are in the gaia tree. We are not a separate
> user. This stuff is the backbone of UI testing on Travis so it *is*
> important.
> Also at least two of our partners use gaiatest and the test repository for
> their own Firefox OS testing so we have a further dependency, we can't just
> delete the stuff.
> 
> For separate reasons locate an app is being changed to use mozApps API
> already here:
> https://bugzilla.mozilla.org/show_bug.cgi?id=924565 

Good news.

> 
> We definitely still need to be able to kill an app:
> WindowManager.kill(aOrigin)

Hmm, lemme me think if we have alternative.
But this one is simple to keep, I think.

> 
> and WindowManager.getDisplayedApp() is very important to us in the test
> flow. We could probably replace that with div.appWindow.active but I'll have
> to check how well they match each other.

Let's try active to address displayedApp then.

(In reply to Dave Hunt (:davehunt) from comment #5)
> I've raised bug 935423 to remove the gaia-ui-tests repository as it's no
> longer used and still causing confusion.
> 
> Alive: This is in the gaia repository at
> https://github.com/mozilla-b2g/gaia/blob/
> 3b5fe429f2414f2a9d7241b311b399033bb10612/tests/python/gaia-ui-tests/gaiatest/
> atoms/gaia_apps.js
> 
> I would be happy to review a patch that either modified that JS file or the
> Python class that uses it, which is here:
> https://github.com/mozilla-b2g/gaia/blob/
> 3b5fe429f2414f2a9d7241b311b399033bb10612/tests/python/gaia-ui-tests/gaiatest/
> gaia_test.py#L56
> 
> Any changes would need a large amount of testing, not just the tests in the
> gaia repo, but any Python packages that depend on gaiatest. This includes
> Eideticker, b2gpopulate, and b2gperf.

Fine, then that means I may be able to fix uitest in my bug as well as the gaia patch.
Does the test you mentioned is running on travis now? I guess they don't be there?
How to ensure they work fine?

Comment 7

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

BTW thanks for you guys' quick response!!

Comment 8

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Alive Kuo [:alive][11/4-11/8 at SFO ww.] from comment #6)
> > Any changes would need a large amount of testing, not just the tests in the
> > gaia repo, but any Python packages that depend on gaiatest. This includes
> > Eideticker, b2gpopulate, and b2gperf.
> 
> Fine, then that means I may be able to fix uitest in my bug as well as the
> gaia patch.
> Does the test you mentioned is running on travis now? I guess they don't be
> there?
> How to ensure they work fine?

They're not running in Travis, no.. and they are pinned to specific releases of gaiatest. Any changes would ultimately result in a new gaiatest release, and we may want to update the dependencies of these packages. Once you have a patch I'd be happy to test them to see if there are any issues.

Note that bug 924565 has a patch for the apps JavaScript code, which is pending review.

Comment 9

[Zac C (:zac)]

I'll experiment with "div.appWindow.active" then as it correlates with another task I'm working on now too.

Comment 10

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

(In reply to Zac C (:zac) from comment #9)
> I'll experiment with "div.appWindow.active" then as it correlates with
> another task I'm working on now too.

More info:
Exactly, the displayedApp in window manager is updated once any app to app switch is happening.
And the active class in the end would be added to the app frame that is foreground. That means you might see more than one apps are having active class but in the long run there shall be only one. You could also check 'opening' 'closing' class to know they're having transition now.

If what you need is to know the name of the app, I may be also to add a "name" field on the app frame attribute.

Comment 11

[Zac C (:zac)]

(In reply to Alive Kuo [:alive][11/4-11/8 at SFO ww.] from comment #10)
> (In reply to Zac C (:zac) from comment #9)
> > I'll experiment with "div.appWindow.active" then as it correlates with
> > another task I'm working on now too.
> 
> More info:
> Exactly, the displayedApp in window manager is updated once any app to app
> switch is happening.
> And the active class in the end would be added to the app frame that is
> foreground. That means you might see more than one apps are having active
> class but in the long run there shall be only one. You could also check
> 'opening' 'closing' class to know they're having transition now.
> 
> If what you need is to know the name of the app, I may be also to add a
> "name" field on the app frame attribute.

Cool, reckon I can do all that in Python with the marionette client!

Comment 12

[Zac C (:zac)]

Alive, I'm just experimenting with this right now. It is working well with minimal effort and I think it will work fine.

(In reply to Alive Kuo [:alive][NEEDINFO] from comment #10)
> If what you need is to know the name of the app, I may be also to add a
> "name" field on the app frame attribute.

This would definitely help us a lot in terms of not having to migrate the logic inside tests where we wait explicitly for the displayed app to be a name.
There is already a name attribute but it is set to "main" for every frame so no good to us now. 
Would it have to be done in this pull request or separately?

Comment 13

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

(In reply to Zac C (:zac) from comment #12)
> Alive, I'm just experimenting with this right now. It is working well with
> minimal effort and I think it will work fine.
> 
> (In reply to Alive Kuo [:alive][NEEDINFO] from comment #10)
> > If what you need is to know the name of the app, I may be also to add a
> > "name" field on the app frame attribute.
> 
> This would definitely help us a lot in terms of not having to migrate the
> logic inside tests where we wait explicitly for the displayed app to be a
> name.
> There is already a name attribute but it is set to "main" for every frame so
> no good to us now. 
> Would it have to be done in this pull request or separately?

The name on the iframe is totally for other usage, don't use that. It's for window.open's second argument usage.
Yes, I will add it on the frame(div.appWindow) but I'm not sure if you want a localized name or non-localized name here? Or both?

Comment 14

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

Hi, you would have

div.appWindow[data-manifest-name="XXXX"]
div.appWindow[data-localized-name="YYYY"]

when bug 907013 is landed.
The later one would change whenever system locale is changed but the first one is always the default name in the manifest.
Give me some feedback please.

Comment 15

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

See last comment.

Comment 16

[Zac C (:zac)]

For the time being we would just use data-manifest-name. I can work with that when it's merged.

Comment 17

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

Make meta clear

Comment 18

[Zac C (:zac)]

We're now safely using the new AppWindowManager with a side-plan to move more of this to a complete Marionette solution (bug #936010)