1651490 - add xpcshell tests for opengpg messages

Status: RESOLVED FIXED

(MailNews Core :: Security: OpenPGP, task)

Description

[Magnus Melin [:mkmelin]]

In bug 1011625 we added tests for S/MIME. We should add similar tests for OpenPGP.
See https://hg.mozilla.org/comm-central/rev/5d7419c95b2b

Comment 1

[Lasana Murray]

Attachment: bug1651490.patch

Attaching this for feedback.

I think I finally adapted the test enough to test with openpgp. There is some clean up and additional assertions to add but I wanted to get some feedback before I continue.

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 9185321 [details] [diff] [review]
bug1651490.patch

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

::: mailnews/mime/test/unit/head_mime.js
@@ +98,5 @@
>      }
>    }
>  
> +  msgWindow =
> +    msgWindow != null

only check falsy/truthy

if (!msgWindow) {
  msgWindow = .....
}

::: mailnews/mime/test/unit/test_openpgp_decrypt.js
@@ +49,5 @@
> +
> +let gInbox;
> +let openPGPEmailDir = "../../../data/openpgp/";
> +
> +let headerSink = {

So this is simulating Enigmail.hdrView.headerPane? Please add a code comment of what it is. 
Could be useful to put this functionality in  OpenPGPTestUtils?

@@ +165,5 @@
> +  },
> +];
> +
> +let gCopyWaiter = PromiseUtils.defer();
> +

I think you don't need this. 
You have two "tasks" now, but they are just async helper functions

@@ +299,5 @@
> +function run_test() {
> +  gInbox = configure_message_injection({
> +    mode: "local",
> +  });
> +  run_next_test();

I think it's easier to not use the run_test() version, but just add_task as you go, from top as needed

Comment 3

[Lasana Murray]

Attachment: bug1651490v2.patch

Updated the tests to use the status flags since I was more or less using them anyway and there are so many. Also included a test for the keyId though it's not always available. In future I'd like to also test the email but looks like that isn't passed to updateSecurityStatus(). I'll file a bug for that.

These need the patch from bug 1675325 to work.

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 9186318 [details] [diff] [review]
bug1651490v2.patch

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

::: mailnews/mime/test/unit/head_mime.js
@@ +97,5 @@
>        );
>      }
>    }
>  
> +  msgWindow = msgWindow

would just do 
if (!msgWindow)  {
  msgWindow = Cc["@mozilla.org/messenger/msgwindow;1"].createInstance(Ci.nsIMsgWindow);
}

or 

msgWindow = msgWIndow || Cc["@mozilla.org/messenger/msgwindow;1"].createInstance(Ci.nsIMsgWindow);

::: mailnews/mime/test/unit/test_openpgp_decrypt.js
@@ +34,5 @@
> +  "EnigmailCore"
> +);
> +
> +/* import-globals-from ../../../test/resources/asyncTestUtils.js */
> +load("../../../resources/asyncTestUtils.js");

This is an old framework you should not use... it predates async and promises which is the new way.
I don't see you using it directly... but maybe I didn't check closely enough.

@@ +132,5 @@
> + * @type Test[]
> + */
> +const tests = [
> +  {
> +    filename:

would be good to for each test add a description property that says what it's testing

@@ +155,5 @@
> +      "unsigned-encrypted-to-0xf231550c4f47e38e-from-0xfbfcc82a015e7330-with-key.eml",
> +    contents,
> +    from: "[e-mail]",
> +    // keyId: OpenPGPTestUtils.BOB_KEY_ID,
> +    // ^keyId is not passed when message is not signed, should it?

No, if it's not signed there is no key that signed.

@@ +306,5 @@
> + * status flags the test specifies.
> + */
> +add_task(async function testMimeDecryptOpenPGPMessages() {
> +  let hdrIndex = 0;
> +  for (let test of tests) {

... and here

info(``Running test: ${test.description}`)

@@ +307,5 @@
> + */
> +add_task(async function testMimeDecryptOpenPGPMessages() {
> +  let hdrIndex = 0;
> +  for (let test of tests) {
> +    if (test.skip) {

and here also output an info that the test is being skipped

Comment 5

[Lasana Murray]

This is an old framework you should not use...

It's needed by the messageInjection.js script.

https://searchfox.org/comm-central/rev/7c87b9f5a18f5b6df13e6d06cd7d7ce8bb173c5e/mailnews/test/resources/messageInjection.js#205

Comment 6

[Magnus Melin [:mkmelin]]

I see. Let's go with this then. Filed bug 1676114.

Comment 7

[Lasana Murray]

Attachment: bug1651490v3.patch

Added description properties to tests.

Comment 8

[Lasana Murray]

Try run on the way, I cleaned up the linting issues in the patch attached:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&selectedTaskRun=FlQ8v1lHR8SgkXdBeBYM7A.0

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 9186608 [details] [diff] [review]
bug1651490v3.patch

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

LGTM, r=mkmelin

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/8fd1b44855a3
Add mime xpcshell tests for openpgp. r=mkmelin

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9e145e871c4c
followup to make the new OpenPGP xpcshell tests work after bug 1664701. rs=bustage-fix

Comment 12

[Lasana Murray]

^ Did some thing break? Can I put that change you made in an ensureOpenPGP() function instead of executing when the module is imported?

xpcshell tests would then do:

add_task(async function(){ await OpenPGPTestUtils.ensureOpenPGP() })

Comment 13

[Magnus Melin [:mkmelin]]

Yes the push before yours changed module initialization for openpgp... which broke the test. https://treeherder.mozilla.org/jobs?repo=comm-central&revision=8fd1b44855a35346818ca6878759dfb3f1cccd05

I'm not sure any changes are needed. This would only ever be a problem in xpcshell tests, and there I think it will be fairly clear what needs doing. Maybe.

Comment 14

[Lasana Murray]

There are other things that need to be initialized before OpenPGPTestUtils can be used in an xpcshell test, probably related to the bug for that push. It wasn't really clear at first touch.

I think they can all go in a method on OpenPGPTestUtils. My opinion is that code should not be executed when a library style module is imported, takes control away from the importing module and leaves room for bugs.

Comment 15

[Magnus Melin [:mkmelin]]

Agreed.

Comment 16

[Magnus Melin [:mkmelin]]

Please request uplifts for this and related tests so 78 test coverage is improved.

Comment 17

[Lasana Murray]

Comment on attachment 9186608 [details] [diff] [review]
bug1651490v3.patch

[Approval Request Comment]
Regression caused by (bug #): None. Adds more tests for OpenPGP.
User impact if declined: None really.
Testing completed (on c-c, etc.): c-c
Risk to taking this patch (and alternatives if risky): None that I can think of. This makes testing OpenPGP support less manual.

Comment 18

[Wayne Mery (:wsmwk)]

Comment on attachment 9186608 [details] [diff] [review]
bug1651490v3.patch

[Triage Comment]
Approved for esr78

Comment 19

[Kai Engert (:KaiE:)]

78.5.1
https://hg.mozilla.org/releases/comm-esr78/rev/0a445863f424b6cba1165db059fb3b1e9c75ef0f