Closed
Bug 1433648
Opened 7 years ago
Closed 7 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)
Thunderbird
Testing Infrastructure
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 60.0
People
(Reporter: jorgk-bmo, Assigned: aceman)
References
Details
(Whiteboard: [Thunderbird-testfailure: Z all][Thunderbird-temporary-fix])
Attachments
(2 files, 1 obsolete file)
1.15 KB,
patch
|
Details | Diff | Splinter Review | |
18.86 KB,
patch
|
aceman
:
review+
|
Details | Diff | Splinter Review |
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)
Reporter | ||
Updated•7 years ago
|
Whiteboard: [Thunderbird-testfailure: Z all]
Reporter | ||
Comment 1•7 years ago
|
||
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.
Reporter | ||
Comment 2•7 years ago
|
||
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.
Reporter | ||
Comment 4•7 years ago
|
||
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)
Reporter | ||
Comment 5•7 years ago
|
||
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
Comment 7•7 years ago
|
||
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)
Updated•7 years ago
|
Flags: needinfo?(bugs)
Comment 8•7 years ago
|
||
I got it.
https://searchfox.org/comm-central/source/mail/test/resources/mozmill/mozmill/extension/resource/stdlib/EventUtils.js#338-342,345
mozmill's EventUtils still uses obsolete API, nsIDOMWindowUtils.sendKeyEvent().
related to bug 1134540 and bug 1134542.
It should be rewritten with new API, nsITextInputProcessor like EventUtils for Mochitest:
https://searchfox.org/mozilla-central/rev/11d0ff9f36465ce19b0c43d1ecc3025791eeb808/testing/mochitest/tests/SimpleTest/EventUtils.js#873-905
Reporter | ||
Comment 9•7 years ago
|
||
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)
Comment 10•7 years ago
|
||
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.
Assignee | ||
Comment 11•7 years ago
|
||
>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.
Comment 12•7 years ago
|
||
I'll remove nsIDOMWindowUtils.sendKeyEvent(). So, please update mozmill's EventUtils.js. (see also bug 1434317)
Blocks: 1134542
Assignee | ||
Comment 13•7 years ago
|
||
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)
Reporter | ||
Comment 14•7 years ago
|
||
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+
Assignee | ||
Comment 15•7 years ago
|
||
(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 16•7 years ago
|
||
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+
Assignee | ||
Comment 17•7 years ago
|
||
Good idea, thanks.
Assignee: nobody → acelists
Attachment #8947182 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8947351 -
Flags: review+
Keywords: leave-open → checkin-needed
OS: Unspecified → All
Hardware: Unspecified → All
Version: unspecified → Trunk
Comment 18•7 years ago
|
||
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
Reporter | ||
Comment 19•7 years ago
|
||
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.
Description
•