Closed Bug 1670307 Opened 3 months ago Closed 3 months ago

Implement mochitests for the Message Security popup pane

Categories

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

enhancement

Tracking

(thunderbird_esr78+ fixed, thunderbird83 affected)

RESOLVED FIXED
84 Branch
Tracking Status
thunderbird_esr78 + fixed
thunderbird83 --- affected

People

(Reporter: aleca, Assigned: aleca)

References

Details

(Keywords: testcase)

Attachments

(1 file, 4 obsolete files)

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.

Depends on: 1669014
Priority: -- → P1
Status: NEW → ASSIGNED
Depends on: 1672851

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.

Attachment #9185271 - Flags: review?(mkmelin+mozilla)
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
Attachment #9185271 - Flags: review?(mkmelin+mozilla)

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.

Attachment #9185271 - Attachment is obsolete: true
Attachment #9185570 - Flags: review?(mkmelin+mozilla)
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
Attachment #9185570 - Flags: review?(mkmelin+mozilla) → feedback+

Nice, using the PromiseCopyListener makes it easier to read.

Attachment #9185570 - Attachment is obsolete: true
Attachment #9185622 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9185622 [details] [diff] [review]
1670307-message-security-tests.diff

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

LGTM, thx! r=mkmelin
Attachment #9185622 - Flags: review?(mkmelin+mozilla) → review+

Pushed by alessandro@thunderbird.net:
https://hg.mozilla.org/comm-central/rev/6a9bb138631c
Implement mochitests for Message Security popup pane. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → 84 Branch

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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Blocks: 1673689

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.

Attachment #9185622 - Attachment is obsolete: true
Attachment #9186083 - Flags: review?(mkmelin+mozilla)

Windows 10 try run is all green

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

Attachment #9186157 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9186157 [details] [diff] [review]
1673689-key-wizard-tests-v2.diff

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

LGTM
Attachment #9186157 - Flags: review?(mkmelin+mozilla) → review+
Attachment #9186083 - Attachment is obsolete: true
Attachment #9186083 - Flags: review?(mkmelin+mozilla)

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/b8ac64b6ada8
Implement mochitests for Message Security popup pane. r=mkmelin DONTBUILD

Status: REOPENED → RESOLVED
Closed: 3 months ago3 months ago
Resolution: --- → FIXED

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

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

Attachment #9186157 - Flags: approval-comm-esr78?

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

[Triage Comment]
Test-only code.

Attachment #9186157 - Flags: approval-comm-esr78? → approval-comm-esr78+
You need to log in before you can comment on or make changes to this bug.