Closed Bug 868439 Opened 7 years ago Closed 7 years ago

Convert some uses of nsIPrefBranch to SpecialPowers

Categories

(Testing :: Mochitest, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla24

People

(Reporter: martijn.martijn, Assigned: martijn.martijn)

References

(Blocks 1 open bug, )

Details

Attachments

(1 file, 6 obsolete files)

Attached patch patch (obsolete) — Splinter Review
See url search, I guess I'm partially doing work from bug 649563.
I need to see if those tests would pass now on b2g mochitest, then I can remove those from the exlude list from b2g.json file.

Bug 649563, comment 9 is mentioning using SpecialPowers.pushPrefenv preferably, so perhaps this patch could still be improved.

test_bug260264_nested.html is failing while trying to run as single test, btw. I think that is a bug. It was annoying at least, because I thought I broke something at first.
Oh, the +var splat = []; part should be removed.
Some other old and now wrong use of pref service in tests here:
http://mxr.mozilla.org/mozilla-central/search?string=Components.interfaces.nsIPrefService
I'll try to fix those instances in a follow-up bug.
If you want these tests to be B2G-friendly then you really do want to use pushPrefEnv. That ensures that the pref is actually set in the content process before calling the callback.
Regarding comment 3, chatted with ted on irc about this, it seems that all preference setting has to be rewritten to pushPrefEnv, ideally.
In:
http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/bugs/utils_bug260264.js
There is also some permissions code that has to be rewritten to use SpecialPowers.pushPermissions, btw.
Working on converting test_bug260264.html, it's quite difficult.
Note, I'm currently getting this error at:
http://mxr.mozilla.org/mozilla-central/source/testing/specialpowers/content/specialpowersAPI.js#537
537           cleeanupTodo.value = originalValue;
That definitely seems like a typo there.
Attached file test_bug260264.html (obsolete) —
This is what I currently have for test_bug260264.html.
It's really complicated to change it pushPrefEnv and pushPermissions, because of all the nested calls. It's as good as a complete rewrite of the test, but it's probably not testing absolutely the same anymore.
I get 12 unexpected-fails with the test, not sure where those are coming from.
Ok, the unexpected-fails are happening because UNKNOWN value is not supported, this is related to bug 859401, basically.
Attached patch wip patch (obsolete) — Splinter Review
Ok, it is getting too complicated.
I'll first try to fix bug 859401, so test_bug260264.html can be properly converted.
But I should probably spin off a new bug for that, because rewriting that test is quite a bit of work and quite different from other tests.
Attachment #745172 - Attachment is obsolete: true
Attachment #745415 - Attachment is obsolete: true
Depends on: 859401
(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #9)
> But I should probably spin off a new bug for that, because rewriting that
> test is quite a bit of work and quite different from other tests.

I filed bug 873403 for it.
Attached patch patch (obsolete) — Splinter Review
Ok, this has mainly become changes in the media tests now.
These files are changed:
M content/media/test/Makefile.in
M content/media/test/manifest.js
M content/media/test/test_buffered.html
M content/media/test/test_bug493187.html
M content/media/test/test_can_play_type_mpeg.html
M content/media/test/test_closing_connections.html
M content/media/test/test_decode_error.html
M content/media/test/test_preload_actions.html
M content/media/test/test_preload_suspend.html
M content/media/test/test_progress.html
M content/media/test/test_video_to_canvas.html
M layout/forms/test/test_bug411236.html
M toolkit/content/tests/widgets/Makefile.in
M toolkit/content/tests/widgets/test_audiocontrols_dimensions.html
M toolkit/content/tests/widgets/test_videocontrols.html
M toolkit/content/tests/widgets/test_videocontrols_audio_direction.html
M toolkit/content/tests/widgets/test_videocontrols_video_direction.html
M toolkit/content/tests/widgets/videocontrols_direction_test.js
R content/media/test/use_large_cache.js
R toolkit/content/tests/widgets/use_large_cache.js

I guess the layout/forms/test/test_bug411236.html file is a little out of place.

I'm a little worried about toolkit/content/tests/widgets/test_videocontrols.html.
Because I'm setting the pref with pushPrefEnv and the invoked function is having a window load eventlistener. Theoretically, I think the pushPrefEnv invoked function can be invoked after the window load event, right? Although, it didn't seem to give me trouble when I tested it. Do you have an idea on how to circumvent this, or perhaps this isn't an issue?

Btw, I changed content/media/test/test_preload_suspend.html , but that test is disabled, see bug 493692.
Attachment #747387 - Attachment is obsolete: true
Attachment #751338 - Flags: review?(jmaher)
Comment on attachment 751338 [details] [diff] [review]
patch

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

just a couple nits here.  Thanks for making the big pass through here and using the latest and greatest SpecialPowers code!

::: content/media/test/test_closing_connections.html
@@ +46,4 @@
>  } else {
>    todo(false, "No types supported");
>  }
> +}

can you indent the code inside of beginTest here?

::: layout/forms/test/test_bug411236.html
@@ +60,5 @@
>  
>  function do_test() {
> +  // accessibility.tabfocus must be set to value 7 before running test also
> +  // on a mac.
> +  SpecialPowers.pushPrefEnv({"set": [["accessibility.tabfocus", 7]]}, do_test2);

I would prefer to call do_test beginTest() and not use do_test2, but do_test()
Attachment #751338 - Flags: review?(jmaher) → review+
Attached patch patch for checkin (obsolete) — Splinter Review
Ok, made those changes. I also rechecked the affected tests to make sure they run properly on my macbook.
Attachment #751338 - Attachment is obsolete: true
Assignee: nobody → martijn.martijn
Keywords: checkin-needed
Currently, there is bug 875092, which prevents me from running mochitests on Fennec locally.
Ok, I think I know what's going on with test_access_control.html. On Android the media.preload.default is 1, while on desktop it is 2. 
I put the prefs setting in manifest.js inside the MediaTestManager.runTests function, but that is not called at all by test_access_control.html, hence the time out there.
I fixed that by setting the prefs in the test_access_control.html file itself and combine it with a function to test on. There are more of those cases with this problem:
http://mxr.mozilla.org/mozilla-central/search?string=+src=%22manifest.js%22
test_autoplay.html, test_bug463162.xhtml, test_error_in_video_document.html, test_delay_load.html, test_defaultMuted.html, test_contentDuration7.html, test_closing_connections.html, test_can_play_type_webm.html, test_can_play_type_wave.html, test_can_play_type_ogg.html,  test_can_play_type_no_webm.html, test_can_play_type_no_wave.html, test_can_play_type_no_ogg.html, test_can_play_type.html, test_paused.html, test_no_load_event.html,  test_networkState.html, test_mixed_principals.html, test_load_source.html, test_load.html, test_error_on_404.html, test_seekable2.html, test_source.html, test_reactivate.html, test_referer.html, test_source_media.html, test_bug726904.html, test_source_null.html, test_streams_element_capture_reset.html, test_too_many_elements.html, test_standalone.html, test_source_write.html, test_streams_autoplay.html, test_seekable3.html, test_reset_src.html,  test_dash_detect_stream_switch.html, test_streams_gc.html, test_can_play_type_no_dash.html, test_can_play_type_dash.html, test_can_play_type_mpeg.html and test_streams_srcObject.html

I guess I should write a manager.setPrefs function in manifest.js that has to be called from those test files.

I have no idea why test_audio_event_adopt.html would be timing out. I haven't changed that file and it doesn't use manifest.js

test_bug465498.html is timing out after the opus test-pass, that one is related to the pref setting in manifest.js that this file is using. But I have no idea why it is failing.
test_bug495145.html seems to be even timing out earlier. It's using the same file array as test_bug465498.html: gSmallTests.
can you get a patch up and try server run linked where you put setPrefs in manifest.js ?
I can't put setPrefs in manifest.js. It uses the SpecialPowers.pushPrefEnv function, which is an asynchronous call and expects to run some function after it.
Attached patch patch (obsolete) — Splinter Review
Ok, I'm leaving manifest.js as it is for now and will try to fix that in a follow-up bug.
This is only changing the use_large_cache.js thing (and some unrelated fix).

I'll push this to try.
Attachment #751935 - Attachment is obsolete: true
Attachment #762195 - Flags: review?(jmaher)
Comment on attachment 762195 [details] [diff] [review]
patch

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

This looks great.  r=me assuming you resolve the one comment in test-closing_connections.html.

::: content/media/test/test_closing_connections.html
@@ +43,5 @@
> +      v.src = resource.name;
> +      document.body.appendChild(v);
> +    }
> +  } else {
> +    todo(false, "No types supported");

in this case we will always waitForExplicitFinish, whereas before we would only do it if resource != null.
Attachment #762195 - Flags: review?(jmaher) → review+
(In reply to Joel Maher (:jmaher) from comment #23)
> in this case we will always waitForExplicitFinish, whereas before we would
> only do it if resource != null.

Yeah, that's a good observation. However, it seems to me that previous code was wrong, because this would potentially allow a Simpletest.finish() to be called without a Simpletest.waitForExplicitFinish() call.
So I think my patch actually fixes that.
Comment on attachment 762195 [details] [diff] [review]
patch

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

::: content/media/test/test_closing_connections.html
@@ +43,5 @@
> +      v.src = resource.name;
> +      document.body.appendChild(v);
> +    }
> +  } else {
> +    todo(false, "No types supported");

in looking at this further, the waitForExplicitFinish is just fine, I do see in the source file a script src=use_large_cache.js reference, did we forget to remove that?
Yes, you're right. No idea how those files got missing from the patch.
Attachment #762195 - Attachment is obsolete: true
I've pushed this to try, just to be sure: https://tbpl.mozilla.org/?tree=Try&rev=2e80ddaadab1
The try run seems fine, it's not completely finished, but I'm confident that the patch is fine.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/09fcb384c6a0
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.