1181201 - ftu/test/unit/tutorial_test.js went permafail when bug 1143575 landed

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::First Time Experience, defect, P2)

Description

[Ryan VanderMeulen [:RyanVM]]

Bug 1143575 has some discussion:

(In reply to Julien Wajsberg [:julienw] from bug 1143575 comment #325)
> Hi,
> 
> in B2G Desktop (I haven't tried B2G) we don't get the "error" event in case
> a video does not load.
> 
> Testcase: https://everlong.org/mozilla/test-video-error.html

(In reply to Julien Wajsberg [:julienw] from bug 1143575 comment #326)
> Mmm sorry, it looks like I don't get the error event in previous versions
> either. Still investigating why this makes bug 1108507 not intermittent
> then...

(In reply to Julien Wajsberg [:julienw] from bug 1143575 comment #327)
> I don't know yet if this is related to the issue we have in the unit tests,
> but I noticed that we have an invalidation problem.
> 
> For example load any video inside Firefox or B2G with your patch:
> http://techslides.com/demos/sample-videos/small.webm
> 
> The video does not update properly with the patch. It updates if I move the
> mouse cursor on top of the video.
> 
> (I'm on Linux, in case this is useful).

From IRC:

sfoster: right because that's what that test is testing - the ability to catch the load event on the mp4. If the mp4 fails to load due to some platform issue, it will fail
sfoster: hrm, so I could probably mock the <video> element so the test is testing the FTU functionality, not the ability of the platform to load/decode video. From the point of view of these *unit* tests that's the right thing to do. But it is also catching a real problem here
sfoster: RyanVM|sheriffduty: so we could disable the tutorial test until I can do this <video> mocking. But lets be sure there's adequate coverage in gecko as that test is the canary in the mine right now

So per the IRC discussion and given that it was a 66-rev patchset that caused it, I'm going to disable the test for now in lieu of backing out (though I'm not horribly happy about it). I'm worried based on the information above that we're wallpapering over a legit problem here by ignoring the test failure.

Note that the Try pushes for bug 1143575 also showed these failures, but buried under the blue auto-retries that our Gaia test harnesses use to pretend that they don't fail as often as they do.

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Test disabled:
https://github.com/mozilla-b2g/gaia/commit/86af137c7f4c41c1185e609339002ab47fd6c640

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #0)
> (In reply to Julien Wajsberg [:julienw] from bug 1143575 comment #327)
> > I don't know yet if this is related to the issue we have in the unit tests,
> > but I noticed that we have an invalidation problem.
> > 
> > For example load any video inside Firefox or B2G with your patch:
> > http://techslides.com/demos/sample-videos/small.webm
> > 
> > The video does not update properly with the patch. It updates if I move the
> > mouse cursor on top of the video.

Pushed a fix for that in bug 1181303. Possibly it might fix this bug too.

Comment 3

[GitHub Autolander Bot]

Attachment: [gaia] sfoster:ftu-reenable-tutorial-tests-bug-1181201 > mozilla-b2g:master

Comment 4

[Sam Foster [:sfoster] (he/him)]

The run on taskcluster[1] shows green so it does look like the patch on bug 1181303 fixed it. 

But if you look closely you'll see the first time around we had video app unit tests failures[2]. Those are obviously orthogonal to this patch - which just re-enables the FTU tutorial unit tests, but might be worth keeping an eye on in the context of the fix landed in bug 1181303. 

1. https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=3393d9d46772920d605e531d61010e2768342a11
2. https://tools.taskcluster.net/task-inspector/#U2GvBDCkSeCLvmxmH0bg4A/0

Comment 5

[Sam Foster [:sfoster] (he/him)]

Test re-enabled on gaia/master: https://github.com/mozilla-b2g/gaia/commit/85f2b501f46a811602d99c69bd5058629d1537f2

Note, the actual fix was in bug 1181303

Comment 6

[Russ Nicoletti [:russn]]

(In reply to Sam Foster [:sfoster] from comment #4)
> But if you look closely you'll see the first time around we had video app
> unit tests failures[2]. Those are obviously orthogonal to this patch - which
> just re-enables the FTU tutorial unit tests, but might be worth keeping an
> eye on in the context of the fix landed in bug 1181303. 
> 
> 2. https://tools.taskcluster.net/task-inspector/#U2GvBDCkSeCLvmxmH0bg4A/0

The video unit test failure referred to above is a known intermittent failure; it is being tracked by bug 1147871.

Comment 7

[Kevin Grandon :kgrandon]

Disabled again at the request of sheriffs for failing 8/9 times on b-i: https://github.com/mozilla-b2g/gaia/commit/1f98c478b0991465ff7e86e8664f017b82db2514

Comment 8

[Sam Foster [:sfoster] (he/him)]

I'll explain why this test is failing as its not super clear from the code. Tutorial.init() is async as it needs to load some config data. In FTU we have 2 different paths that can mean Tutorial may be asked to start() while init() is still underway. The test is ensuring that in either case, the Tutorial is initialized and starts correctly. 

It looks like the callback passed to Tutorial.start(cb) is never called. start() involves loading up the first video . The Tutorial does handle the case where video fails to load, see the _loadMedia function at https://github.com/mozilla-b2g/gaia/blob/master/apps/ftu/js/tutorial.js#L20,L60

I'll do some digging and see if I can pinpoint where its getting snarled up. Roc, if you have other tricks up your sleeve, or can spot any bad assumptions in that code, please let me know.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The video element has preload="auto" which on B2G means preload="metadata". In Gecko preload="metadata" means you'll advance to at least HAVE_CURRENT_DATA, but you won't necessarily reach HAVE_FUTURE_DATA (required for canplay to fire) before we stop loading the data. That could explain these failures.

Maybe you could alter this code to use the "autoplay" attribute instead? That'd probably be the best.

Alternatively just call play() on the video as soon as you set the src.

Comment 10

[GitHub Autolander Bot]

Attachment: [gaia] sfoster:ftu-autoplay-bug-1181201 > mozilla-b2g:master

Comment 11

[Sam Foster [:sfoster] (he/him)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (offline July 8-10) from comment #9)
> The video element has preload="auto" which on B2G means preload="metadata".
> In Gecko preload="metadata" means you'll advance to at least
> HAVE_CURRENT_DATA, but you won't necessarily reach HAVE_FUTURE_DATA
> (required for canplay to fire) before we stop loading the data. That could
> explain these failures.
> 
> Maybe you could alter this code to use the "autoplay" attribute instead?
> That'd probably be the best.

Sounds both reasonable and plausible. Trying it out at: https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=c89b004ba70334c24f3851efe5eae796de5c9767

Comment 12

[Sam Foster [:sfoster] (he/him)]

Doh, didn't un-disable the test in question, will push again :)

Comment 13

[Sam Foster [:sfoster] (he/him)]

Moving component

Comment 14

[Sam Foster [:sfoster] (he/him)]

Comment on attachment 8632165 [details] [review]
[gaia] sfoster:ftu-autoplay-bug-1181201 > mozilla-b2g:master

Assuming tests stay green...

Comment 15

[Fernando Campo (:fcampo)]

Comment on attachment 8632165 [details] [review]
[gaia] sfoster:ftu-autoplay-bug-1181201 > mozilla-b2g:master

got to love the tiny patches :D

Comment 16

[Sam Foster [:sfoster] (he/him)]

Merged to master: https://github.com/mozilla-b2g/gaia/commit/26003f00c5d527d780f8e6d417ebc966e19f24be

One of the findmydevice tests (Gu9) was having issues, but looks like it worked itself out - and is entirely unrelated anyhow.

Comment 17

[Julien Wajsberg [:julienw]]

Wondering if you should remove the call to "play" in https://github.com/mozilla-b2g/gaia/blob/master/apps/ftu/js/tutorial.js#L245 now.

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Re-disabled because it's still permafailing on b2g-inbound
https://github.com/mozilla-b2g/gaia/commit/3af01fbe2142880accf069c77ff64ad3287b4e7a

Comment 19

[Sam Foster [:sfoster] (he/him)]

while b2g may live on, FTU will not.