1027144 - Re-enable test_homescreen_move_app.py, test_homescreen_delete_app.py

Status: RESOLVED FIXED

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

Description

[Zac C (:zac)]

Over the last weekend changes were made to the Homescreen that regressed the testability.

Instead of this being resolved, tests were disabled.

The devs have described the problem as "In gaia_dragdrop, in the contextmenu event handler we don't handle it if the transition is still ending - this will then bubble up to the app and it will treat it as a long-press on the wallpaper."

It needs to be resolved because it is blocking P1 QA Smoketest coverage.


Rough regression range:

Passing:
Gaia revision 	90777363ed0a
Gecko build 	20140612160203
Gecko revision 	aab3362f97e9
Gecko version 	33.0a1


Failing:
Gaia      dfc4703bb81d1fa4f2087a1a6124b47a80a5d1de
Gecko     https://hg.mozilla.org/mozilla-central/rev/80431d4fd0da
BuildID   20140616040202
Version   33.0a1

Comment 1

[Kevin Grandon :kgrandon]

I think we should wait until transitionend before starting a new drag operation. The gaia fix will likely be to prevent the contextmenu while a transitionend is in progress - though a user will probably never run into this.

Also FWIW we do already have test coverage for this in the app. It's a unit test currently, and we're also working on JS marionette tests for this.

Comment 2

[Jason Smith [:jsmith]]

QA blocker - needs to block the release.

Comment 3

[Jason Smith [:jsmith]]

This impacts 2.0 coverage right? Not just 2.1?

Comment 4

[Zac C (:zac)]

I haven't checked yet. We're enabling vertical hs tests on v2.0 tomorrow so we'll know then. Keeping ni? until then.

Comment 5

[Zac C (:zac)]

(In reply to Kevin Grandon :kgrandon from comment #1)
> I think we should wait until transitionend before starting a new drag
> operation. The gaia fix will likely be to prevent the contextmenu while a
> transitionend is in progress - though a user will probably never run into
> this.

I did try to put some generous waits into the test to wait for edit mode to settle and stabilise but it did not resolve it. but I agree it's a Marionette only problem, I could not replicate it manually.

Comment 6

[Jason Smith [:jsmith]]

Will wait on a 2.0 nom until I get confirmation that this happens on 2.0.

Comment 7

[Jason Smith [:jsmith]]

Confirmed this test got shut off on 2.0 as well.

Comment 8

[Kevin Grandon :kgrandon]

I understand the desire to have these tests and we really want them as well, but how does this constitute a FL blocker? I understand the value in tests and they are really nice, but to me it doesn't seem like they should necessarily block the feature from landing. We have coverage for this functionality, and we will work on getting this working again for device coverage.

Can we remove FL blocking and keep FC blocking?

Comment 9

[Jason Smith [:jsmith]]

(In reply to Kevin Grandon :kgrandon from comment #8)
> I understand the desire to have these tests and we really want them as well,
> but how does this constitute a FL blocker? I understand the value in tests
> and they are really nice, but to me it doesn't seem like they should
> necessarily block the feature from landing. We have coverage for this
> functionality, and we will work on getting this working again for device
> coverage.
> 
> Can we remove FL blocking and keep FC blocking?

I don't think so. This is caused QA to lose smoketest coverage, so we need this test turned on asap.

Comment 10

[Kevin Grandon :kgrandon]

We will turn this on asap, but I think you are abusing the feature landing flag for this. Feature landing === feature work.

Comment 11

[Jason Smith [:jsmith]]

(In reply to Kevin Grandon :kgrandon from comment #10)
> We will turn this on asap, but I think you are abusing the feature landing
> flag for this. Feature landing === feature work.

No, that's wrong. Feature landing means:

* No smoketest blockers
* No automation coverage lost due to landing of a feature (this is what is causing this to block)
* No missing use cases

Comment 12

[Kevin Grandon :kgrandon]

Renaming this because we can't change anything about gaia to support this, we need to fix the waits in the test. I'm looking into it now.

Comment 13

[Kevin Grandon :kgrandon]

Attachment: Github pull request

Hey guys - anyone have time for a review?

Comment 14

[Jason Smith [:jsmith]]

I'll pull blocking flag since now I understand this doesn't require a gaia change. Still need to block FL though for the reasons mentioned above.

Comment 15

[Zac C (:zac)]

Comment on attachment 8443076 [details] [review]
Github pull request

Bebe is off today.

r+, It works very well. Thanks a lot Kevin, you're a true gentleman!

Comment 16

[Zac C (:zac)]

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

Comment 17

[Zac C (:zac)]

I'll trial this on v2.0

Comment 18

[Zac C (:zac)]

This doesn't go straight onto v2.0, there must be some vertical hs commits waiting to be uplifted to v2.0.  (or at least on the build I was using)

Comment 19

[Kevin Grandon :kgrandon]

Thanks Zac. 

I uplifted a lot of commits last night and I'm generally doing everything in resolution order. It's best if we mark this as resolved so it goes in landing order, and it should be fine to uplift now - I'll follow through on it. Let me know if you have any objections to that.

Comment 20

[Kevin Grandon :kgrandon]

Comment on attachment 8443076 [details] [review]
Github pull request

This is a test and is needed to ensure we don't regress the vertical homescreen in 2.0. Not part of the build.

Comment 21

[Kevin Grandon :kgrandon]

2.0: https://github.com/mozilla-b2g/gaia/commit/d7fd03c94aaf0791ba4c68753230a08b4ed050cd

Comment 22

[Ed Morley [:emorley]]

Reverted for intermittent test failures:
https://tbpl.mozilla.org/php/getParsedLog.php?id=42273009&tree=B2g-Inbound

https://github.com/mozilla-b2g/gaia/commit/a0807151b0b28feb59b323a30b1f74dc8801be32

Comment 23

[Zac C (:zac)]

Reckon this is actually a functional regression introduced recently.

Comment 24

[Jason Smith [:jsmith]]

Ed - Are you sure you backed out the right patch here? I would expect a Gaia patch to be backed out here that caused the functional regression, not the test.

Comment 25

[Ed Morley [:emorley]]

(In reply to Jason Smith [:jsmith] from comment #24)
> Ed - Are you sure you backed out the right patch here? I would expect a Gaia
> patch to be backed out here that caused the functional regression, not the
> test.

Yes I'm sure.
The test was flaky, was supposed to be fixed, and is still flaky, hence the fixing and re-enablement of the test being reverted.
Note comment 22 said /intermittent/ rather than permanently failing & also bear in mind this landed late on friday, and not as many jobs run (or are starred) over the weekend, so this test wasn't necessarily "fine" and then regressed later by something else.

Comment 26

[Ed Morley [:emorley]]

The uplift also turned gaia-ui-tests perma-orange on Aurora:
https://tbpl.mozilla.org/?tree=Mozilla-Aurora&jobname=gaia-ui

Please can you back it out there?

Comment 27

[Ed Morley [:emorley]]

Another on trunk:
https://tbpl.mozilla.org/php/getParsedLog.php?id=42276928&tree=Mozilla-Inbound

Comment 28

[Kevin Grandon :kgrandon]

We were tracking the perma-orange in bug 1028640, which was due to waiting on bug 962645 to be uplifted.

I would like the tests to maintain the same state across the branch, so going to perform a backout on 2.0 while we investigate.

https://github.com/mozilla-b2g/gaia/commit/112b26d38f9ef91ca00d435055f5d28267423e53

Comment 29

[Askeing Yen[:askeing][:fyen]]

Add a wait before "first_app_before_move = self.homescreen.visible_apps[0].name" can fix the "IndexError: list index out of range" issue

https://github.com/mozilla-b2g/gaia/pull/20920

(In reply to Ed Morley [:edmorley UTC+0] from comment #27)
> Another on trunk:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=42276928&tree=Mozilla-
> Inbound

Comment 30

[Zac C (:zac)]

Attachment: github pr

On behalf of Askeing.

Triggered it a few times to validate its stability:
https://tbpl.mozilla.org/?tree=Gaia-Try&rev=a431b50a07c2a30a4ea5c815cd3c27fb4b811399

Comment 31

[Ed Morley [:emorley]]

(In reply to Zac C (:zac) from comment #30)
> Triggered it a few times to validate its stability:
> https://tbpl.mozilla.org/?tree=Gaia-
> Try&rev=a431b50a07c2a30a4ea5c815cd3c27fb4b811399

Thanks for pushing to try :-) We'll need to run it something like 100 times to know for sure; have requested some additional retriggers.

Comment 32

[Jason Smith [:jsmith]]

Clearing the feature-b2g flag - this isn't feature work, but rather automation coverage lost against the FL criteria QA has set.

Comment 33

[Zac C (:zac)]

I've disabled and fixed the disabled messages for the two failing tests.

Also gave this bug the dependency upon the homescreen fix.

master
https://github.com/mozilla-b2g/gaia/commit/5ab7b1ce3144eb399575a76b58d2600b34d02411
v2.0
https://github.com/mozilla-b2g/gaia/commit/d815bfbb70d0f4a3757bb9eb93956a59a103d181

Comment 34

[Ed Morley [:emorley]]

(In reply to Ed Morley [:edmorley UTC+0] from comment #31)
> (In reply to Zac C (:zac) from comment #30)
> > Triggered it a few times to validate its stability:
> > https://tbpl.mozilla.org/?tree=Gaia-
> > Try&rev=a431b50a07c2a30a4ea5c815cd3c27fb4b811399
> 
> Thanks for pushing to try :-) We'll need to run it something like 100 times
> to know for sure; have requested some additional retriggers.

This found a failure:
https://tbpl.mozilla.org/php/getParsedLog.php?id=42455542&tree=Gaia-Try
 TEST-UNEXPECTED-FAIL | test_homescreen_move_app.py test_homescreen_move_app.TestMoveApp.test_move_app_position | AssertionError: u'Phone' != u'FM Radio'

Comment 35

[Zac C (:zac)]

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

This is not ready for review as the bug is blocked.

Comment 36

[Kevin Grandon :kgrandon]

Attachment: Github revert

Pull request with just the revert, try results starting to come in and look promising: https://tbpl.mozilla.org/?tree=Gaia-Try&rev=791b9aeda2d8071b638a8eb4f80211b38b6e9b77

Assuming we get a slew of greens, it should be fine to land this now that the dependent bug has landed.

Comment 37

[Kevin Grandon :kgrandon]

A few failures for clock, but the intermittent here seems to have gone away, landing..

Comment 38

[Kevin Grandon :kgrandon]

Landed: https://github.com/mozilla-b2g/gaia/commit/35ca14719bf894a3b57be5596bc8e14cd2336935

Comment 39

[Zac C (:zac)]

(In reply to Kevin Grandon :kgrandon from comment #37)
> A few failures for clock, but the intermittent here seems to have gone away,
> landing..

Damn, thought I resolved those last week :(

Thanks for re-enabling this test!

Comment 40

[Ryan VanderMeulen [:RyanVM]]

Kevin, does something need to be done on v2.0 here still?

Comment 41

[Kevin Grandon :kgrandon]

The test was turned off in 2.0, so I applied this patch to re-enable it now that the dependent bug has landed. A=test only.

Comment 42

[Viorela Ioia [:viorela]]

Attachment: Github PR: https://github.com/mozilla-b2g/gaia/pull/21188

Let's re-enable test_homescreen_delete_app.py too

Comment 43

[Zac C (:zac)]

Comment on attachment 8448040 [details] [review]
Github PR: https://github.com/mozilla-b2g/gaia/pull/21188

r+ thanks!

Comment 44

[Zac C (:zac)]

c#42 patch merged:
https://github.com/mozilla-b2g/gaia/commit/481bac2913becb06cc16ea0bc6a96cf6ad9c3b15

Comment 45

[Robert Chira [:RobertC]]

Attachment: Pull request v2.0

test_delete_app need to be re-enabled on v2.0

Comment 46

[Zac C (:zac)]

Re-enabled on v2.0:
https://github.com/mozilla-b2g/gaia/commit/69141339d27a9cb93bbaad84d3972e724c0f311d