Closed Bug 943556 Opened 7 years ago Closed 7 years ago

Intermittent test_videocontrols_standalone.html | Height of audio element should be 28, which is equal to the controls bar. - got 150, expected 28

Categories

(Core :: Audio/Video, defect)

Product:
Core
Component:
Audio/Video
Platform:
x86_64
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla28
Tracking Flags:
Tracking Status
firefox26 --- unaffected
firefox27 --- unaffected
firefox28 --- fixed
firefox-esr24 --- unaffected

People

(Reporter: RyanVM, Assigned: jaws)

Details

(Keywords: intermittent-failure)

Attachments

(1 file, 2 obsolete files)

Patch v1.2
4.69 KB, patch
Gijs
: review+
Details | Diff | Splinter Review 
jaws, weren't you playing with this test recently?

https://tbpl.mozilla.org/php/getParsedLog.php?id=31087377&tree=B2g-Inbound

Rev5 MacOSX Mountain Lion 10.8 b2g-inbound opt test mochitest-5 on 2013-11-26 02:34:28 PST for push f66d18acab45
slave: talos-mtnlion-r5-082

02:41:57     INFO -  134626 INFO TEST-START | /tests/toolkit/content/tests/widgets/test_videocontrols_standalone.html
02:41:57     INFO -  134627 INFO TEST-PASS | /tests/toolkit/content/tests/widgets/test_videocontrols_standalone.html | Width of the video should match expectation
02:41:57     INFO -  134628 INFO TEST-PASS | /tests/toolkit/content/tests/widgets/test_videocontrols_standalone.html | Height of video should match expectation
02:41:57     INFO -  134629 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/content/tests/widgets/test_videocontrols_standalone.html | Height of audio element should be 28, which is equal to the controls bar. - got 150, expected 28
02:41:57     INFO -  134630 INFO TEST-INFO | MEMORY STAT vsize after test: 3929051136
02:41:57     INFO -  134631 INFO TEST-INFO | MEMORY STAT residentFast after test: 325296128
02:41:57     INFO -  134632 INFO TEST-INFO | MEMORY STAT heapAllocated after test: 104918272
02:41:57     INFO -  134633 INFO TEST-END | /tests/toolkit/content/tests/widgets/test_videocontrols_standalone.html | finished in 368ms
Flags: needinfo?(jaws)
Yes, this test is new. I'll investigate it.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Flags: needinfo?(jaws)
Attached patch Patch (obsolete) — — Splinter Review 
I can't explain comment #2 or #3. The failures make no sense to me since the test is saying that it expects the height to be 28px, but that only happens if the user agent doesn't include "Android" in it, and these test failures are on Android 4.0. Notice too that the actual height is 123, which is the height that the test expects controls on Android to have.

However, this patch does adjust the event that is listened to for the measurement to be slightly later which should fix the initial orange.
Attachment #8339068 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8339068 [details] [diff] [review]
Patch

Review of attachment 8339068 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Jared Wein [:jaws] from comment #4)
> Created attachment 8339068 [details] [diff] [review]
> Patch
> 
> I can't explain comment #2 or #3. The failures make no sense to me since the
> test is saying that it expects the height to be 28px, but that only happens
> if the user agent doesn't include "Android" in it, and these test failures
> are on Android 4.0. Notice too that the actual height is 123, which is the
> height that the test expects controls on Android to have.
> 
> However, this patch does adjust the event that is listened to for the
> measurement to be slightly later which should fix the initial orange.

Is it possible another test messed with the userAgent here as well? I watched as philor figured out another intermittent orange caused by something like that recently... Might be worth info()ing the userAgent when you check it. See bug 942470.

::: toolkit/content/tests/widgets/test_videocontrols_standalone.html
@@ +25,5 @@
>    var video = getMediaElement(popup);
>    if (video.readyState >= video.HAVE_METADATA)
>      runTestVideo(video);
>    else {
> +    video.addEventListener("play", function onPlay() {

This should match the readyState check (or an alternative) because now the two paths through this if/else statement diverge (further) which sets us up for more intermittent orange in the future.
Attachment #8339068 - Flags: review?(gijskruitbosch+bugs)
Attached patch Patch v1.1 (obsolete) — — Splinter Review 
Good catch on the readyState change. Thanks!
Attachment #8339068 - Attachment is obsolete: true
Attachment #8339106 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8339106 [details] [diff] [review]
Patch v1.1

Review of attachment 8339106 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM
Attachment #8339106 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to TBPL Robot from comment #12)
> philor
> https://tbpl.mozilla.org/php/getParsedLog.php?id=31180834&tree=Fx-Team
> Android 4.0 Panda fx-team opt test mochitest-8 on 2013-11-27 12:21:19
> revision: 7897e6dbbebf
> slave: panda-0152
> 
> 77420 ERROR TEST-UNEXPECTED-FAIL |
> /tests/toolkit/content/tests/widgets/test_videocontrols_standalone.html |
> Height of audio element should be 28, which is equal to the controls bar. -
> got 123, expected 28
> 11-27 12:48:34.437 I/GeckoDump( 2236): 77420 ERROR TEST-UNEXPECTED-FAIL |
> /tests/toolkit/content/tests/widgets/test_videocontrols_standalone.html |
> Height of audio element should be 28, which is equal to the controls bar. -
> got 123, expected 28
> 11-27 12:48:34.437 I/GeckoDump( 2236): 77420 ERROR TEST-UNEXPECTED-FAIL |
> /tests/toolkit/content/tests/widgets/test_videocontrols_standalone.html |
> Height of audio element should be 28, which is equal to the controls bar. -
> got 123, expected 28

The user agent in this failure is "DummyUserAgent". These are incarnations of bug 942470. The 150 number though should be fixed by this patch nonetheless.
Pushed a follow-up patch to switch from checking HAVE_ENOUGH_DATA to !element.paused since that will achieve the correct balance with waiting for the "play" event.

https://hg.mozilla.org/integration/fx-team/rev/48c0dfb60fac
https://hg.mozilla.org/mozilla-central/rev/6cbce980e1c2
https://hg.mozilla.org/mozilla-central/rev/48c0dfb60fac
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
This is probably at fault for turning Win8 bc debug tests semi-perma-orange, after the merge to inbound.  I backed out both changesets landed here:

https://hg.mozilla.org/integration/mozilla-inbound/rev/aef6901931d8

See https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=7e9a7bde5c2c for example.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #19)
> This is probably at fault for turning Win8 bc debug tests semi-perma-orange,
> after the merge to inbound.  I backed out both changesets landed here:
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/aef6901931d8
> 
> See https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=7e9a7bde5c2c for
> example.

hm strange also after the backout this error still happens :(
Attached patch Patch v1.2 — — Splinter Review 
This patch is a roll-up of the previous two patches from this bug. It also includes a waitForCondition that will spin until the audio/video reaches the desired size (timing out at 3 seconds).

https://tbpl.mozilla.org/?tree=Try&rev=277dd64aa426
Attachment #8339106 - Attachment is obsolete: true
Attachment #8341551 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8341551 [details] [diff] [review]
Patch v1.2

Review of attachment 8341551 [details] [diff] [review]:
-----------------------------------------------------------------

r=me assuming the retriggers are green(ish)

::: toolkit/content/tests/widgets/test_videocontrols_standalone.html
@@ +18,5 @@
>  function getMediaElement(aWindow) {
>    return aWindow.document.getElementsByTagName("video")[0];
>  }
>  
> +function waitForCondition(condition, nextTest, errorMsg) {

Nit: can this be in the appropriate head.js file so not all tests have to roll their own?
Attachment #8341551 - Flags: review?(gijskruitbosch+bugs) → review+
I didn't move it to a head.js file originally because no other files were using it, but I guess that will happen eventually.

https://hg.mozilla.org/integration/fx-team/rev/f096db1e9ddd
https://hg.mozilla.org/mozilla-central/rev/f096db1e9ddd
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.