Closed Bug 1673652 Opened 4 years ago Closed 4 years ago

implement mochitests for openpgp message compose

Categories

(MailNews Core :: Security: OpenPGP, enhancement)

Product:
MailNews Core
Component:
Security: OpenPGP
Type:
enhancement

Tracking

(thunderbird_esr78 wontfix)

Status:
RESOLVED FIXED
Milestone:
86 Branch
Tracking Flags:
Tracking Status
thunderbird_esr78 --- wontfix

People

(Reporter: mkmelin, Assigned: lasana)

References

Details

Attachments

(1 file, 8 obsolete files)

bug1673652v9.patch
(deleted), patch
mkmelin
: review+
Details | Diff | Splinter Review

We should add mochitests to test various scenarios for message composition where OpenPGP is involved:

  • Create + send (use Send Later) a signed message + open in Outbox, verify it's signed

  • Create + send an encrypted+signed message + open in Outbox, verify it's encrypted+signed

  • Create + send an encrypted (only) + open in Outbox, verify it's encrypted+signed

  • Try to send to encrypted when
    ** recipient key is not available
    ** recipient key is available but not (yet) trusted
    ** more than one recipient, one recipient key available but other not
    ** more than one recipient, one recipient key available, some not yet trusted
    (Note that this flow is clunky and should be improved, so too much details may not be worth testing, the basics still would be the same.)

  • Reply to an encrypted message should use encryption by default

Attached patch bug1673652.patch (obsolete) (deleted) — — Splinter Review

Here are the tests I wrote for this. They work locally but there are failures on the try server I'm trying to figure out.

Attachment #9190386 - Flags: feedback?(mkmelin+mozilla)
Status: NEW → ASSIGNED
Attached patch bug1673652.patch (obsolete) (deleted) — — Splinter Review

Fixed some typos.

Attachment #9190386 - Attachment is obsolete: true
Attachment #9190386 - Flags: feedback?(mkmelin+mozilla)
Attachment #9190513 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9190513 [details] [diff] [review] bug1673652.patch Review of attachment 9190513 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/test/browser/openpgp/composition/head.js @@ +6,5 @@ > + > +/** > + * Used to intercept the alert prompt that comes before the key status dialog. > + */ > +async function getAlertPromise() { Please rename this to something like unableToSendEncryptedDialog @@ +19,5 @@ > + ), > + "send message with encryption error prompt found" > + ); > + > + let closePromise = BrowserTestUtils.domWindowClosed(win); I find these easier to read if I name it e.g. let closed = ... await closed; Having the type (Promise) in the variable name is not super good practice. @@ +30,5 @@ > +/** > + * Used to intercept the dialog that displays the key statuses when an error > + * is encountered. > + */ > +async function getKeyStatusDialogPromise(callback) { It's a bit odd to mix callbacks and async Can't you just let this function resolve(win); Then let win = await getKeyStatusDialog; win.dowhatever() @@ +42,5 @@ > + > + // Used in some of the tests to verify key status display. > + win.MozXULElement.insertFTLIfNeeded( > + "messenger/openpgp/composeKeyStatus.ftl" > + ); I think you shouldn't add translations to a document. Who knows if it would cause side effects. Better to just create new Localization() and use that for accessing needed Fluent strings.
Attachment #9190513 - Flags: review?(mkmelin+mozilla) → feedback+

I find these easier to read if I name it e.g.
let closed = ...
await closed;
Having the type (Promise) in the variable name is not super good practice.

closed seems kind of vague to me, anyone unfamiliar with BrowserTestUtils might think it's a boolean at first glance. In fact await <boolean> is valid syntax (probably due to the automatic wrapping) so they won't be wrong in thinking that. The reason I put "Promise" in the names is to be descriptive as possible. It isn't always clear that something is a Promise without reading the source, especially when we assign them to variables before awaiting.

It's a bit odd to mix callbacks and async
Can't you just let this function resolve(win);
Then let win = await getKeyStatusDialog; win.dowhatever()

The goDoCommand call blocks until the dialog is closed so we need to close it in the handler of promiseAlertDialogOpen or the test will timeout. I'll add a comment about that.

Well, having type in the variable name is generally frowned upon. We don't write things like enabledBoolean either. Your right that sometimes keeping in mind what type it is is important, but here it's usually not many lines between.

Attached patch bug1673652v2.patch (obsolete) (deleted) — — Splinter Review

Changes made, cleaned up the tests a bit.

Attachment #9190513 - Attachment is obsolete: true
Attachment #9191677 - Flags: review?(mkmelin+mozilla)

Finally got the tests to pass on try server (debug builds most likely fail due to being slower) except for OSX. The screenshot made also shows blank windows so I'm not sure what its problem is.

https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=6dfd327830a2dbada568288d928a9ff08ba3cea5

Comment on attachment 9191677 [details] [diff] [review] bug1673652v2.patch Review of attachment 9191677 [details] [diff] [review]: ----------------------------------------------------------------- Looks pretty good. If you can't get the debug builds to pass, we could consider disabling this test for debug. ::: mail/test/browser/openpgp/composition/browser_composeEncrypted.js @@ +47,5 @@ > + * @param {Function} callback - A function that is called with the dialog window > + * when it's intercepted. Opening the dialog will block until it's closed so > + * this function must close the dialog or tests will timeout. > + */ > +async function handleKeyStatusDialog(callback) { instead of passing in a callback, maybe it would be better to have this return the window? So return new Promise(resolve => { ..... resolve(win); }); @@ +324,5 @@ > + ); > + > + await OpenPGPTestUtils.toggleMessageEncryption(composeWin); > + > + let alert = handleUnableToSendEncryptedDialog(); risky using a built in function name as variable name... @@ +347,5 @@ > + > + Assert.equal( > + richItem.textContent, > + txt, > + "key acceptance status is correct" Confusing message once it fails. Please change to "... should be correct" @@ +518,5 @@ > + > + Assert.equal( > + infoList.itemChildren.length, > + 2, > + "key statuses are displayed" should be. There are similar messages elsewhere too that would be confusing. ::: mail/test/browser/openpgp/composition/head.js @@ +13,5 @@ > + win.goDoCommand("cmd_sendLater"); > + await closePromise; > + > + // Give encryption/signing time to finish. > + return new Promise(resolve => setTimeout(resolve, 0)); 0 is default, so not needed
Attachment #9191677 - Flags: review?(mkmelin+mozilla) → review+

instead of passing in a callback, maybe it would be better to have this return the window?

It does actually return the window via promiseAlertDialogOpen, the callback is used because the dialog is opened with the "modal" property:
https://searchfox.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#1842

The moment it opens, the current code path will block until it is closed. We can't get a reference to the window without using await on handleKeyStatusDialog which will also block until we open the window.

Attached patch bug1673652v3.patch (obsolete) (deleted) — — Splinter Review

Updated some of the Assert messages.

Attachment #9191677 - Attachment is obsolete: true
Attachment #9192239 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9192239 [details] [diff] [review] bug1673652v3.patch Review of attachment 9192239 [details] [diff] [review]: ----------------------------------------------------------------- Looks good bug osx still fails
Attachment #9192239 - Flags: review?(mkmelin+mozilla) → review+

I suspect the failure here is related to the L10Registry error message. If you look at the stack trace being logged it looks like the same kind of issue I brought up in bug 1679213:

2020-12-14T18:17:01.408Z] 18:17:01     INFO - L10nRegistry.loadSync@resource://gre/modules/L10nRegistry.jsm:692:19
[task 2020-12-14T18:17:01.408Z] 18:17:01     INFO - fetchFile@resource://gre/modules/L10nRegistry.jsm:607:31
[task 2020-12-14T18:17:01.408Z] 18:17:01     INFO - generateResourceSetSync/<@resource://gre/modules/L10nRegistry.jsm:512:19
[task 2020-12-14T18:17:01.408Z] 18:17:01     INFO - generateResourceSetSync@resource://gre/modules/L10nRegistry.jsm:507:22
[task 2020-12-14T18:17:01.408Z] 18:17:01     INFO - generateResourceSetsForLocaleSync@resource://gre/modules/L10nRegistry.jsm:449:44
[task 2020-12-14T18:17:01.408Z] 18:17:01     INFO - generateBundlesSync@resource://gre/modules/L10nRegistry.jsm:186:7
[task 2020-12-14T18:17:01.409Z] 18:17:01     INFO - touchNext@resource://gre/modules/Localization.jsm:167:37
[task 2020-12-14T18:17:01.409Z] 18:17:01     INFO - generateBundles@resource://gre/modules/Localization.jsm:473:15
[task 2020-12-14T18:17:01.409Z] 18:17:01     INFO - @chrome://messenger/content/messengercompose/MsgComposeCommands.js:61:19
[task 2020-12-14T18:17:01.409Z] 18:17:01     INFO - waitFor@resource://testing-common/mozmill/utils.jsm:166:12
[task 2020-12-14T18:17:01.409Z] 18:17:01     INFO - WindowWatcher_waitForWindowOpen@resource://testing-common/mozmill/WindowHelpers.jsm:266:11
[task 2020-12-14T18:17:01.409Z] 18:17:01     INFO - wait_for_new_window@resource://testing-common/mozmill/WindowHelpers.jsm:637:19
[task 2020-12-14T18:17:01.409Z] 18:17:01     INFO - wait_for_compose_window@resource://testing-common/mozmill/ComposeHelpers.jsm:277:34
[task 2020-12-14T18:17:01.409Z] 18:17:01     INFO - open_compose_new_mail@resource://testing-common/mozmill/ComposeHelpers.jsm:80:10
[task 2020-12-14T18:17:01.409Z] 18:17:01     INFO - testEncryptedMessageComposition@chrome://mochitests/content/browser/comm/mail/test/browser/openpgp/composition/browser_composeEncrypted.js:153:13
[task 2020-12-14T18:17:01.409Z] 18:17:01     INFO - Tester_execTest/<@chrome://mochikit/content/browser-test.js:1069:34

utils.waitFor is invoked and it calls thread.processNextEvent(true) which throws things off. Maybe it's causing things to be executed out of order? Initially I started converting waitFor to use the BrowserTestUtils functions but those changes affect other places and did break at least one test I wrote.

Should I try again? Should we land this for now?

Flags: needinfo?(mkmelin+mozilla)

Bug 422451 comment 0 provides some insight into the problem with nextThread. Bug 1359923 says it should not be called from JS. Looking more and more like it needs to be replaced.

Looks like we don't use it much in core but in quite a few tests:
https://searchfox.org/comm-central/search?q=processNextEvent&path=&case=true&regexp=false

Maybe something like await promiseAnimationFrame needs to be added`? IIRC I read localization is guaranteed to be ready after that (unless there are bugs).
But yes, let's try to get it landed, with potentially debug+osx disabled if they are refusing to work.

Flags: needinfo?(mkmelin+mozilla)
Attached patch bug1673652v4.patch (obsolete) (deleted) — — Splinter Review

I set the test to skip on osx.

Attachment #9192239 - Attachment is obsolete: true
Attachment #9194601 - Flags: review?(mkmelin+mozilla)
Attached patch bug1673652v5.patch (obsolete) (deleted) — — Splinter Review

Follow up fix for typo causing bct5

Attachment #9194601 - Attachment is obsolete: true
Attachment #9194601 - Flags: review?(mkmelin+mozilla)
Attachment #9194676 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9194676 [details] [diff] [review] bug1673652v5.patch Review of attachment 9194676 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/test/browser/openpgp/composition/browser.ini @@ +11,5 @@ > +support-files = ../data/** > + > +[browser_composeSigned.js] > +[browser_composeEncrypted.js] > +skip-if = os == "mac" # L10NRegistry throws NS_ERROR_FILE_UNRECOGNIZED_PATH Apparently browser_composeSigned.js also fails on osx, based on the try run I did. When we disable, also add the bug number for reference., like `skip-if = os == "mac" # Bug 1673652 - L10NRegistry throws NS_ERROR_FILE_UNRECOGNIZED_PATH` And you also needed to disable for debug, no? And, there's an ES failure ::: mail/test/browser/openpgp/composition/browser_composeEncrypted.js @@ +651,5 @@ > + ); > +}); > + > +registerCleanupFunction(function tearDown() { > + bobIdentity.setUnicharAttribute("openpgp_key_id", initialKeyIdPref); initialKeyIdPref is pretty pointless to keep around since you'll setup and remove the account anyway ::: mail/test/browser/shared-modules/OpenPGPTestUtils.jsm @@ +234,5 @@ > + > +async function clickMenuOption(win, selector) { > + let menu = win.document.querySelector("#optionsMenu"); > + EventUtils.synthesizeMouseAtCenter(menu, {}, win); > + await BrowserTestUtils.waitForEvent(menu, "popupshown"); This is wrong, maybe the thing that is causing the problems? You need to set up the waiting before clicking, otherwise you can easily wait for something that already happened, which won't work. So ` let popupshown = BrowserTestUtils.waitForEvent(menu, "popupshown"); EventUtils.synthesizeMouseAtCenter(menu, {}, win); await popupshown; ` @@ +242,5 @@ > + {}, > + win > + ); > + > + await BrowserTestUtils.waitForEvent(menu, "popuphidden"); same thing here, you need to set up the waiting before clicking.
Attachment #9194676 - Flags: review?(mkmelin+mozilla) → feedback+
Attached patch bug1673652v6.patch (obsolete) (deleted) — — Splinter Review

Changes made, disabled all the tests on debug and mac.

Attachment #9194676 - Attachment is obsolete: true
Attachment #9194942 - Flags: review?(mkmelin+mozilla)

Please note; this now needs the patch from bug 1633726 to land before applying.

Comment on attachment 9194942 [details] [diff] [review] bug1673652v6.patch Review of attachment 9194942 [details] [diff] [review]: ----------------------------------------------------------------- r=mkmelin with the below fixed ::: mail/test/browser/openpgp/composition/browser.ini @@ +6,5 @@ > + mail.spotlight.firstRunDone=true > + mail.winsearch.firstRunDone=true > + mailnews.start_page.override_url=about:blank > + mailnews.start_page.url=about:blank > +skip-if = os == "mac" || debug # Bug 1673652 - L10NRegistry throws NS_ERROR_FILE_UNRECOGNIZED_PATH I'm surprised if this works, at least it's unusual to place it here. You should put this line *below* each test you want to disable.
Attachment #9194942 - Flags: review?(mkmelin+mozilla) → review+
status-thunderbird_esr78: --- → wontfix
Target Milestone: --- → 86 Branch

I'm surprised if this works, at least it's unusual to place it here. You should put this line below each test you want to disable.

I saw it done like that elsewhere, example:
https://searchfox.org/comm-central/source/mail/test/browser/account/browser.ini#45
https://searchfox.org/comm-central/source/mozilla/accessible/tests/browser/browser.ini#2

Ah, ok. I'd still prefer these to be disabled on a per-test basis.
The tests of https://searchfox.org/comm-central/rev/e385c646e9890c5f9722489a5305fda7e9bf0ebb/mail/test/browser/account/browser.ini#45 look like they could be re-enabled since bug 1265637 is now fixed.

Attached patch bug1673652v8.patch (obsolete) (deleted) — — Splinter Review

Change made.

Attachment #9194942 - Attachment is obsolete: true
Attachment #9194946 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9194946 [details] [diff] [review] bug1673652v8.patch Review of attachment 9194946 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/test/browser/openpgp/composition/browser.ini @@ +6,5 @@ > + mail.spotlight.firstRunDone=true > + mail.winsearch.firstRunDone=true > + mailnews.start_page.override_url=about:blank > + mailnews.start_page.url=about:blank > +skip-if = os == "mac" || debug # Bug 1673652 - L10NRegistry throws NS_ERROR_FILE_UNRECOGNIZED_PATH This is still the in the wrong place.
Attachment #9194946 - Flags: review?(mkmelin+mozilla)
Attached patch bug1673652v9.patch (deleted) — — Splinter Review

Sorry uploaded the wrong file, I have way too many bookmarks and patches lying around.

Attachment #9194946 - Attachment is obsolete: true
Attachment #9194951 - Flags: review?(mkmelin+mozilla)
Attachment #9194951 - Flags: review?(mkmelin+mozilla) → review+

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/6e53cb981c40
Add tests for OpenPGP message composition. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: