1588831 - Mailing list functionality is not tested

Status: RESOLVED FIXED

(Thunderbird :: Address Book, task, P3)

Description

[Paul Morris [:pmorris]]

We could use some test coverage for mailing list functionality. Recently there were a number of regressions in this area that were fixed. Tests will help detect and prevent such regressions.

Comment 1

[Paul Morris [:pmorris]]

Attachment: wip-mailing-list-tests-0.patch

A WIP patch on adding some basic mochitests. Working with secondary windows, dialogs, and trees is a little tricky. Currently the test fails due to bug 1588795.

Geoff, feedback is welcome if you have a moment, not urgent.

Comment 2

[Paul Morris [:pmorris]]

Attachment: mailing-list-tests-1.patch

Tests creating a new mailing list and then editing it. Passes with the fix for bug 1588795 applied. Still need to figure out where to put mochitest utility functions where they'll be generally accessible.

Comment 3

[Geoff Lankow (:darktrojan)]

Comment on attachment 9101936 [details] [diff] [review]
mailing-list-tests-1.patch

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

Good, but with a lot of room for improvement.

::: mailnews/addrbook/test/browser/browser_mailing_lists.js
@@ +14,5 @@
> +
> +  // Wait for document to load.
> +  let windowLoaded = false;
> +  abWindow.addEventListener("load", () => (windowLoaded = true));
> +  await waitUntil(() => windowLoaded);

BrowserTestUtils.waitForEvent(abWindow, "load")

In fact, you could use domWindowOpened for this: see https://searchfox.org/mozilla-central/rev/d7537a9cd2974efa45141d974e5fc7c727f1a78f/testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm#2038

@@ +22,5 @@
> +  // Create a new mailing list with some addresses in it.
> +
> +  let newAddressListButton = abDoc.getElementById("button-newlist");
> +
> +  let mailingListWindowPromise = BrowserTestUtils.promiseAlertDialogOpen(

I think this should use promiseAlertDialog instead of promiseAlertDialogOpen, so that we can be sure the dialog has closed again.

@@ +26,5 @@
> +  let mailingListWindowPromise = BrowserTestUtils.promiseAlertDialogOpen(
> +    null,
> +    "chrome://messenger/content/addressbook/abMailListDialog.xul",
> +    // A callback that can interact with the mailing list dialog.
> +    async mailingListWindow => {

Here you should test that the dialog is set up correctly. Right address book selected, no values in the text boxes or contacts in the list.

@@ +33,5 @@
> +
> +      let titleInput = mlDocument.getElementById("ListName");
> +      let addressInput = mlDocument.getElementById("addressCol1#1");
> +      titleInput.value = "Mochitest mailing list 1";
> +      addressInput.value = addresses.join(", ");

Perhaps use EventUtils.sendString, since it would test that the keyboard behaviour does what we expect.

@@ +36,5 @@
> +      titleInput.value = "Mochitest mailing list 1";
> +      addressInput.value = addresses.join(", ");
> +
> +      mlDocElement.getButton("accept").click();
> +      return Promise.resolve();

This line is unnecessary in an async function.

@@ +47,5 @@
> +    { clickCount: 1 },
> +    abWindow
> +  );
> +
> +  await mailingListWindowPromise;

Here, check that the list and contacts were added to the nsIAbDirectory and that the contacts were added to the list. Mochitests should check the back end as well as the front end where feasible.

@@ +56,5 @@
> +
> +  // Double-click on "Personal Address Book" to reveal the mailing list.
> +  treeClick(dirTree, 1, 0, { clickCount: 2 }, abWindow);
> +
> +  let mailingListWindowPromise2 = BrowserTestUtils.promiseAlertDialogOpen(

If you're going to name this mailingListWindowPromise2, name the other one mailingListWindowPromise1, or just reuse the variable.

@@ +60,5 @@
> +  let mailingListWindowPromise2 = BrowserTestUtils.promiseAlertDialogOpen(
> +    null,
> +    "chrome://messenger/content/addressbook/abEditListDialog.xul",
> +    // A callback that can interact with the mailing list dialog.
> +    async mailingListWindow => {

Again, check the name, nickname, description values.

Etc., etc.. I'm sure you will apply the comments I've made so far to the rest of the test, so I won't repeat them.

::: mailnews/addrbook/test/browser/head.js
@@ +27,5 @@
> + * @param {Function} test - Test function that returns true or false.
> + * @param {number} timeLimit - How long to retry the test until giving up.
> + * @param {Promise} - A promise that is resolved when we are done.
> + */
> +function waitUntil(test, timeLimit = 8192) {

This is a bad practice. There's almost always some event you can listen for and if there isn't there probably should be. Some exceptions apply, and yeah I know mozmill does it everywhere, this isn't mozmill.

You don't need it in this test as I pointed out elsewhere, but I thought I'd mention it anyway.

@@ +53,5 @@
> +async function openAddressBook() {
> +  const addressBookButton = document.getElementById("button-address");
> +  EventUtils.synthesizeMouseAtCenter(addressBookButton, { clickCount: 1 });
> +
> +  await new Promise(resolve => setTimeout(resolve));

Probably better if this function did the waiting for the new window to load before resolving. Also, after it does load, it would be better to do abWindow.setTimeout here, since it puts `resolve` on the end of the event queue of that window.

Comment 4

[Paul Morris [:pmorris]]

Attachment: mailing-list-tests-2.patch

Thanks for the review and the help navigating this mochitest world.

Before the previous patch I had tried using EventUtils.sendString but I wasn't waiting for the dialog to be ready to handle the input so it wasn't working. Now I'm starting to get the swing of it.

So this addresses all of your comments and adds a few other things. I've made a couple of functions more generally accessible as utility functions. There might be ways to improve the implementation of making that work.

Try run (using an artifact build): https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=28d265c58b51bb30b492eebfef8d801a55d8f726

Comment 5

[Magnus Melin [:mkmelin]]

Comment on attachment 9103748 [details] [diff] [review]
mailing-list-tests-2.patch

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

Some drive-by comments

::: mail/base/modules/MailUtils.jsm
@@ +536,5 @@
> +
> +  /**
> +   * Finds a mailing list anywhere in the address books.
> +   *
> +   * @param {string} entryName  Value against which dirName is checked.

for things like these, please add a " - " as recommended by JSDoc
   * @param {string} entryName - Value against which dirName is checked.

@@ +539,5 @@
> +   *
> +   * @param {string} entryName  Value against which dirName is checked.
> +   * @returns {nsIAbDirectory|null}  Found list or null.
> +   */
> +  findListInAddressBooks(entryName, catchFn = ex => console.warn(ex)) {

I think you shouldn't add en error callback. If you actually need something like that, just return a promise.

But then again, these try/catches look like they are not at all needed, and should be removed.

@@ +547,5 @@
> +      let abDir = null;
> +      try {
> +        abDir = allAddressBooks.getNext().QueryInterface(Ci.nsIAbDirectory);
> +      } catch (ex) {
> +        catchFn(ex);

you're not returning after the failure

@@ +550,5 @@
> +      } catch (ex) {
> +        catchFn(ex);
> +      }
> +
> +      if (abDir != null && abDir.supportsMailingLists) {

just do a truthy compare of abDir. But I guess you shouldn''t need that due to the above

::: mailnews/addrbook/test/browser/browser_mailing_lists.js
@@ +7,5 @@
> +add_task(async () => {
> +  const { MailUtils } = ChromeUtils.import("resource:///modules/MailUtils.jsm");
> +  const { fixIterator } = ChromeUtils.import(
> +    "resource:///modules/iteratorUtils.jsm"
> +  );

For imports, just add them top level

Comment 6

[Paul Morris [:pmorris]]

Attachment: mailing-list-tests-3.patch

OK, thanks Magnus. I've made those changes.

Comment 7

[Paul Morris [:pmorris]]

Attachment: mailing-list-tests-4.patch

Geoff asked me to "create a test address book to put your contacts in, and make sure it gets removed at the end. See if you can get rid of the setTimeout(resolve, 500) too".

This patch does that, but now there are 4 failing subtests. For some reason deleting addresses from the mailing list doesn't work. Everything works fine when doing the steps manually. (sigh) Here's the error:

0:05.38 GECKO(13224) JavaScript error: chrome://messenger/content/addressbook/abMailListDialog.js, line 144: NS_ERROR_NOT_AVAILABLE: Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIAbDirectory.deleteCards]
 0:05.38 INFO Console message: [JavaScript Error: "NS_ERROR_NOT_AVAILABLE: Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIAbDirectory.deleteCards]" {file: "chrome://messenger/content/addressbook/abMailListDialog.js" line: 144}]
updateMailListMembers@chrome://messenger/content/addressbook/abMailListDialog.js:144:14
EditListOKButton@chrome://messenger/content/addressbook/abMailListDialog.js:241:26
_fireButtonEvent@chrome://global/content/elements/dialog.js:533:19
_doButtonCommand@chrome://global/content/elements/dialog.js:512:29
_handleButtonCommand@chrome://global/content/elements/dialog.js:504:39
mailingListWindowPromise2<@chrome://mochitests/content/browser/comm/mailnews/addrbook/test/browser/browser_mailing_lists.js:289:40
async*promiseAlertDialogOpen@resource://testing-common/BrowserTestUtils.jsm:2150:13
async*promiseAlertDialog@resource://testing-common/BrowserTestUtils.jsm:2178:26
@chrome://mochitests/content/browser/comm/mailnews/addrbook/test/browser/browser_mailing_lists.js:195:52
Async*Tester_execTest/<@chrome://mochikit/content/browser-test.js:1069:34
Tester_execTest@chrome://mochikit/content/browser-test.js:1104:11
nextTest/<@chrome://mochikit/content/browser-test.js:932:14
SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:805:67

There's also a number of these errors:

 0:05.14 INFO Console message: [JavaScript Error: "ERROR: failed to set status text: TypeError: getSelectedDirectory(...) is null" {file: "chrome://messenger/content/addressbook/addressbook.js" line: 541}]
SetStatusText@chrome://messenger/content/addressbook/addressbook.js:541:8
onCountChanged@chrome://messenger/content/addressbook/addressbook.js:144:18
updateMailListMembers@chrome://messenger/content/addressbook/abMailListDialog.js:140:14
MailListOKButton@chrome://messenger/content/addressbook/abMailListDialog.js:170:28
_fireButtonEvent@chrome://global/content/elements/dialog.js:533:19
_doButtonCommand@chrome://global/content/elements/dialog.js:512:29
_handleButtonCommand@chrome://global/content/elements/dialog.js:504:39
mailingListWindowPromise1<@chrome://mochitests/content/browser/comm/mailnews/addrbook/test/browser/browser_mailing_lists.js:140:40
async*promiseAlertDialogOpen@resource://testing-common/BrowserTestUtils.jsm:2150:13
async*promiseAlertDialog@resource://testing-common/BrowserTestUtils.jsm:2178:26
@chrome://mochitests/content/browser/comm/mailnews/addrbook/test/browser/browser_mailing_lists.js:91:52
Async*Tester_execTest/<@chrome://mochikit/content/browser-test.js:1069:34
Tester_execTest@chrome://mochikit/content/browser-test.js:1104:11
nextTest/<@chrome://mochikit/content/browser-test.js:932:14
SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:805:67

And as described in the patch, creating the address book by going through the UI was not reliable. Sometimes it worked and sometimes it didn't, so I'm currently calling the function directly to avoid making a flaky test. Doing a short timeout seemed to fix it, but that's no longer allowed by eslint, and I don't know of any other events to listen for since we're already waiting for "load" on the address book window.

Any insights on any of this are welcome, of course. (I'm starting to see why these tests weren't written before...)

Comment 8

[Geoff Lankow (:darktrojan)]

I don't yet know why, but the error is coming from somewhere in the Mork address book code. This shouldn't be happening, we should only be using JS address books now, but I forgot to switch some code over. I'm now fixing that in bug 1594246.

Comment 9

[Paul Morris [:pmorris]]

Attachment: mailing-list-tests-5.patch

Rebased and bug 1594246 fixes the first error. This patch now fixes the other error in addressbook.js -- a problem with trying to access a directory name before the directory is there. I'd prefer creating the address book by using the UI, but I'm not sure how to do that in a reliable way. (See comments in the patch.)

Comment 10

[Paul Morris [:pmorris]]

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

Comment 11

[Geoff Lankow (:darktrojan)]

Comment on attachment 9107048 [details] [diff] [review]
mailing-list-tests-5.patch

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

Getting closer. Sorry this is taking so long for feedback.

More general comments about testing:
It's harder to read a test if you have to keep in mind the current state of the UI (ie. knowing what each key press does). If you want to change a textbox or click on a button, go directly to it and do what you want. Sure, test that pressing keys does the right thing (especially in weird UI like this) but actually test it if you do (ie. input 2 has focus, press tab, test that input 3 has focus).

Also, don't be afraid to write a completely separate test if there's something else that needs testing. Tests are cheap. You don't need to do everything in one go.

::: mailnews/addrbook/test/browser/browser_mailing_lists.js
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, you can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/* globals DisplayNameUtils openAddressBook promiseDialogOpen */

openAddressBook is implicitly included because it's in the head file.
promiseDialogOpen isn't a thing any more.

You could explicitly import DisplayNameUtils, like you have with MailUtils and fixIterator which are already present. Or just list those as globals.

@@ +44,5 @@
> +    }
> +  );
> +
> +  // Using the UI was unreliable (see below) so just call the function.
> +  abWindow.AbNewAddressBook();

I would just skip this whole section and create your new book with MailServices.ab.newAddressBook. That would give you a reference to the book itself for testing things on it.

But you've done this now so leave it, although the commented bit that doesn't work can go.

@@ +117,5 @@
> +      is(
> +        abPopup.label,
> +        abPopupItems[1].label,
> +        "the correct address book is selected in the drop down menu"
> +      );

This doesn't test what you say it does. It only proves that a menulist does what a menulist does.

What you should test here is that abPopup.label == "Mochitest Address Book" and that abPopup.value is the book's URI.

@@ +134,5 @@
> +      EventUtils.sendKey("TAB", mlWindow);
> +      EventUtils.sendString(
> +        listInputs.addresses.slice(0, 2).join(", "),
> +        mlWindow
> +      );

This could use a comment about adding two addresses to the list.

@@ +157,5 @@
> +    "address zero was saved to the address book"
> +  );
> +  ok(
> +    DisplayNameUtils.getCardForEmail(listInputs.addresses[1]).card,
> +    "address one was saved to the address book"

Hmm. We know the card exists with the right address. Do we know it exists in the right address book?

@@ +160,5 @@
> +    DisplayNameUtils.getCardForEmail(listInputs.addresses[1]).card,
> +    "address one was saved to the address book"
> +  );
> +
> +  let mailList = MailUtils.findListInAddressBooks(listInputs.name);

Again, do we know the list got added to the right address book?

@@ +261,5 @@
> +      // Delete the address in the second row (address one).
> +      let counter = 4;
> +      while (counter) {
> +        EventUtils.sendKey("UP", mlWindow);
> +        await new Promise(resolve => mlWindow.setTimeout(resolve));

What's going on here isn't clear. Better to explicitly click on the row you want then make a change. You can send Ctrl+A to select all the text with EventUtils.synthesizeKey("a", { accelKey: true }, mlWindow);

@@ +330,5 @@
> +  listCards = [...fixIterator(mailList.addressLists, Ci.nsIAbCard)];
> +
> +  ok(
> +    listCards[0].hasEmailAddress(listInputs.addresses[0]),
> +    "address zero was saved in the mailing list (is still there)"

You haven't checked address 1 was removed.

@@ +355,5 @@
> +      let listNickName = mlDocument.getElementById("ListNickName");
> +      let listDescription = mlDocument.getElementById("ListDescription");
> +      let addressInput1 = mlDocument.getElementById("addressCol1#1");
> +      let addressInput2 = mlDocument.getElementById("addressCol1#2");
> +      let addressInput3 = mlDocument.getElementById("addressCol1#3");

You should check there are no more addresses in the list.

Comment 12

[Paul Morris [:pmorris]]

Attachment: mailing-list-tests-6.patch

Thanks for the review. I'm figuring out the quirks of testing with mochitest as I go, so it's... a process. This patch addresses your comments.

(In reply to Geoff Lankow (:darktrojan) from comment #11)

More general comments about testing:
It's harder to read a test if you have to keep in mind the current state of
the UI (ie. knowing what each key press does). If you want to change a
textbox or click on a button, go directly to it and do what you want. Sure,
test that pressing keys does the right thing (especially in weird UI like
this) but actually test it if you do (ie. input 2 has focus, press tab, test
that input 3 has focus).

Makes sense. Earlier I wasn't having much luck with the clicking approach because I hadn't figured out to say do more than one key at a time, like for ctrl+A, (not exactly well documented...). So I was relying on keyboard navigation to tab to inputs to get the text selected. But that's all in the past now.

Also, don't be afraid to write a completely separate test if there's
something else that needs testing. Tests are cheap. You don't need to do
everything in one go.

Okay, I've broken this up a bit.

Comment 13

[Geoff Lankow (:darktrojan)]

Comment on attachment 9108178 [details] [diff] [review]
mailing-list-tests-6.patch

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

This is pretty close. Just a few bits to tidy up.

::: mailnews/addrbook/test/browser/browser_mailing_lists.js
@@ +243,5 @@
> +      // Modify the list's name, nick name, and description fields.
> +      let modifyField = id => {
> +        EventUtils.synthesizeMouseAtCenter(id, { clickCount: 1 }, mlWindow);
> +        EventUtils.synthesizeKey("a", { accelKey: true }, mlWindow);
> +        EventUtils.sendKey("RIGHT", mlWindow);

Probably the end key would be better. You don't need the Ctrl+A, either.

@@ +258,5 @@
> +  // Double-click on the address book name to reveal the mailing list.
> +  dirTreeClick(2, 0, { clickCount: 2 });
> +
> +  // Open the mailing list dialog, the callback above interacts with it.
> +  dirTreeClick(3, 0, { clickCount: 2 });

We should be sure we're clicking on the right thing. Use something like this to get the text in the tree, and check it's what we expect:
tree.view.getCellText(row, tree.columns[0])

(Here and elsewhere.)

@@ +288,5 @@
> +  let mailList = MailUtils.findListInAddressBooks(
> +    inputs.name + inputs.modification
> +  );
> +
> +  ok(mailList, "mailing list still exists");

It still exists, but is it the same one as before? We might have created a new one.

@@ +326,5 @@
> +  );
> +
> +  let hasAddressOne = listCards.reduce((hasAddrOne, card) => {
> +    return hasAddrOne || card.hasEmailAddress(inputs.addresses[1]);
> +  }, false);

Just do listCards.find?

::: mailnews/addrbook/test/browser/head.js
@@ +3,5 @@
> + * file, you can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/* globals MailServices */
> +
> +/* exported createNewAddressBook, openAddressBook */

Not needed any more.

Comment 14

[Paul Morris [:pmorris]]

Attachment: mailing-list-tests-7.patch

Thanks again for the review. This addresses your comments and makes a couple other improvements like using a global object and refactoring the dirTreeClick function.

Comment 15

[Paul Morris [:pmorris]]

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

Comment 16

[Geoff Lankow (:darktrojan)]

Comment on attachment 9108817 [details] [diff] [review]
mailing-list-tests-7.patch

I'm starting to think you just enjoy changing stuff. Let's land this and move on to the next thing.

Comment 17

[Paul Morris [:pmorris]]

Thanks, only when we're talking about at least incremental improvements. :) All for landing this and moving on.
...and jorgk beat me to adding the checkin flag.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/54d186205801
Add a mochitest for mailing lists. r=darktrojan