Closed Bug 1433648 Opened 2 years ago Closed 2 years ago

TEST-UNEXPECTED-FAIL | C:\slave\test\build\tests\mozmill\cloudfile\test-cloudfile-attachment-urls.js - five failing tests

Categories

(Thunderbird :: Testing Infrastructure, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 60.0

People

(Reporter: jorgk, Assigned: aceman)

References

Details

(Whiteboard: [Thunderbird-testfailure: Z all][Thunderbird-temporary-fix])

Attachments

(2 files, 1 obsolete file)

TEST-UNEXPECTED-FAIL | C:\slave\test\build\tests\mozmill\cloudfile\test-cloudfile-attachment-urls.js | test-cloudfile-attachment-urls.js::test_adding_filelinks_to_nonempty_reply_above
TEST-UNEXPECTED-FAIL | C:\slave\test\build\tests\mozmill\cloudfile\test-cloudfile-attachment-urls.js | test-cloudfile-attachment-urls.js::test_adding_filelinks_to_empty_reply_below
TEST-UNEXPECTED-FAIL | C:\slave\test\build\tests\mozmill\cloudfile\test-cloudfile-attachment-urls.js | test-cloudfile-attachment-urls.js::test_adding_filelinks_to_nonempty_reply_below
TEST-UNEXPECTED-FAIL | C:\slave\test\build\tests\mozmill\cloudfile\test-cloudfile-attachment-urls.js | test-cloudfile-attachment-urls.js::test_adding_filelinks_to_forward
TEST-UNEXPECTED-FAIL | C:\slave\test\build\tests\mozmill\cloudfile\test-cloudfile-attachment-urls.js | test-cloudfile-attachment-urls.js::test_insertion_restores_caret_point 

First seen after the M-C merge this morning.

M-C last good: 6190b1e65676010bc61ca612b14198992a
M-C first bad: 7c637da65c1fec33fc36a014485acad541

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6190b1e65676010bc61ca612b14198992a&tochange=7c637da65c1fec33fc36a014485acad541

https://archive.mozilla.org/pub/thunderbird/nightly/2018/01/2018-01-27-03-02-01-comm-central/comm-central_win7_ix_test-mozmill-bm111-tests1-windows-build191.txt.gz

First error in the log is:
INFO -  SUMMARY-UNEXPECTED-FAIL | C:\slave\test\build\tests\mozmill\cloudfile\test-cloudfile-attachment-urls.js | test-cloudfile-attachment-urls.js::test_adding_filelinks_to_nonempty_reply_above
INFO -    EXCEPTION: a != b: 'This is a line of textand here's another!' != 'and here's another!'.
INFO -      at: test-folder-display-helpers.js line 2917
INFO -         assert_true test-folder-display-helpers.js:2917 11
INFO -         assert_equals test-folder-display-helpers.js:2904 3
INFO -         subtest_adding_filelinks_to_reply_above_plaintext test-cloudfile-attachment-urls.js:462 5

The test "types" two lines, defined in
  var kLines = ["This is a line of text", "and here's another!"];
and later tests for the second of those lines

  } else {
    let targetText = aText[aText.length - 1]; <== last line
    let textNode = br.previousSibling;
    assert_equals(textNode.nodeType, kTextNodeType);
    assert_equals(textNode.nodeValue, targetText); <== failing line 462

I haven't run the test locally, but it seems to come from:
34abf9793163 Masayuki Nakano - Bug 1433101 - part 2: Treat Enter and Shift+Enter as printable key r=smaug
81157078b090 Masayuki Nakano - Bug 1433101 - part 1: Add new pref which disables keypress event for non-printable keys only for the default event group in web content r=smaug

Somehow our way to enter a new line stopped working in Mozmill. I can still create newlines in a Daily with this change, so maybe we need to adjust Mozmill.

Another one for Aceman, so we don't get bored. Any hints from author or reviewer of bug 1433101?
Flags: needinfo?(masayuki)
Flags: needinfo?(bugs)
Flags: needinfo?(acelists)
Whiteboard: [Thunderbird-testfailure: Z all]
I watched the test now and both lines "This is a line of text" and "and here's another!" are entered without newline in between. So we need to tweak the function that does the typing.
Not sure why this stopped working:

function type_in_composer(aController, aText) {
  // If we have any typing to do, let's do it.
  let frame = aController.eid("content-frame");
  for (let [i, aLine] of aText.entries()) {
    aController.type(frame, aLine);
    if (i < aText.length - 1)
      aController.keypress(frame, "VK_RETURN", {});
  }
}
(In reply to Jorg K (GMT+1) from comment #2)
> function type_in_composer(aController, aText) {
>   // If we have any typing to do, let's do it.
>   let frame = aController.eid("content-frame");
>   for (let [i, aLine] of aText.entries()) {
>     aController.type(frame, aLine);

Interestingly .type(aLine + "\n") makes a proper newline.
Based on Aceman's investigation, this makes all the tests pass for me.
Flags: needinfo?(acelists)
Attachment #8946095 - Flags: review?(acelists)
Attachment #8946095 - Flags: feedback?(masayuki)
I'm going to land that one-line hack now, happy to back it out for a better solution.
Keywords: leave-open
Whiteboard: [Thunderbird-testfailure: Z all] → [Thunderbird-testfailure: Z all][Thunderbird-temporary-fix]
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/8e9463f5d78a
type \n instead of sending VK_RETURN. rs=bustage-fix
Perhaps, it doesn't generate correct Enter key event. I guess it doesn't set KeyboardEvent.key value to "Enter", but just set KeyboardEvent.keyCode to KeyboardEvent.DOM_VK_RETURN.
Flags: needinfo?(masayuki)
Comment on attachment 8946095 [details] [diff] [review]
1433648-newline.patch [landed in comment #6 as a temporary fix]

Thanks Masayuki-san for the research. We'll switch to the new interface then (and backout the hack).
Attachment #8946095 - Attachment description: 1433648-newline.patch → 1433648-newline.patch [landed in comment #6 as a temporary fix]
Attachment #8946095 - Flags: review?(acelists)
Attachment #8946095 - Flags: feedback?(masayuki)
FYI: Using nsITextInputProcess means that keypress event is dispatched only when it's necessary. E.g., if preceding keydown event's default is prevented, keypress event is automatically not dispatched.  If given event is a modifier key, keypress event is never fired.  So, if some tests expect such illegal keypress event, you need to fix such tests too.  But I guess that it must be rare.
>if preceding keydown event's default is prevented, keypress event is automatically not dispatched.

It seems to me this is also what the current mozmill code honors.
I'll remove nsIDOMWindowUtils.sendKeyEvent(). So, please update mozmill's EventUtils.js. (see also bug 1434317)
Blocks: 1134542
Thanks for the tip. Using the m-c code seems to work, fixes this test and produces no regressions in other tests: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d6a8a7242e2fe50e4203d50dfd8f7ef6e3f6ca13
I just copied the needed functions. The only changes are the _EU_{cc,ci} which I replaced with the real Components.* because somehow setting the attributes on 'window' didn't work ('window' wasn't found).
Attachment #8947182 - Flags: review?(jorgk)
Attachment #8947182 - Flags: feedback?(masayuki)
Comment on attachment 8947182 [details] [diff] [review]
replace nsIDOMWindowUtils.sendKeyEvent()

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

Thanks for fixing this. Not much to review here if this is a (slightly modified) copy of M-C code.

I'd say: rs=jorgk.

::: mail/test/resources/mozmill/mozmill/extension/resource/stdlib/EventUtils.js
@@ +316,5 @@
>   * a keydown, a keypress and then a keyup event are fired in sequence.
>   *
>   * aWindow is optional, and defaults to the current window object.
>   */
> +function synthesizeKey(aKey, aEvent, aWindow = window, aCallback)

What's the story with the 'aCallback' parameter? Are there any callers providing it?

@@ +831,5 @@
>  
>    return utils.sendSelectionSetEvent(aOffset, aLength, aReverse);
>  }
> +
> +/* This block copied from mozilla-central/testing/mochitest/tests/SimpleTest/EventUtils.js */

Nit: Perhaps add a verb here, like "was".
Attachment #8947182 - Flags: review?(jorgk) → review+
(In reply to Jorg K (GMT+1) from comment #14)
> mail/test/resources/mozmill/mozmill/extension/resource/stdlib/EventUtils.js
> @@ +316,5 @@
> >   * a keydown, a keypress and then a keyup event are fired in sequence.
> >   *
> >   * aWindow is optional, and defaults to the current window object.
> >   */
> > +function synthesizeKey(aKey, aEvent, aWindow = window, aCallback)
> 
> What's the story with the 'aCallback' parameter? Are there any callers
> providing it?

I don't know, we probably do not provide it. I didn't want to change the original code too much by removing it. We may also need it in the future.
Comment on attachment 8947182 [details] [diff] [review]
replace nsIDOMWindowUtils.sendKeyEvent()

Sure, if new lines are just copied from EventUtils.js of mochitest, looks fine.

And perhaps, you should copy the explanation of synthesizeKey() from mochitest's EventUtils.js too. When I make it use nsITextInputProcessor, I added several features to it. So, other developers should read the comment when they use it.
Attachment #8947182 - Flags: feedback?(masayuki) → feedback+
Good idea, thanks.
Assignee: nobody → acelists
Attachment #8947182 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8947351 - Flags: review+
OS: Unspecified → All
Hardware: Unspecified → All
Version: unspecified → Trunk
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/6a1bb92d2e92
Convert mozmill's EventUtils.js from the obsolete nsIDOMWindowUtils.sendKeyEvent() API to nsITextInputProcessor. r=jorgk
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
I've taken the liberty to make this comment block nicer:
https://hg.mozilla.org/comm-central/rev/6a1bb92d2e92#l2.126
Target Milestone: --- → Thunderbird 60.0
You need to log in before you can comment on or make changes to this bug.