Closed Bug 1181201 Opened 6 years ago Closed 4 years ago

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

Categories

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

defect

Tracking

(firefox42 affected)

RESOLVED WONTFIX
FxOS-S3 (24Jul)
Tracking Status
firefox42 --- affected

People

(Reporter: RyanVM, Assigned: sfoster)

References

Details

(Whiteboard: [test disabled][leave open])

Attachments

(2 files)

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.
Flags: needinfo?(roc)
See Also: → 1181216
(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.
Flags: needinfo?(roc)
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
Test re-enabled on gaia/master: https://github.com/mozilla-b2g/gaia/commit/85f2b501f46a811602d99c69bd5058629d1537f2

Note, the actual fix was in bug 1181303
Assignee: nobody → sfoster
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
(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.
Disabled again at the request of sheriffs for failing 8/9 times on b-i: https://github.com/mozilla-b2g/gaia/commit/1f98c478b0991465ff7e86e8664f017b82db2514
Status: RESOLVED → REOPENED
Flags: needinfo?(sfoster)
Resolution: FIXED → ---
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.
Flags: needinfo?(sfoster) → needinfo?(roc)
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.
Flags: needinfo?(roc)
(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
Doh, didn't un-disable the test in question, will push again :)
Moving component
Component: Video/Audio → Gaia::First Time Experience
Product: Core → Firefox OS
Target Milestone: --- → FxOS-S3 (24Jul)
Version: Trunk → unspecified
Comment on attachment 8632165 [details] [review]
[gaia] sfoster:ftu-autoplay-bug-1181201 > mozilla-b2g:master

Assuming tests stay green...
Attachment #8632165 - Flags: review?(fernando.campo)
Comment on attachment 8632165 [details] [review]
[gaia] sfoster:ftu-autoplay-bug-1181201 > mozilla-b2g:master

got to love the tiny patches :D
Attachment #8632165 - Flags: review?(fernando.campo) → review+
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.
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Priority: -- → P2
Resolution: --- → FIXED
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.
Re-disabled because it's still permafailing on b2g-inbound
https://github.com/mozilla-b2g/gaia/commit/3af01fbe2142880accf069c77ff64ad3287b4e7a
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [test disabled][leave open]
while b2g may live on, FTU will not.
Status: REOPENED → RESOLVED
Closed: 6 years ago4 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.