Closed
Bug 818466
Opened 12 years ago
Closed 12 years ago
Basic automated WebRTC gUM tests are broken on Android
Categories
(Core :: WebRTC, defect)
Tracking
()
RESOLVED
FIXED
mozilla20
People
(Reporter: whimboo, Assigned: whimboo)
References
Details
(Whiteboard: [getUserMedia][blocking-gum+][qa-])
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
jesup
:
review+
jsmith
:
review+
|
Details | Diff | Splinter Review |
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.
Comment 1•12 years ago
|
||
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)
Comment 2•12 years ago
|
||
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.
Assignee | ||
Comment 3•12 years ago
|
||
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.
Comment 4•12 years ago
|
||
(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.
Assignee | ||
Comment 5•12 years ago
|
||
The conversation is long. Can you reference a specific comment please?
Comment 6•12 years ago
|
||
Oh. I see what you mean in comment 3. It's failing on each 4*. What does 4* do?
Assignee | ||
Comment 7•12 years ago
|
||
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.
Comment 8•12 years ago
|
||
(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.
Comment 9•12 years ago
|
||
*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)
Comment 10•12 years ago
|
||
Well...actually on a galaxy nexus it wasn't even android:
Linux armv7l
I'll look into an alternative way to check then.
Assignee | ||
Comment 11•12 years ago
|
||
Dan, do you have an idea what to use best here to special case tests for Android?
Comment 12•12 years ago
|
||
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.
Comment 13•12 years ago
|
||
Okay that worked in comment 12. Will update the patch now...
Comment 14•12 years ago
|
||
Updated•12 years ago
|
Attachment #688973 -
Flags: review?(hskupin)
Comment 15•12 years ago
|
||
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).
Assignee | ||
Comment 16•12 years ago
|
||
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+
Comment 17•12 years ago
|
||
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...
Assignee | ||
Comment 18•12 years ago
|
||
(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.
Assignee | ||
Comment 19•12 years ago
|
||
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
Comment 20•12 years ago
|
||
If I'm reading the results correctly from try, looks like we are green.
Assignee | ||
Comment 21•12 years ago
|
||
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.
Updated•12 years ago
|
Whiteboard: [getUserMedia] → [getUserMedia][blocking-gum+]
Assignee | ||
Comment 22•12 years ago
|
||
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)
Updated•12 years ago
|
Attachment #691503 -
Flags: review?(rjesup) → review+
Comment 23•12 years ago
|
||
Comment on attachment 691503 [details] [diff] [review]
Patch v2
Also looks good on my end.
Attachment #691503 -
Flags: review+
Assignee | ||
Comment 24•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Updated•12 years ago
|
Whiteboard: [getUserMedia][blocking-gum+] → [getUserMedia][blocking-gum+][qa-]
Updated•12 years ago
|
Flags: in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•