Closed Bug 1325967 Opened 7 years ago Closed 7 years ago

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

Categories

(MailNews Core :: Composition, defect)

Product:
MailNews Core
Component:
Composition
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(thunderbird51 fixed, thunderbird52 fixed, thunderbird53 fixed)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 53.0
Tracking Flags:
Tracking Status
thunderbird51 --- fixed
thunderbird52 --- fixed
thunderbird53 --- fixed

People

(Reporter: jorgk-bmo, Assigned: jorgk-bmo)

References

Details

Attachments

(1 file, 7 obsolete files)

1325967-charset-tests.patch (v5d).
6.64 KB, patch
jorgk-bmo
: review+
jorgk-bmo
: approval-comm-aurora+
jorgk-bmo
: approval-comm-beta+
Details | Diff | Splinter Review 
+++ 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.
Just so that Magnus can see that he will get his tests ;-)
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
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 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+
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 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.
Good idea. Will do before landing, but let's see whether the test works on Linux/Mac.
Split into multiple tests now.
Attachment #8822097 - Attachment is obsolete: true
Attachment #8822100 - Flags: review+
So, it failed on OS X. Is there another menu where you could set the charset? E.g. the context menu?
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
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)
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 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.
(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.
Yes but that's by accident, so you couldn't visually check if it's correct or not.
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.
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)
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+
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.
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 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+
(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.
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
QP by popular demand ;-)
Attachment #8822215 - Attachment is obsolete: true
Attachment #8822249 - Flags: review+
(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!
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+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/a81efb5860a39074da8567637669363e17524b0f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-thunderbird51: --- → affected
status-thunderbird52: --- → affected
status-thunderbird53: --- → fixed
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 53.0
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)
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)
Thanks!!! Why doesn't the damn thing say "Can't open file format-flowed.eml"? Grrrr.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: