Closed
Bug 896582
Opened 11 years ago
Closed 11 years ago
Implement a workaround to bug 838726 to get a bunch of content/media mochitests running on B2G
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
mozilla25
People
(Reporter: jsmith, Assigned: martijn.martijn)
References
Details
Attachments
(1 file, 7 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
bug 838726 will currently will lead to causing many of the content/media mochitests to fail with something like the following:
00:03:50 INFO - 3 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/test/test_invalid_reject.html | We expected 'tests/content/media/test/320x240.ogv' to exist, but it doesn't!
Martijn mentions there's a workaround we can do here that Android considered doing here in manifest.js:
http://mxr.mozilla.org/mozilla-central/source/content/media/test/manifest.js#213
As see at that line of code, we should consider adding a B2G here like the following:
SpecialPowers.Services.appinfo.name == "B2G"
Let's see if we can implement a workaround to get around bug 838726 to be able to get many of these content/media mochitests running.
Reference Try Run:
https://tbpl.mozilla.org/?tree=Try&rev=20a337b39982
Reporter | ||
Updated•11 years ago
|
Blocks: b2g-mochitests
Assignee | ||
Comment 1•11 years ago
|
||
Wip patch, that at least completes all the media tests, althought there are still 25 failures to be worked out.
I have some major issues on my vm with Ubuntu and some files getting random read-only.
I'll post a follow-up patch to be used on tryserver.
Assignee | ||
Comment 2•11 years ago
|
||
Pushed this to try: https://tbpl.mozilla.org/?tree=Try&rev=b9415d926437
Some notes:
- Iirc, all of the testfiles I added in the exclude list, were due to timeouts (no idea why they are happening)
- test files that are opened by window.open don't have a SpecialPowers object
- There is all kinds of netscape.enablePrivilege code lying around in this directory that has to be changed at some point
Attachment #779942 -
Attachment is obsolete: true
Assignee | ||
Comment 3•11 years ago
|
||
wippatch2 pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=d59d4ffa3e2e
Getting some different failures locally, which is strange, but I just added them to the exclude list for now.
Attachment #779953 -
Attachment is obsolete: true
Assignee | ||
Comment 4•11 years ago
|
||
Sorry, some stupid mistake, which made the tryserver run fail, new one: https://tbpl.mozilla.org/?tree=Try&rev=70c80dfde9f8
Attachment #780101 -
Attachment is obsolete: true
Assignee | ||
Comment 5•11 years ago
|
||
Fixed some obvious mistake in test_decoder_disable and added some test files in b2g that were reliably failing: https://tbpl.mozilla.org/?tree=Try&rev=eb128b70ea84
Attachment #780246 -
Attachment is obsolete: true
Assignee | ||
Comment 6•11 years ago
|
||
Final patch (hopefully), I'll do a try server run on this patch, shortly (first I'll make sure this works at least reasonably well, locally).
I have just added content/media/test/test_decoder_disable.html to the exclude list for now, it should be easy to fix, but I'll do that later.
Also added content/media/test/test_can_play_type.html to the exclude list.
Assignee | ||
Comment 7•11 years ago
|
||
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=3d6d5130387c
There is the issue where b2g mochitests fail to even start up when the b2g.json file gets too big, see bug 817638, comment 37.
On my Macbook Pro, I noticed that with the extra entries in the exclude list, time to start up for loading the mochitest app, then loading the first test takes appr. 85s.
Without the entries in the exlude list, time to start up for loading the mochitest app, then loading the first test takes appr. 75s.
Assignee | ||
Comment 8•11 years ago
|
||
Added these to the exclude list:
"content/media/test/test_can_play_type_ogg.html":"",
"content/media/test/test_can_play_type_no_dash.html":"",
Because in the last try run, they caused failures in chunk 2.
Pushed this to try: https://tbpl.mozilla.org/?tree=Try&rev=1e90fb5259ea
Attachment #780333 -
Attachment is obsolete: true
Attachment #780578 -
Attachment is obsolete: true
Assignee | ||
Comment 9•11 years ago
|
||
Comment on attachment 780901 [details] [diff] [review]
896582_media2.diff
Ok, latest try is all green.
Fyi, chunk 3 of mochitest b2g takes the longest, 87 minutes, much longer than the other ones.
Attachment #780901 -
Flags: review?(jgriffin)
Reporter | ||
Comment 10•11 years ago
|
||
Comment on attachment 780901 [details] [diff] [review]
896582_media2.diff
Review of attachment 780901 [details] [diff] [review]:
-----------------------------------------------------------------
::: content/media/test/manifest.js
@@ +680,3 @@
> .classes["@mozilla.org/preferences-service;1"]
> .getService(SpecialPowers.Ci.nsIPrefService);
> var branch = prefService.getBranch("media.");
Nit - If we're referencing SpecialPowers directly, do we still need this variable defined?
::: testing/mochitest/b2g.json
@@ +4,5 @@
> "content": "",
> "docshell": "",
> "dom": "",
> + "layout": "",
> + "content/media/":""
You've already specified content above, so I don't think you need to specify this here as well.
Assignee | ||
Comment 11•11 years ago
|
||
(In reply to Jason Smith [:jsmith] from comment #10)
> Comment on attachment 780901 [details] [diff] [review]
> 896582_media2.diff
>
> Review of attachment 780901 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: content/media/test/manifest.js
> @@ +680,3 @@
> > .classes["@mozilla.org/preferences-service;1"]
> > .getService(SpecialPowers.Ci.nsIPrefService);
> > var branch = prefService.getBranch("media.");
>
> Nit - If we're referencing SpecialPowers directly, do we still need this
> variable defined?
Yes, you're right. But I just quickly made that test file passing on b2g for now. This test file needs to be further improved by using pushPrefEnv.
> ::: testing/mochitest/b2g.json
> You've already specified content above, so I don't think you need to specify
> this here as well.
Yes, you're right, I'll remove it, thanks.
Comment 12•11 years ago
|
||
Comment on attachment 780901 [details] [diff] [review]
896582_media2.diff
Review of attachment 780901 [details] [diff] [review]:
-----------------------------------------------------------------
Ditto Jason's comments, but looks good.
::: content/media/test/file_access_controls.html
@@ -149,5 @@
> }
>
> function done() {
> - netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
> - mediaTestCleanup();
Are we certain removing this is completely harmless?
Attachment #780901 -
Flags: review?(jgriffin) → review+
Assignee | ||
Comment 13•11 years ago
|
||
(In reply to Jonathan Griffin (:jgriffin) from comment #12)
> > function done() {
> > - netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
> > - mediaTestCleanup();
>
> Are we certain removing this is completely harmless?
Yes! On the contrary even, if I don't remove it, this would cause failures in the b2g platorm, because popup windows there currently don't have the SpecialPowers object (mediaTestCleanup() is doing a SpecialPowers.forceGC()).
But when the popup is closed opener.done() is called and that function contains a mediaTestCleanup() anyway.
If you don't mind, I'd rather not change manifest.js not further at this point. I have to change this file to use pushPrefEnv anyhow (and I would remove that stuff then), but this a rather extensive change that could effect a whole bunch of files, see 868439 comment 18.
Comment 14•11 years ago
|
||
(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #13)
> (In reply to Jonathan Griffin (:jgriffin) from comment #12)
> > > function done() {
> > > - netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
> > > - mediaTestCleanup();
> >
> > Are we certain removing this is completely harmless?
>
> Yes! On the contrary even, if I don't remove it, this would cause failures
> in the b2g platorm, because popup windows there currently don't have the
> SpecialPowers object (mediaTestCleanup() is doing a SpecialPowers.forceGC()).
> But when the popup is closed opener.done() is called and that function
> contains a mediaTestCleanup() anyway.
>
> If you don't mind, I'd rather not change manifest.js not further at this
> point. I have to change this file to use pushPrefEnv anyhow (and I would
> remove that stuff then), but this a rather extensive change that could
> effect a whole bunch of files, see 868439 comment 18.
Ok, thanks for the explanation, and sounds fair.
Assignee | ||
Comment 15•11 years ago
|
||
For check-in.
Note for whoever this checks in: there is a chance this would cause b2g mochitest chunks to die on start-up, see bug 817638, comment 37. This is not something that is caught by tryserver. Please backout if that happens, thanks.
Attachment #780901 -
Attachment is obsolete: true
Reporter | ||
Comment 16•11 years ago
|
||
Did you mean to flag in checkin-needed btw per comment 15?
Comment 18•11 years ago
|
||
Flags: in-testsuite+
Keywords: checkin-needed
Comment 19•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Comment 20•11 years ago
|
||
Hmm, what's the story with these Web Audio tests which time out here?
Assignee | ||
Comment 21•11 years ago
|
||
Sorry, you meant this broke tbpl on mozilla-central? Feel free to back out!
Reporter | ||
Comment 22•11 years ago
|
||
(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #21)
> Sorry, you meant this broke tbpl on mozilla-central? Feel free to back out!
I think what Ehsan is talking about here is asking a question about why some of the webaudio tests are turned off in the b2g.json file. Specifically:
1. content/media/webaudio/test/test_audioBufferSourceNodeNeutered.html
2. content/media/webaudio/test/test_audioBufferSourceNodeOffset.html
[1] appears to be timing out. The second one doesn't have an indication of why.
Assignee | ||
Comment 23•11 years ago
|
||
Oh, ok. I'll be working on adding the error messages back (or at least some message of why they are failing). I removed them, because there was some concern it caused failures at start up of the b2g mochitest run, due to the large b2g.json file.
Reporter | ||
Comment 24•11 years ago
|
||
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #20)
> Hmm, what's the story with these Web Audio tests which time out here?
I filed bug 898693 for the timeout for audioBufferSourceNodeNeutered.
For audioBufferSourceNodeOffset, this appears to be a test failure:
03:24:56 INFO - 212 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/webaudio/test/test_audioBufferSourceNodeOffset.html | Correct number of samples received (expected: 24000, actual: 58880).
I see bug 895720 that might be similar to this. Is this the same issue as bug 895720? Or should I file a separate bug for this?
Assignee | ||
Comment 25•11 years ago
|
||
It seems like the same bug, but I don't really know.
Assignee | ||
Comment 26•11 years ago
|
||
(In reply to Jason Smith [:jsmith] from comment #24)
> I filed bug 898693 for the timeout for audioBufferSourceNodeNeutered.
This can probably safely be removed from the b2g.json file, see bug 898693, comment 1.
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → martijn.martijn
Assignee | ||
Comment 27•11 years ago
|
||
(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #6)
> I have just added content/media/test/test_decoder_disable.html to the
> exclude list for now, it should be easy to fix, but I'll do that later.
This is now part of bug 898696.
Comment 28•11 years ago
|
||
I mostly meant to point out that when disabling tests like this, it's really helpful to involve the module owner in the discussion. I found out about this by accident... :/
Assignee | ||
Comment 29•11 years ago
|
||
I didn't disable any tests here, I only enabled tests.
I'm working on adding comments back in the b2g.json exclude list, filing bugs for any new issue that I think I'm seeing and filing them in the component of what it is testing. Does that help?
Assignee | ||
Comment 30•11 years ago
|
||
This introduced a bunch of intermittent failures, btw. Those tests were readded to the exclude list. See bug 901732 and bug 901716.
I filed bug 902677 for the fact that a lot of content/media tests are timing out in b2g, currently.
You need to log in
before you can comment on or make changes to this bug.
Description
•