1670307 - Implement mochitests for the Message Security popup pane

Status: RESOLVED FIXED

(MailNews Core :: Security: OpenPGP, enhancement, P1)

Description

[Alessandro Castellani (:aleca)]

Bug 1647039 improved the UI of the Message Security status, converting the area from a popup dialog to a popup panel, and implementing icons and better visual feedback.
The new panel also is shared between OpenPGP and S/MIME, and the UI elements are hidden/revealed accordingly.

We should implement some tests to cover these scenarios.

Comment 1

[Alessandro Castellani (:aleca)]

Attachment: 1670307-message-security-tests.diff

This patch implements 3 tests:

• Select S/MIME message and confirm correct label and inability to decrypt.
• Select OpenPGP message and confirm correct label and decryption status.
• Select Broken MS-Exchange message and confirm repair process.

All the messages are loaded in a folder to cover this scenario since the browser_viewMessage.js tests deal with messages directly opened from a file.

I have a little bit of an issue with the broken MS-Exchange test.
I had to add a sleep(100) to wait for the repair to conclude, which is strange because the notification should disappear only at the end of the process, which happens correctly when manually testing the repair process, but the test seems to highlight a timing/race issue.
I'm having issues identifying where the problem might be.

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 9185271 [details] [diff] [review]
1670307-message-security-tests.diff

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

::: mail/test/browser/openpgp/browser_viewMessageSecurity.js
@@ +113,5 @@
> +    )
> +  );
> +
> +  // Add the fetched OpenPGP message to the inbox folder.
> +  EnigmailCompat.copyFileToMailFolder(

This is not sync (uses listener), so you'll need to make sure copying concluded before continuing

Comment 3

[Alessandro Castellani (:aleca)]

Attachment: 1670307-message-security-tests.diff

Thanks for the heads up on the missing listener, I completely missed that.

I also found out the issue I was having when testing the broken exchange message and why it wasn't working without an mc.sleep().
The Exchange fixing method literally copies the decrypted body and creates a duplicated message. So I needed to wait for the message to be fully displayed before checking for the encryption icon.

Now the test works without any manual timeout.
I'm waiting for c-c to be fixed before launching a try-run.

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 9185570 [details] [diff] [review]
1670307-message-security-tests.diff

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

Looks pretty good

::: mail/test/browser/openpgp/browser_viewMessageSecurity.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/**
> + * Tests for the display of the Message Security popup panel, which displays
> + * encryption information for both OpenPGP and S/MIME protocol.

Nit: Remove "protocol".

@@ +141,5 @@
> +    () => copyListener.copyDone,
> +    "Timeout waiting for copy to complete",
> +    5000,
> +    100
> +  );

You can do all of this much easier and tidier: like https://searchfox.org/comm-central/rev/bb696b3c1cdd63957cd995bdb50edb40c13ffbc1/mailnews/local/test/unit/test_pop3MultiCopy.js#60-70

Comment 5

[Alessandro Castellani (:aleca)]

Attachment: 1670307-message-security-tests.diff

Nice, using the PromiseCopyListener makes it easier to read.

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 9185622 [details] [diff] [review]
1670307-message-security-tests.diff

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

LGTM, thx! r=mkmelin

Comment 7

[Alessandro Castellani (:aleca)]

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/6a9bb138631c
Implement mochitests for Message Security popup pane. r=mkmelin

Comment 9

[Alessandro Castellani (:aleca)]

Mh, we some failure on Windows and macOS that didn't come out during the try-run

TEST-UNEXPECTED-FAIL | comm/mail/test/browser/openpgp/browser_viewMessageSecurity.js | Timeout waiting for events: DeleteOrMoveMsgCompleted,DeleteOrMoveMsgFailed - JS frame :: resource://testing-common/mozmill/FolderDisplayHelpers.jsm :: FolderListener_waitForEvents :: line 2062
TEST-UNEXPECTED-FAIL | comm/mail/test/browser/openpgp/browser_viewMessageSecurity.js | Cleanup function threw an exception - at resource://testing-common/mozmill/utils.jsm:127

Comment 10

[Magnus Melin [:mkmelin]]

Backout: https://hg.mozilla.org/comm-central/rev/d77200118747f150a9c66cab777ef00c87c5b1c8

Comment 11

[Alessandro Castellani (:aleca)]

Attachment: 1670307-message-security-tests.diff

For whatever reason, it seems that mc.waitFor(() => gInbox.getTotalMessages(false) < count) in the cleanup method is causing intermittent problems on macos and windows, even tho we use the same method in other tests to delete generated messages.

I replaced that with a simple while loop and launched another try-run: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=2e7a4c83facb537cbab282f49c6295e7b6e13b52

Locally on macos the test runs without problems.

Comment 12

[Alessandro Castellani (:aleca)]

Windows 10 try run is all green

Comment 13

[Alessandro Castellani (:aleca)]

Attachment: 1673689-key-wizard-tests-v2.diff

A slight variation which deletes the messages at the end of every test, avoiding a loop during clean up.
Try run: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=c3f0f64be9059f6398db2dd1c62deb9bc20365f4

Comment 14

[Magnus Melin [:mkmelin]]

Comment on attachment 9186157 [details] [diff] [review]
1673689-key-wizard-tests-v2.diff

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

LGTM

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b8ac64b6ada8
Implement mochitests for Message Security popup pane. r=mkmelin DONTBUILD

Comment 16

[Magnus Melin [:mkmelin]]

We'd want these tests on 78 to ensure things work correctly. Test only so might as well go directly to 78.

Comment 17

[Alessandro Castellani (:aleca)]

Comment on attachment 9186157 [details] [diff] [review]
1673689-key-wizard-tests-v2.diff

[Approval Request Comment]
Regression caused by (bug #): -
User impact if declined: No impact as this patch only implements tests
Testing completed (on c-c, etc.): on c-c
Risk to taking this patch (and alternatives if risky): no risk

Comment 18

[Rob Lemley [:rjl]]

Comment on attachment 9186157 [details] [diff] [review]
1673689-key-wizard-tests-v2.diff

[Triage Comment]
Test-only code.

Comment 19

[Rob Lemley [:rjl]]

Thunderbird 78.5.0:
https://hg.mozilla.org/releases/comm-esr78/rev/048e4cfb5548