Closed Bug 1271119 Opened 8 years ago Closed 8 years ago

Port doCommand() to SpecialPowers to allow executing arbitrary editor commands from mochitest-plain

Categories

(Testing :: Mochitest, defect)

Product:
Testing
Component:
Mochitest
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox52 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: ayg, Assigned: ayg)

References

(Blocks 2 open bugs)

Details

Attachments

(9 files)

Bug 1271119 - Add SpecialPowers.doCommand() and .setCommandNode();
58 bytes, text/x-review-board-request
mrbkap
: review+
Details
Bug 1271119 - Port test_selection_move_commands.xul from chrome to plain;
58 bytes, text/x-review-board-request
masayuki
: review+
Details
Bug 1271119 - Port test_bug490879.xul from chrome to plain;
58 bytes, text/x-review-board-request
masayuki
: review+
Details
Bug 1271119 - Port test_bug646194.xul from chrome to mochitest;
58 bytes, text/x-review-board-request
masayuki
: review+
Details
Bug 1271119 - Port test_bug1101392.html from chrome to plain;
58 bytes, text/x-review-board-request
masayuki
: review+
Details
Bug 1271119 - Port test_bug1140617.xul from chrome to plain;
58 bytes, text/x-review-board-request
masayuki
: review+
Details
Bug 1271119 - Port test_bug1153237.html from chrome to plain;
58 bytes, text/x-review-board-request
masayuki
: review+
Details
Bug 1271119 - Port test_bug1248128.html from chrome to plain;
58 bytes, text/x-review-board-request
masayuki
: review+
Details
Bug 1271119 - Port test_bug1248185.html from chrome to plain;
58 bytes, 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
Do you know how I'd go about doing this?
Flags: needinfo?(mrbkap)
(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)
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)
(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)
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)
(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)
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 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 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 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 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 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 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 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 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 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 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 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 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 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+
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
Green try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe56506f5253  The failures are not in files touched by this bug.
Assignee: nobody → ayg
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: