Closed
Bug 933590
Opened 11 years ago
Closed 11 years ago
[Window Management] Remove 'WindowManager' reference in gaia-ui-tests, update to latest API or do not rely on it.
Categories
(Firefox OS Graveyard :: Gaia::UI Tests, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: alive, Unassigned)
References
Details
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;
Updated•11 years ago
|
Component: Gaia::System::Window Mgmt → Gaia::UI Tests
Comment 1•11 years ago
|
||
Alive: With what should WindowManager be replaced in the above code?
Flags: needinfo?(alive)
Reporter | ||
Comment 2•11 years ago
|
||
I will update here when the change finally confirms.
Flags: needinfo?(alive)
Reporter | ||
Updated•11 years ago
|
Blocks: task-manager
Reporter | ||
Updated•11 years ago
|
No longer blocks: task-manager
Reporter | ||
Comment 3•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
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.
Reporter | ||
Comment 6•11 years ago
|
||
(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?
Reporter | ||
Comment 7•11 years ago
|
||
BTW thanks for you guys' quick response!!
Comment 8•11 years ago
|
||
(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•11 years ago
|
||
I'll experiment with "div.appWindow.active" then as it correlates with another task I'm working on now too.
Reporter | ||
Comment 10•11 years ago
|
||
(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•11 years ago
|
||
(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•11 years ago
|
||
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?
Flags: needinfo?(alive)
Reporter | ||
Comment 13•11 years ago
|
||
(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?
Flags: needinfo?(alive)
Reporter | ||
Comment 14•11 years ago
|
||
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 16•11 years ago
|
||
For the time being we would just use data-manifest-name. I can work with that when it's merged.
Flags: needinfo?(zcampbell)
Reporter | ||
Comment 17•11 years ago
|
||
Make meta clear
No longer blocks: window-management
Depends on: app-window-manager
Comment 18•11 years ago
|
||
We're now safely using the new AppWindowManager with a side-plan to move more of this to a complete Marionette solution (bug #936010)
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•