Closed
Bug 1271119
Opened 9 years ago
Closed 8 years ago
Port doCommand() to SpecialPowers to allow executing arbitrary editor commands from mochitest-plain
Categories
(Testing :: Mochitest, defect)
Testing
Mochitest
Tracking
(firefox52 fixed)
RESOLVED
FIXED
mozilla52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: ayg, Assigned: ayg)
References
(Blocks 2 open bugs)
Details
Attachments
(9 files)
(deleted),
text/x-review-board-request
|
mrbkap
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
masayuki
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
masayuki
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
masayuki
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
masayuki
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
masayuki
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
masayuki
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
masayuki
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
masayuki
:
review+
|
Details |
Per bug 1269209 comment 12. When this is done, the following tests from editor/libeditor/tests/ should be movable to mochitest-plain:
* test_bug490879.xul
* test_bug646194.xul
* test_bug1101392.html
* test_bug1140617.xul
* test_bug1153237.html
* test_bug1248128.html (this one might work with synthesizeKey, but I don't see any reason to bother)
* test_bug1248185.html
* test_selection_move_commands.xul
Comment 2•8 years ago
|
||
(In reply to :Aryeh Gregor (working until September 2) from comment #1)
> Do you know how I'd go about doing this?
It looks like you should be able to add a function that gets the docShell out of the window and uses its doCommand function (docShell is chrome-only, so you'll have to add this to specialpowersAPI.js), it'd be something like
SpecialPowers.prototype = {
...
doCommand(cmd) {
this._getDocShell().doCommand(cmd);
},
};
And replacing (e.g. from test_selection_move_commands.xul);
function doCommand(cmd) {...}
with
function doCommand(cmd) {
return SpecialPowers.doCommand(cmd);
}
(code completely untested, follow advice at own risk :-)).
Flags: needinfo?(mrbkap)
Assignee | ||
Comment 3•8 years ago
|
||
Great, that worked perfectly (with slight adjustment)! Next question: what do I do about popupNode?
http://searchfox.org/mozilla-central/rev/064025c802c2/editor/libeditor/tests/test_bug490879.xul#49-50,58
Flags: needinfo?(mrbkap)
Comment 4•8 years ago
|
||
(In reply to Aryeh Gregor (:ayg) (working until September 2) from comment #3)
> Great, that worked perfectly (with slight adjustment)! Next question: what
> do I do about popupNode?
You should be able to do SpecialPowers._getDocShell().contentViewer.QueryInterface(SpecialPowers.Ci.nsIContentViewerEdit).setCommandNode(node);
(Hopefully cleaned up a bunch.)
and then use your doCommand function as you would otherwise.
Flags: needinfo?(mrbkap)
Assignee | ||
Comment 5•8 years ago
|
||
Okay, so my patch set is working almost perfectly (yay!). There's just one more test failure I can't figure out:
https://treeherder.mozilla.org/logviewer.html#?job_id=26668322&repo=try#L13163
The failure is presumably due to the test that was run immediately before, editor/libeditor/tests/test_selection_move_commands.html, because the test where the failure was reported a) wasn't touched and b) doesn't seem like it could possibly cause the failure. But test_selection_move_commands.html does seem to do the right thing, I think:
https://hg.mozilla.org/try/rev/9e77c4776ea2ca60f8d174f4e45dfc9f55247da1#l3.206
Any ideas?
Flags: needinfo?(mrbkap)
Comment 6•8 years ago
|
||
(In reply to Aryeh Gregor (:ayg) (working until September 1) from comment #5)
> Any ideas?
If I'm reading the test correctly, it looks likes we're trying to call nsIDOMWindowUtils.restoreNormalRefresh from within execTests. However, all calls to execTest's generator are surrounded by calls to nsIDOMWindowUtils.advanceTimeAndRefresh meaning that we'll restore the normal refresh driver (before we call SimpleTest.finish) and then re-grab control of the refresh driver before leaving the test but after the checks caused by SimpleTest.finish.
I believe that this can be fixed by changing the control flow a bit (and using SpawnTask.js [1]):
SimpleTest.waitForExplicitFinish();
function* setup() {
yield SpecialPowers.pushPrefEnv(...);
winUtils.advanceTimeAnd....;
}
function* runTests() {
// the current exec tests, but ending with:
yield SpecialPowers.pushPrefEnv({set: [["layout..."]]});
runSelectionTests(body, 1);
yield SpecialPowers.pushPrefEnv(...);
runSelectionTests(...);
}
function cleanup() {
winUtils.restoreNormalRefresh();
SimpleTest.finish();
}
function* test_runner() {
let curTest = runTests();
while (true) {
winUtils.advanceTimeAndRefresh(100);
if (curTest.next().done) {
break;
}
winUtils.advanceTimeAndRefresh(100);
yield new Promise(resolve => {
setTimeout(resolve, 20);
});
}
}
spawn_task(setup);
spawn_task(test_runner);
spawn_task(cleanup);
Another thing to try would be to see if that setTimeout is really necessary. If all we need to do is return to the event loop, you could even possibly get rid of test_runner and use add_task:
add_task(setup);
add_task(runTests);
add_task(cleanup);
(removing the SimpleTest.waitForExplicitFinish and SimpleTest.finish calls from their respective functions).
That way, the control flow clearly enters the test and exits without accidentally re-setting anything. Of course, I haven't actually tried any of this.
[1] http://searchfox.org/mozilla-central/rev/6536590412ea212c291719d1ed226fae65a0f917/testing/mochitest/tests/SimpleTest/SpawnTask.js
Flags: needinfo?(mrbkap)
Assignee | ||
Comment 7•8 years ago
|
||
Green try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bfea11688d89&exclusion_profile=false
This is blocked on my getting MozReview working so I can get review.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 17•8 years ago
|
||
mozreview-review |
Comment on attachment 8804689 [details]
Bug 1271119 - Add SpecialPowers.doCommand() and .setCommandNode();
https://reviewboard.mozilla.org/r/88622/#review87802
Attachment #8804689 -
Flags: review?(mrbkap) → review+
Comment 18•8 years ago
|
||
mozreview-review |
Comment on attachment 8804690 [details]
Bug 1271119 - Port test_selection_move_commands.xul from chrome to plain;
https://reviewboard.mozilla.org/r/88624/#review87882
Looks like that you removed test_selection_move_commands.xul and create test_selection_move_commands.html. Therefore, I cannot check the diff of removed file and added file. Additionally, this means that the commit history would be gone if we'd land this changeset as is.
Please use |hg rename editor/libeditor/tests/test_selection_move_commands.xul editor/libeditor/tests/test_selection_move_commands.html| and modify the html file.
Attachment #8804690 -
Flags: review?(masayuki) → review-
Comment 19•8 years ago
|
||
mozreview-review |
Comment on attachment 8804691 [details]
Bug 1271119 - Port test_bug490879.xul from chrome to plain;
https://reviewboard.mozilla.org/r/88626/#review87884
Same, please use |hg rename| and keep the commit history of the file and show me the diff.
Attachment #8804691 -
Flags: review?(masayuki) → review-
Comment 20•8 years ago
|
||
mozreview-review |
Comment on attachment 8804692 [details]
Bug 1271119 - Port test_bug646194.xul from chrome to mochitest;
https://reviewboard.mozilla.org/r/88628/#review87886
Same, please use |hg rename| and keep the commit history of the file and show me the diff.
Attachment #8804692 -
Flags: review?(masayuki) → review-
Comment 21•8 years ago
|
||
mozreview-review |
Comment on attachment 8804694 [details]
Bug 1271119 - Port test_bug1140617.xul from chrome to plain;
https://reviewboard.mozilla.org/r/88632/#review87888
Same, please use |hg rename| and keep the commit history of the file and show me the diff.
Attachment #8804694 -
Flags: review?(masayuki) → review-
Comment 22•8 years ago
|
||
mozreview-review |
Comment on attachment 8804695 [details]
Bug 1271119 - Port test_bug1153237.html from chrome to plain;
https://reviewboard.mozilla.org/r/88634/#review87952
Attachment #8804695 -
Flags: review?(masayuki) → review+
Comment 23•8 years ago
|
||
mozreview-review |
Comment on attachment 8804697 [details]
Bug 1271119 - Port test_bug1248185.html from chrome to plain;
https://reviewboard.mozilla.org/r/88638/#review87956
Attachment #8804697 -
Flags: review?(masayuki) → review+
Comment 24•8 years ago
|
||
mozreview-review |
Comment on attachment 8804693 [details]
Bug 1271119 - Port test_bug1101392.html from chrome to plain;
https://reviewboard.mozilla.org/r/88630/#review87960
Attachment #8804693 -
Flags: review?(masayuki) → review+
Comment 25•8 years ago
|
||
mozreview-review |
Comment on attachment 8804696 [details]
Bug 1271119 - Port test_bug1248128.html from chrome to plain;
https://reviewboard.mozilla.org/r/88636/#review87964
You inserted Tabs into the test, please replace them with whitespaces. (I cannot point them because if I pointed them from MozReview, it causes internal server error (500).)
Attachment #8804696 -
Flags: review?(masayuki) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 34•8 years ago
|
||
mozreview-review |
Comment on attachment 8804690 [details]
Bug 1271119 - Port test_selection_move_commands.xul from chrome to plain;
https://reviewboard.mozilla.org/r/88624/#review88322
Attachment #8804690 -
Flags: review?(masayuki) → review+
Comment 35•8 years ago
|
||
mozreview-review |
Comment on attachment 8804691 [details]
Bug 1271119 - Port test_bug490879.xul from chrome to plain;
https://reviewboard.mozilla.org/r/88626/#review88328
Attachment #8804691 -
Flags: review?(masayuki) → review+
Comment 36•8 years ago
|
||
mozreview-review |
Comment on attachment 8804692 [details]
Bug 1271119 - Port test_bug646194.xul from chrome to mochitest;
https://reviewboard.mozilla.org/r/88628/#review88332
Attachment #8804692 -
Flags: review?(masayuki) → review+
Comment 37•8 years ago
|
||
mozreview-review |
Comment on attachment 8804694 [details]
Bug 1271119 - Port test_bug1140617.xul from chrome to plain;
https://reviewboard.mozilla.org/r/88632/#review88336
::: editor/libeditor/tests/test_bug1140617.html:11
(Diff revision 2)
> - function pasteIntoAndCheck() {
> + SpecialPowers.setCommandNode(window, document.getElementById("i"));
> + SpecialPowers.doCommand(window, "cmd_copyImageContents");
Just I wonder, if SpecialPowers.setCommandNode() is always used with SpecialPowers.doCommand(), it could be simpler to add optional argument to doCommand(). But I guess that separated API may be clearer.
Thank you very much for your work on editor tests. I learned a lot from your patch!
Attachment #8804694 -
Flags: review?(masayuki) → review+
Comment 38•8 years ago
|
||
Pushed by ayg@aryeh.name:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ceb151e3e51
Add SpecialPowers.doCommand() and .setCommandNode(); r=mrbkap
https://hg.mozilla.org/integration/mozilla-inbound/rev/26187d6585ae
Port test_selection_move_commands.xul from chrome to plain; r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/8fe257d9c243
Port test_bug490879.xul from chrome to plain; r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae9a33708ecc
Port test_bug646194.xul from chrome to mochitest; r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/1da8e3d851c6
Port test_bug1101392.html from chrome to plain; r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c540c446896
Port test_bug1140617.xul from chrome to plain; r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/62c0f7ba9e59
Port test_bug1153237.html from chrome to plain; r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f480c1204e8
Port test_bug1248128.html from chrome to plain; r=masayuki
https://hg.mozilla.org/integration/mozilla-inbound/rev/e688081176ae
Port test_bug1248185.html from chrome to plain; r=masayuki
Assignee | ||
Comment 39•8 years ago
|
||
Green try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe56506f5253 The failures are not in files touched by this bug.
Comment 40•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9ceb151e3e51
https://hg.mozilla.org/mozilla-central/rev/26187d6585ae
https://hg.mozilla.org/mozilla-central/rev/8fe257d9c243
https://hg.mozilla.org/mozilla-central/rev/ae9a33708ecc
https://hg.mozilla.org/mozilla-central/rev/1da8e3d851c6
https://hg.mozilla.org/mozilla-central/rev/6c540c446896
https://hg.mozilla.org/mozilla-central/rev/62c0f7ba9e59
https://hg.mozilla.org/mozilla-central/rev/8f480c1204e8
https://hg.mozilla.org/mozilla-central/rev/e688081176ae
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Updated•8 years ago
|
Assignee: nobody → ayg
You need to log in
before you can comment on or make changes to this bug.
Description
•