Create compose test for the following: reply/forward/etc. 1) without viewing message first 2) with charset override 3) with default charset enforced.

RESOLVED FIXED in Thunderbird 53.0

Status

MailNews Core
Composition
RESOLVED FIXED
5 months ago
5 months ago

People

(Reporter: Jorg K (GMT+2), Assigned: Jorg K (GMT+2))

Tracking

Trunk
Thunderbird 53.0
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird51 fixed, thunderbird52 fixed, thunderbird53 fixed)

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Assignee)

Description

5 months ago
+++ This bug was initially created as a clone of Bug #1323377 +++

See bug 1323377 comment #82.

Create compose test for the following:

Reply/forward/etc. ...
1) without viewing message first
2) with charset override
3) with default charset enforced.

The intent is to enhance test-reply-multipart-charset.js.
(Assignee)

Comment 1

5 months ago
Created attachment 8822083 [details] [diff] [review]
WIP: 1325967-charset-tests.patch (v1).

Just so that Magnus can see that he will get his tests ;-)
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
(Assignee)

Comment 2

5 months ago
Created attachment 8822091 [details] [diff] [review]
1325967-charset-tests.patch (v2).

Hi Magnus, I hope this is what you had in mind. I've added a bunch of tests, all pass after bug 1323377 landed (as expected). Anything else you'd like to cover here?

I'll let Aceman do the review since he knows best what menu-clicking works and what doesn't on the various platforms.
Attachment #8822083 - Attachment is obsolete: true
Attachment #8822091 - Flags: review?(acelists)
Attachment #8822091 - Flags: feedback?(mkmelin+mozilla)

Comment 3

5 months ago
Comment on attachment 8822091 [details] [diff] [review]
1325967-charset-tests.patch (v2).

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

::: mail/test/mozmill/composition/test-reply-multipart-charset.js
@@ +31,5 @@
>  }
>  
> +function subtest_replyEditAsNewForward_charset(aAction, aFile, aCharset,
> +                                               aOverride = null,
> +                                               aNotViewed = false) {

If the default is false, maybe naming the argument aViewed = true would be better to avoid double negation?

@@ +53,5 @@
>    close_window(msgc);
>  
>    let msg = select_click_row(0);
> +  if (!aNotViewed) {
> +    // If it's not viewed, we can't check the following.

Maybe if you are describing a case when the conditions is NOT fulfilled ("not viewed"=aNotViewed(true)) the comment shouldn't be inside the if block?

@@ +60,5 @@
> +
> +  if (aOverride) {
> +    // Display the message using the override charset.
> +    mc.click(mc.eid("menu_View"));
> +    mc.click_menus_in_sequence(mc.e("menu_View_Popup"), [

This looks fine for Win/Linux. Please check the menu items are the same on Mac. Or even if you can operate the main menu items on Mac, I think there were some problems with that. Do a try run.

@@ +103,5 @@
>  
>    // Check that a UTF-16 encoded e-mail is forced to UTF-8 when replying.
>    subtest_replyEditAsNewForward_charset(1, "./body-utf16.eml", "UTF-8");
> +
> +  // Check that the override is honoured.

Please mention bug 1323377 somewhere.
Attachment #8822091 - Flags: review+
(Assignee)

Comment 4

5 months ago
Created attachment 8822097 [details] [diff] [review]
1325967-charset-tests.patch (v3).

Thanks for the quick review. Issues addressed. Here's a try run:

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=3e5f2ad5823e2d9940ab9ec5d4dbc9685892324c
Attachment #8822091 - Attachment is obsolete: true
Attachment #8822091 - Flags: review?(acelists)
Attachment #8822091 - Flags: feedback?(mkmelin+mozilla)
Attachment #8822097 - Flags: review+

Comment 5

5 months ago
Comment on attachment 8822097 [details] [diff] [review]
1325967-charset-tests.patch (v3).

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

::: mail/test/mozmill/composition/test-reply-multipart-charset.js
@@ +107,3 @@
>    subtest_replyEditAsNewForward_charset(1, "./body-utf16.eml", "UTF-8");
> +
> +  // Check that the override is honoured (inspired by bug 1323377).

You could split the bug 1323377 tests into a new test_function so that this one is not a "mixed bag" and also failure reports are more precise in the future.
(Assignee)

Comment 6

5 months ago
Good idea. Will do before landing, but let's see whether the test works on Linux/Mac.
(Assignee)

Comment 7

5 months ago
Created attachment 8822100 [details] [diff] [review]
1325967-charset-tests.patch (v4).

Split into multiple tests now.
Attachment #8822097 - Attachment is obsolete: true
Attachment #8822100 - Flags: review+

Comment 8

5 months ago
So, it failed on OS X. Is there another menu where you could set the charset? E.g. the context menu?
(Assignee)

Comment 9

5 months ago
Yes, it failed, sadly the opt run gives no details, so I'll run it again in debug mode. No, it's not on the context menu.

I'm not really surprised given:
https://dxr.mozilla.org/comm-central/search?q=On+OS+X+the+main+menu+seems+not+accessible+for+clicking+from+mozmill.&redirect=false

Let's see again:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a4d2879fe45ae8a62ccbc7587524584526706466
(Assignee)

Comment 10

5 months ago
After splitting test, I get:
TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/composition/test-reply-multipart-charset.js | test-reply-multipart-charset.js::test_replyEditAsNewForward_override
TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/composition/test-reply-multipart-charset.js | test-reply-multipart-charset.js::test_replyEditAsNewForward_noPreview

INFO -  SUMMARY-UNEXPECTED-FAIL | test-reply-multipart-charset.js | test-reply-multipart-charset.js::test_replyEditAsNewForward_override
INFO -    EXCEPTION: Popup never opened! id=menu_View_Popup, state=closed

INFO -  SUMMARY-UNEXPECTED-FAIL | test-reply-multipart-charset.js | test-reply-multipart-charset.js::test_replyEditAsNewForward_noPreview
INFO -    EXCEPTION: Popup never opened! id=menu_View_Popup, state=closed

This is the code:

+    mc.click(mc.eid("menu_View"));
+    mc.click_menus_in_sequence(mc.e("menu_View_Popup"), [
+      {label: "Text Encoding"},
+      {label: aOverride},
+    ]);

+  mc.click(mc.eid("menu_View"));
+  mc.click_menus_in_sequence(mc.e("menu_View_Popup"), [
+    {label: "Layout"},
+    {label: "Message Pane"},
+  ]);


So we click the View menu and from the popup we select "Text Encoding" followed by a charset being passed in, or in the second case, "Layout" followed by "Message Pane".

Aceman, what do you suggest here? How do you feel about skipping those tests on Mac? If ever there were a regression, the other two platforms would surely detect it. Since neither me nor you have Mac hardware, I think the little gain to he had here doesn't warrant the effort of trying to get this to work via try pushes.

For choosing the encoding, I've already tried to copy MailSetCharacterSet(), like so:
mc.window.mailCharacterSet = XXX";
mc.window.charsetOverride = true;
mc.messageDisplay.keyForCharsetOverride = mc.messageDisplay.displayedMessage.messageKey;
but at least the third line didn't work.
If I read correctly, mc.messageDisplay should give access to gMessageDisplay.
Flags: needinfo?(acelists)
(Assignee)

Comment 11

5 months ago
Correction, I put:
  mc.window.mailCharacterSet = aOverride;
  mc.window.charsetOverride = true;
  mc.messageDisplay.keyForCharsetOverride = mc.messageDisplay.displayedMessage.messageKey;
and that doesn't complain, but the test fails with:
  Compose window has the wrong charset: 'euc-kr' != 'utf-8'.
since the UTF-8 override doesn't stick somehow.

Comment 12

5 months ago
Comment on attachment 8822100 [details] [diff] [review]
1325967-charset-tests.patch (v4).

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

Yes something like this.

::: mail/test/mozmill/composition/body-greek.eml
@@ +5,5 @@
> +MIME-Version: 1.0
> +Content-Type: text/plain; charset=ISO-8859-7;
> +Content-Transfer-Encoding: 8bit
> +
> +This is no Greek ;-)

Why not? It can be pretty confusing to test what's right, when the text is actually wrong.
(Assignee)

Comment 13

5 months ago
(In reply to Magnus Melin from comment #12)
> > +This is no Greek ;-)
> Why not? It can be pretty confusing to test what's right, when the text is
> actually wrong.
The test isn't wrong. We use a Greek ISO charset to encode some ASCII text. That's perfectly valid. What counts is that the software applies the right charset. And it does.

Comment 14

5 months ago
Yes but that's by accident, so you couldn't visually check if it's correct or not.
(Assignee)

Comment 15

5 months ago
OK, I'll give you some Greek text in the final version.

Using |App menu > View > Text Encoding > ...| works but sadly there is no |View > Layout|. Any idea how to switch off the preview pane, right, pressing "F8" as least on Windows.
(Assignee)

Comment 16

5 months ago
Created attachment 8822208 [details] [diff] [review]
1325967-charset-tests.patch (v5).

In this patch:
1) The Greek message contains some Greek text.
   This turns the whole patch into ISO-8859-7 and I think
   we will have lots of problems with this since patches are
   normally UTF-8. Even ANSI (with "ü" for Düllmann) already
   causes problems. I think the review tool etc. will just fail.
   More trouble than it's worth.
2) Used the app menu to select the encoding. Let's see whether this
   works on the other platforms.
3) Used mc.window.goDoCommand("cmd_toggleMessagePane"); to switch off
   the message pane.
Attachment #8822100 - Attachment is obsolete: true
Flags: needinfo?(acelists)
(Assignee)

Comment 17

5 months ago
Created attachment 8822211 [details] [diff] [review]
1325967-charset-tests.patch (v5b).

Added a warning in the ISO-8859-7 encoded file and added white-space where it had gone missing.

Note that the correct text is: Καλησπέρα and not what the review shows: ÊáëçóðÝñá.

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c15eb13e203ded9b824cc4e2c2481a712e24adf8

The changeset:
https://hg.mozilla.org/try-comm-central/rev/edd3da8b8463f0887d0e23783700523f84ee2e26
looks are garbled, since it's viewed as UTF-8 by default. If you switch the encoding to "Greek (ISO)" in FF (assuming that FF is your browser), then it looks OK.

I guess this is r+ since it was r+ before and I just did the enhancements which people wanted.
Attachment #8822208 - Attachment is obsolete: true
Attachment #8822211 - Flags: review+
(Assignee)

Comment 18

5 months ago
Created attachment 8822215 [details] [diff] [review]
1325967-charset-tests.patch (v5c).

Aceman is watching and he doesn't like ISO-8859-7 encoded patches. OK, here's base64 ;-)

You can stick your seal of approval onto it again.
Attachment #8822211 - Attachment is obsolete: true
Attachment #8822215 - Flags: review?(acelists)
> here's base64 ;-)
FYI, for future test case.
Quoted-printable is another way. Can be generated by ascii text editor only.
(Assignee)

Comment 20

5 months ago
Aceman, that try is green, so kindly review again.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c15eb13e203ded9b824cc4e2c2481a712e24adf8

I'd like to land this after the next M-C merge.

Comment 21

5 months ago
Comment on attachment 8822215 [details] [diff] [review]
1325967-charset-tests.patch (v5c).

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

I also proposed quoted-printable as it is better readable...

Good that it passes now on all platforms, thanks.

::: mail/test/mozmill/composition/test-reply-multipart-charset.js
@@ +9,3 @@
>   * Tests that the reply to a message picks up the charset from the body
>   * and not from an attachment. Also test "Edit as new", forward inline and
>   * forward as attachment.

I would put these comment blocks before the respective test_functions.

@@ +10,5 @@
>   * and not from an attachment. Also test "Edit as new", forward inline and
>   * forward as attachment.
> + *
> + * Bug 961983:
> + * Tests that UFT-16 is not used in a composition.

UTF-16
Attachment #8822215 - Flags: review?(acelists) → review+
(Assignee)

Comment 22

5 months ago
(In reply to :aceman from comment #21)
> I also proposed quoted-printable as it is better readable...
You're sure that Greek text will be readable? Where do I find a QP encoder which accepts an input charset like ISO-8859-7. There are a few online base64 encoders, but most don't do ISO-8859-7. In the end, I found one.

> I would put these comment blocks before the respective test_functions.
Well, in the header of the test you usually have a block explaining what it does. Since this ended up being a mixed bag of stuff, the header block is a little longer. Each function also has its comment.

> UTF-16
Thanks.
(Assignee)

Comment 23

5 months ago
OK, found one: http://www.webatic.com/run/convert/qp.php

Do you prefer this?
Here comes some Greek text: =CA=E1=EB=E7=F3=F0=DD=F1=E1
(Assignee)

Comment 24

5 months ago
Created attachment 8822249 [details] [diff] [review]
1325967-charset-tests.patch (v5d).

QP by popular demand ;-)
Attachment #8822215 - Attachment is obsolete: true
Attachment #8822249 - Flags: review+

Comment 25

5 months ago
(In reply to Jorg K (GMT+1) from comment #23)
> OK, found one: http://www.webatic.com/run/convert/qp.php
> 
> Do you prefer this?
> Here comes some Greek text: =CA=E1=EB=E7=F3=F0=DD=F1=E1

While I can't display it in TB now, syntactically this looks like what I had in mind. Thanks!
(Assignee)

Comment 26

5 months ago
Comment on attachment 8822249 [details] [diff] [review]
1325967-charset-tests.patch (v5d).

Let's take this to Aurora and Beta since that's where bug 1323377 will go.
Attachment #8822249 - Flags: approval-comm-beta+
Attachment #8822249 - Flags: approval-comm-aurora+
(Assignee)

Updated

5 months ago
Keywords: checkin-needed
(Assignee)

Comment 27

5 months ago
https://hg.mozilla.org/comm-central/rev/a81efb5860a39074da8567637669363e17524b0f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-thunderbird51: --- → affected
status-thunderbird52: --- → affected
status-thunderbird53: --- → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 53.0
(Assignee)

Comment 28

5 months ago
Aurora (TB 52):
https://hg.mozilla.org/releases/comm-aurora/rev/9152817cd3676dd5d9ca64bee508c36d67872a6d
status-thunderbird52: affected → fixed
(Assignee)

Comment 29

5 months ago
Beta (TB 51):
https://hg.mozilla.org/releases/comm-beta/rev/26265ec715cf1ffa58436256667ebc108c90eebd
status-thunderbird51: affected → fixed
(Assignee)

Comment 30

5 months ago
Aceman, can you please advise me here. I landed this on beta
https://treeherder.mozilla.org/#/jobs?repo=comm-beta&revision=94c29c2b3a6ff6ac8dffae2d5fa915673f8c476a
and now Mozmill fails (works OK on C-A and C-C).

Here's the failure:
07:14:28     INFO -  SUMMARY-PASS | test-reply-multipart-charset.js::setupModule
07:14:28     INFO -  SUMMARY-PASS | test-reply-multipart-charset.js::test_replyEditAsNewForward_charsetFromBody
07:14:28     INFO -  SUMMARY-PASS | test-reply-multipart-charset.js::test_reply_noUTF16
07:14:28     INFO -  SUMMARY-PASS | test-reply-multipart-charset.js::test_replyEditAsNewForward_override
07:14:28     INFO -  SUMMARY-PASS | test-reply-multipart-charset.js::test_replyEditAsNewForward_enforceDefault
07:14:28     INFO -  SUMMARY-UNEXPECTED-FAIL | test-reply-multipart-charset.js | test-reply-multipart-charset.js::test_replyEditAsNewForward_noPreview
07:14:28     INFO -    EXCEPTION: Timed out waiting for message display completion.
07:14:28     INFO -      at: utils.js line 447
07:14:28     INFO -         TimeoutError utils.js:447 13
07:14:28     INFO -         waitFor utils.js:485 11
07:14:28     INFO -         wait_for_message_display_completion test-folder-display-helpers.js:1619 3
07:14:28     INFO -         open_message_from_file test-folder-display-helpers.js:651 3
07:14:28     INFO -         subtest_replyEditAsNewForward_charset test-reply-multipart-charset.js:52 14
07:14:28     INFO -         test_replyEditAsNewForward_noPreview test-reply-multipart-charset.js:148 3
07:14:28     INFO -         Runner.prototype.wrapper frame.js:585 9
07:14:28     INFO -         Runner.prototype._runTestModule frame.js:655 9
07:14:28     INFO -         Runner.prototype.runTestModule frame.js:701 3
07:14:28     INFO -         Runner.prototype.runTestDirectory frame.js:525 7
07:14:28     INFO -         runTestDirectory frame.js:707 3
07:14:28     INFO -         Bridge.prototype._execFunction server.js:179 10
07:14:28     INFO -         Bridge.prototype.execFunction server.js:183 16
07:14:28     INFO -         Session.prototype.receive server.js:283 3
07:14:28     INFO -         AsyncRead.prototype.onDataAvailable server.js:88 3
07:14:28     INFO -  SUMMARY-PASS | test-reply-multipart-charset.js::teardownModule

What do I do? Here are my options:
1) Back all three changesets out
2) Back out only the test
3) Uplift some Mozmill fixes to beta so that the test will pass.

Here's the log of test-folder-display-helpers.js:
https://hg.mozilla.org/comm-central/log/tip/mail/test/mozmill/shared-modules/test-folder-display-helpers.js

Hmm, somewhere this is waiting for message display completion where the message pane is switched off and the message is not displayed. This code should avoid the problem:
  if (aViewed) {
    // Only if the preview pane is on, we can check the following.
    assert_selected_and_displayed(mc, msg);
  }

I compared test-folder-display-helpers.js on C-B and C-C and didn't find anything relevant, only bug 1312218.
Flags: needinfo?(acelists)

Comment 31

5 months ago
It is failing on line 52, open_message_from_file() ? Is the 'format-flowed.eml' file existing? I do not see it on c-b. It was added in bug 456053.
Flags: needinfo?(acelists)
(Assignee)

Comment 32

5 months ago
Thanks!!! Why doesn't the damn thing say "Can't open file format-flowed.eml"? Grrrr.
(Assignee)

Comment 33

5 months ago
Follow-up for Beta (TB 51): Fixed test:
https://hg.mozilla.org/releases/comm-beta/rev/0aac7dab1fc46f3c9641b24133fe84ae4f827577
You need to log in before you can comment on or make changes to this bug.