1028941 - Only kill ftu instead of all in setUp

Status: RESOLVED FIXED

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

Description

[Zac C (:zac)]

The Gaia team are getting more adventurous with open/background/partially open apps, for example OperatorVariant, Call screen, Search collections etc.

Let's experiment with adapting the GaiaTestCase setUp to only kill the FTU just to be sure we're not accidentally killing any important or newly added apps with kill_all.

Comment 1

[Zac C (:zac)]

Instead of kill_all, do [pseudo-code] "if FTU in self.apps.running_apps:  self.apps.kill(FTU)"

Comment 2

[Zac C (:zac)]

Or ignore comment #1, do whatever solution you feel is best. The kill method only accepts a GaiaApp object which limits it a bit.

Comment 3

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

The point of setUp is to restore to a known state. If we only kill FTU then we may leave applications open from a previous test, which could invalidate the current test.

Comment 4

[Zac C (:zac)]

Ok that's true. That workflow we can cover by keeping keep kill_all inside the cleanup_gaia method.

I am just wary of messing with Gaia too much. Especially as some of the new homescreen is integrated as always-running apps and it's hard to keep the killAll atom up to date with what's supposed to be open and what's not.

I wonder if there's a way to detect from the AppWindowManager whether the app should be kept alive or not.

Comment 5

[Zac C (:zac)]

Attachment: github pr

Comment 6

[Zac C (:zac)]

adhoc +smoketest run:
http://selenium.qa.mtv2.mozilla.com:8080/job/b2g.flame.mozilla-central.ui.adhoc/41/

Comment 7

[Florin Strugariu [:Bebe]]

Comment on attachment 8447104 [details] [review]
github pr

TBPL is still failing

Comment 8

[Zac C (:zac)]

Comment on attachment 8447104 [details] [review]
github pr

Ran into problems in the unit tests because now running_apps is returning the correct apps!

Dave and I also discussed making kill all only kill user-killable apps and I got this data from the StackManager (cards view).

There's now a disparity between the kill_all and what running_apps returns which might seem odd at first but makes sense once you understand the distinction between all apps and the user-killable apps.

I've updated the test_kill* unit tests to reflect this.

We still need running_apps to return all apps so that we can detect when the FTU is running.

Comment 9

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

Comment on attachment 8447104 [details] [review]
github pr

I think we need to be consistent in the atom when we're talking about all apps or only apps that can be closed by a user. At the moment getRunningApps, numRunningApps, and killAll are not consistent.

We could perhaps pass an optional boolean to the JavaScript functions to indicate if we want to include the apps that users can't close. Something like includeSystemApps, perhaps?

Also, requesting feedback from Alive on the safety of using window.wrappedJSObject.StackManager._stack

Comment 10

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

Comment on attachment 8447104 [details] [review]
github pr

I dare not to say this is correct or not.
The stack is not including:
* Homescreen
* Callscreen
* Secure camera app on Lockscreen

BTW why do you need to change this?
What's the benefit?

Comment 11

[Zac C (:zac)]

Alive, we want to exclude killing important System apps like Homescreen, callscreen, lockscreen, collections etc.

I became concerned that the test suite was too aggressive and may kill some important apps and then cause Gaia to misbehave.

However we still need to cleanup Gaia after tests so we need to know which apps are safe to kill and which ones are not. Gaia does know this because it is able to exclude some apps from the Cards View.

Comment 12

[Florin Strugariu [:Bebe]]

Code looks OK

And travis passes. I ran a addhoc  http://selenium.qa.mtv2.mozilla.com:8080/job/b2g.flame.mozilla-central.ui.adhoc/42

Comment 13

[Zac C (:zac)]

@Alive, I looked through AppWindowManager to find a property that defined whether it was user-killable or a protected system window but I could not find one. Does AWM not expose this information?

That's why I ended up using this stack array.

Comment 14

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

Zac, thanks for explanation.
I am a little confused why we don't just restart the b2g instance after each test.

Does comment 11 means if the test is to test homescreen then the homescreen app will remain its state until the tests are all ended?

Comment 15

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

(In reply to Zac C (:zac) from comment #13)
> @Alive, I looked through AppWindowManager to find a property that defined
> whether it was user-killable or a protected system window but I could not
> find one. Does AWM not expose this information?

Right, because this is not a feature requested before. There's no 'close all apps' UI to go.

Comment 16

[Zac C (:zac)]

(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #14)
> Zac, thanks for explanation.
> I am a little confused why we don't just restart the b2g instance after each
> test.
> 
90% of the time we do just restart, but some partners and non-functional test suites use this framework and they prefer not to restart for their particular test case.

> Does comment 11 means if the test is to test homescreen then the homescreen
> app will remain its state until the tests are all ended?

Yes, basically.

Thanks for your help, I'll tidy up this pull a bit when I get a chance.

Comment 17

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

Comment on attachment 8447104 [details] [review]
github pr

f+ if you use snapshot()

Comment 18

[Zac C (:zac)]

Comment on attachment 8447104 [details] [review]
github pr

re-r?

Comment 19

[Zac C (:zac)]

Also retriggered an adhoc
http://selenium.qa.mtv2.mozilla.com:8080/job/b2g.flame.mozilla-central.ui.adhoc/49/

Comment 20

[Zac C (:zac)]

I'll also add that I did experiment with adding a unit test for killAll including system apps but sometimes the homescreen did not re-start properly and I did not trust that it would be stable. I really think that killing system apps should not be used if it can be avoided, but I've included it as a capability in the atom for consistency.

Comment 21

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

Comment on attachment 8447104 [details] [review]
github pr

Sorry, I don't really have enough time before PTO to give this the attention it deserves. I had a quick look through and in general it looks good, however I'd prefer docstrings to be used for methods rather than line comments.

Also, this will have an effect on Eideticker, b2gperf, and probably other consumers. As far as I can tell FTU is considered a system app, and therefore will not be killed using kill_all(), which is currently expected. Is there a way to consider this a user killable app, as we know it's safe to kill?

Comment 22

[Zac C (:zac)]

Comment on attachment 8447104 [details] [review]
github pr

docstrings are easy to fix. 
I'll r? bsilverberg on it.

No huge rush Bob.

Comment 23

[Zac C (:zac)]

I've put kill FTU into the kill_all method so that consumers won't have to change their setUp.

Comment 24

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8447104 [details] [review]
github pr

A couple of code changes are required to make the code work. Other than that it looks good to me. I made a number of comments that can be taken or not.

Comment 25

[Zac C (:zac)]

Comment on attachment 8447104 [details] [review]
github pr

r? again

Comment 26

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8447104 [details] [review]
github pr

Looks good now. Just remove the one extraneous comment.

Comment 27

[Zac C (:zac)]

Merged:
https://github.com/mozilla-b2g/gaia/commit/ad5814b01d97ddeb5ff36c556972533c3cff0396

Comment 28

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

Uplifted to v2.0:
https://github.com/mozilla-b2g/gaia/commit/bcadbf5bff770e6e7034e056d46189b3d84b72f7