Closed Bug 1135383 Opened 5 years ago Closed 5 years ago

Convert Marionette unit tests to use SpecialPowers.pushPrefEnv

Categories

(Testing :: Marionette, defect)

x86
macOS
defect
Not set

Tracking

(firefox39 fixed)

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: martijn.martijn, Assigned: anishchandran94)

Details

Attachments

(1 file, 5 obsolete files)

Attached patch marionette.diff (obsolete) — Splinter Review
These are the marionette-test files that need converting:
layout/base/tests/marionette/test_touchcaret.py
layout/base/tests/marionette/test_selectioncarets.py
layout/base/tests/marionette/test_selectioncarets_multiplerange.py
testing/marionette/client/marionette/tests/unit/test_switch_remote_frame.py 
testing/marionette/client/marionette/tests/unit/test_getactiveframe_oop.py (this test doesn't seem to work currently, see bug 925688, comment 124)

You can run and test those files, using: ./mach marionette-test TEST_FILE

Also, if you happen to see instances of addPermission, those need to be converted to use pushPermissions, see here for example of this:
http://mxr.mozilla.org/mozilla-central/source/dom/base/test/test_XHR_anon.html?force=1#172
Attached patch marionette.patch (obsolete) — Splinter Review
Comment on attachment 8567670 [details] [diff] [review]
marionette.patch

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

::: layout/base/tests/marionette/test_selectioncarets_multiplerange.py
@@ +29,2 @@
>              ('true' if enabled else 'false'))
> +        

The pushPrefEnv calls (and hence the execute_async_script calls can be combined into one, I think.

::: testing/marionette/client/marionette/tests/unit/test_switch_remote_frame.py
@@ +22,5 @@
>              """)
> +        self.marionette.execute_async_script(
> +            'SpecialPowers.pushPrefEnv({"set": [["dom.ipc.browser_frames.oop_by_default", true]]}, marionetteScriptFinished);')
> +        self.marionette.execute_async_script(
> +            'SpecialPowers.pushPrefEnv({"set": [["dom.mozBrowserFramesEnabled", true]]}, marionetteScriptFinished);')

I guess I could have also said that in the previous time I review, but the execute_async_script calls could be combined into one, combining the pushPrefEnv calls into one.
Attachment #8567670 - Flags: review+
Attachment #8567670 - Flags: review?(jmaher)
(In reply to Martijn Wargers [:mwargers] (QA) from comment #2)
> I guess I could have also said that in the previous time I review, but the
> execute_async_script calls could be combined into one, combining the
> pushPrefEnv calls into one.

But don't bother with it anymore, at this point, unless Joel Maher sees something wrong with the patch and you need to make other changes, anyway.
Comment on attachment 8567670 [details] [diff] [review]
marionette.patch

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

code wise this looks great and please address the issues Martijn has already brought up.
Attachment #8567670 - Flags: review?(jmaher) → review+
Attached patch marionette.patch (obsolete) — Splinter Review
Attachment #8567670 - Attachment is obsolete: true
Attachment #8568071 - Flags: review+
Attachment #8567514 - Attachment is obsolete: true
Attached patch marionette.diff (for checkin) (obsolete) — Splinter Review
Now with check-in comment, this is ready to be checked in.
Attachment #8568071 - Attachment is obsolete: true
Keywords: checkin-needed
Hi, this failed to apply:

applying marionette.diff
patching file layout/base/tests/marionette/test_selectioncarets_multiplerange.py
Hunk #1 FAILED at 15
1 out of 1 hunks FAILED -- saving rejects to file layout/base/tests/marionette/test_selectioncarets_multiplerange.py.rej
patch failed, unable to continue (try -v)


could you take a look, thanks!
Flags: needinfo?(anishchandran94)
Keywords: checkin-needed
Attached patch marionette.diff (obsolete) — Splinter Review
Flags: needinfo?(anishchandran94)
Attachment #8569311 - Flags: review?(martijn.martijn)
Comment on attachment 8569311 [details] [diff] [review]
marionette.diff

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

test_selectioncarets_multiplerange.py is not there in the patch.
Attachment #8569311 - Flags: review?(martijn.martijn) → review-
Attached patch marionette.diffSplinter Review
This is an update patch, ready for checkin. It's updated to the latest trunk, so it should apply cleanly now.
Attachment #8568623 - Attachment is obsolete: true
Attachment #8569311 - Attachment is obsolete: true
Attachment #8569343 - Attachment filename: marionette.diff → marionette.diff (for checkin)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/dc2c018e12c7
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.