1673652 - implement mochitests for openpgp message compose

Status: RESOLVED FIXED

(MailNews Core :: Security: OpenPGP, enhancement)

Description

[Magnus Melin [:mkmelin]]

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

Comment 1

[Lasana Murray]

Attachment: bug1673652.patch

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.

Comment 2

[Lasana Murray]

Attachment: bug1673652.patch

Fixed some typos.

Comment 3

[Magnus Melin [:mkmelin]]

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.

Comment 4

[Lasana Murray]

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.

Comment 5

[Magnus Melin [:mkmelin]]

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.

Comment 6

[Lasana Murray]

Attachment: bug1673652v2.patch

Changes made, cleaned up the tests a bit.

Comment 7

[Lasana Murray]

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 8

[Magnus Melin [:mkmelin]]

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

Comment 9

[Lasana Murray]

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.

Comment 10

[Lasana Murray]

Attachment: bug1673652v3.patch

Updated some of the Assert messages.

Comment 11

[Lasana Murray]

Try run in progress:

https://treeherder.mozilla.org/jobs?repo=try-comm-central&selectedTaskRun=eHVVmgMbTc-xS9LptnL-Cw.0

Comment 12

[Magnus Melin [:mkmelin]]

Comment on attachment 9192239 [details] [diff] [review]
bug1673652v3.patch

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

Looks good bug osx still fails

Comment 13

[Lasana Murray]

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?

Comment 14

[Lasana Murray]

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.

Comment 15

[Lasana Murray]

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

Comment 16

[Magnus Melin [:mkmelin]]

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.

Comment 17

[Lasana Murray]

Attachment: bug1673652v4.patch

I set the test to skip on osx.

Comment 18

[Lasana Murray]

Latest try run:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=75a6b31ef18157b32168807ab6ebad8841b3cd99

Comment 19

[Lasana Murray]

Attachment: bug1673652v5.patch

Follow up fix for typo causing bct5

Comment 20

[Magnus Melin [:mkmelin]]

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.

Comment 21

[Lasana Murray]

Attachment: bug1673652v6.patch

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

Comment 22

[Lasana Murray]

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

Comment 23

[Lasana Murray]

Try run looks healthy (lint error already fixed).
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=d0858d16280980864268b08cc6e1b835be2baabf

Comment 24

[Magnus Melin [:mkmelin]]

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.

Comment 25

[Lasana Murray]

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

Comment 26

[Magnus Melin [:mkmelin]]

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.

Comment 27

[Lasana Murray]

Attachment: bug1673652v8.patch

Change made.

Comment 28

[Magnus Melin [:mkmelin]]

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.

Comment 29

[Lasana Murray]

Attachment: bug1673652v9.patch

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

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/6e53cb981c40
Add tests for OpenPGP message composition. r=mkmelin