Closed Bug 818466 Opened 9 years ago Closed 9 years ago

Basic automated WebRTC gUM tests are broken on Android

Categories

(Core :: WebRTC, defect)

ARM
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

(Whiteboard: [getUserMedia][blocking-gum+][qa-])

Attachments

(1 file, 1 obsolete file)

As seen by the lastest try server run all three tests which have been created on bug 781534 are failing on Android, even with the special-cased code in place. 

https://tbpl.mozilla.org/?tree=Try&rev=19e9c6b652d5

116 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/media/tests/mochitest/test_getUserMedia_basicAudio.html | Test timed out.
119 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/media/tests/mochitest/test_getUserMedia_basicVideo.html | Test timed out.
122 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/media/tests/mochitest/test_getUserMedia_basicVideoAudio.html | Test timed out.

I will assign this bug to Jason for investigation and for a possible fix. It looks like it's a fault by the test.
The fact that the test is passing on b2g makes me think that this isn't a general problem nor a code problem specifically in the tests themselves (basic video, media stream playback, etc).

My only guess is that the following line is failing on Android in head.js:

  if(desktopSupportedOnly && (navigator.platform === 'Android' ||
     navigator.platform === '')) {

Which would imply I've got the wrong navigator.platform when this runs on Android. Our automation runs on pandaboards for Android, right? And the platform value isn't Android?

Clint - Do you know?
Flags: needinfo?(ctalbert)
Actually hmm...so if that was true in comment 1, it would be failing repeatedly though in comment 1 on every single test run, but it appears to only be failing intermittently.

I actually question that this is a bug in my test and question this is actually a bug in mochitest on Android. Something just isn't adding up here.
Why do you think it's failing intermittently only? For each build we run the mochitests I see failures. Please see 4*.

As best you add some code to your test which dumps info to the console and do a try server run. Checking the logs should show us what navigator.platform is.

Other than that I wonder why we haven't used an ifdef here to exclude Android but do it in head.js.
(In reply to Henrik Skupin (:whimboo) from comment #3)
> Why do you think it's failing intermittently only? For each build we run the
> mochitests I see failures. Please see 4*.
> 
> As best you add some code to your test which dumps info to the console and
> do a try server run. Checking the logs should show us what
> navigator.platform is.
> 
> Other than that I wonder why we haven't used an ifdef here to exclude
> Android but do it in head.js.

See the bug for the implementation of the automation that explains why it was done in head.js.
The conversation is long. Can you reference a specific comment please?
Oh. I see what you mean in comment 3. It's failing on each 4*. What does 4* do?
It's just another chunk of Mochitests. Looks like on Android the tests are shifted and our webrtc tests are getting run in chunk 4.
(In reply to Henrik Skupin (:whimboo) from comment #7)
> It's just another chunk of Mochitests. Looks like on Android the tests are
> shifted and our webrtc tests are getting run in chunk 4.

Ah. I see now. So it's generally deterministically failing.
*Checks Internet* Yup, it's a navigator.platform issue:

http://stackoverflow.com/questions/7246091/how-to-detect-firefox-mobile-with-javascript

I'm using 'Android' but should be using 'android' - I'm going to double on check on my galaxy nexus to be sure.
Flags: needinfo?(ctalbert)
Well...actually on a galaxy nexus it wasn't even android:

Linux armv7l

I'll look into an alternative way to check then.
Dan, do you have an idea what to use best here to special case tests for Android?
I think I got it:

<script type="text/javascript">
if(navigator.userAgent.indexOf("Android") > -1) {
    alert("Yes!");
} else {
   alert("no!");
}
</script>

Give me a sec to try that.
Okay that worked in comment 12. Will update the patch now...
Attached patch Fix Android Gum Tests v1 (obsolete) — Splinter Review
Attachment #688973 - Flags: review?(hskupin)
The reason your mochitests "passed" on b2g is that we don't run all mochitests on b2g yet.  There are many levels of filters that are being run here and if you look at the command line being run on mochitest in the logs you can see that there is a .json file that is used to filter out what is run and not run for mochitests. 

This is a temporary solution so that we can have green tests while we
1. Get all tests up and running for b2g
2. Establish a clean way to do true manifests for mochitests so we don't have random .json filters and make file hackery. (Goal, Q1 2013).
Comment on attachment 688973 [details] [diff] [review]
Fix Android Gum Tests v1

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

Review is too early here. You will also have to enable those tests which will work with that change. Randell has merged m-c to alder about 4h ago so I can now safely push to try. Please check the results of the following try build and update the patch accordingly.

https://tbpl.mozilla.org/?tree=Try&rev=57eeb7b29837

Otherwise f+ given that this should do the trick.
Attachment #688973 - Flags: review?(hskupin) → feedback+
Err...so the try run states many areas of fruitful bustage, but it doesn't look like my patch caused that? I didn't touch the makefiles...
(In reply to Jason Smith [:jsmith] from comment #17)
> Err...so the try run states many areas of fruitful bustage, but it doesn't
> look like my patch caused that? I didn't touch the makefiles...

Looks like it's because I missed to add a backslash at the end of a comment line in my patch on bug 817709. I will push a bustage fix over there. 

Also another try run didn't succeed because of bug 762358. So we have to trigger a clobber build on alder first. But that should not happen  before the before mentioned patch has been merged.
Now that we can run leaking mochitests on alder I have triggered another try server run with the patch applied and all tests enabled. Once results are up lets leave those tests which pass and keep the others commented out.

https://tbpl.mozilla.org/?tree=Try&rev=19863b5927a3
Status: NEW → ASSIGNED
If I'm reading the results correctly from try, looks like we are green.
Yes! Can you please update the patch so we can enable those two tests which do not fail on OS X? Once reviewed I can get it landed.
Whiteboard: [getUserMedia] → [getUserMedia][blocking-gum+]
Attached patch Patch v2Splinter Review
Updated patch as by the investigation made by Jason. Try server runs are green so we are good to enable the next two mochitests!
Assignee: jsmith → hskupin
Attachment #688973 - Attachment is obsolete: true
Attachment #691503 - Flags: review?(rjesup)
Attachment #691503 - Flags: review?(rjesup) → review+
Comment on attachment 691503 [details] [diff] [review]
Patch v2

Also looks good on my end.
Attachment #691503 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/edd45de440ba
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Whiteboard: [getUserMedia][blocking-gum+] → [getUserMedia][blocking-gum+][qa-]
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.