1608304 - Create a mock LDAP server and some tests

Status: RESOLVED FIXED

(MailNews Core :: LDAP Integration, task)

Description

[Geoff Lankow (:darktrojan)]

Most of the LDAP code has zero test coverage. In this bug I'll create a mock server and some basic functionality tests. Then I'll fix offline replication of LDAP address books, which I suspected was broken, and can now prove with a test.

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: 1608304-pt1-move-tests-1.diff

We created this test in the wrong place. It should be under mail/, where the code it tests is. I also found a pointless moz.build line so I removed it and the same line in another file.

Comment 2

[Geoff Lankow (:darktrojan)]

Attachment: 1608304-pt2-create-ldap-tests-1.diff

This part adds the mock server and two tests – one that tests the address book UI works correctly when searching for a contact, and one that tests that offline replication works.

Comment 3

[Geoff Lankow (:darktrojan)]

Attachment: 1608304-pt3-fix-replication-1.diff

This fixes the offline replication, apart from a weird unknown failure when run in debug mode.

There's a lot going on here:

• The replication directory is now instantiated directly instead of through the AB manager. This prevents the AB manager from knowing about it and doing unwanted things like caching.
• When making a second or subsequent replication, we write to a new file and only then, if successful, delete the old one and move the new one into place. This saves a lot of hassle moving things around.
• The SQLite connection needs to be closed before files can move around. I've done this by firing an observer service notification requesting closure and (because it's async) another one responding when it has closed.
• I've used MozPromise to handle the async stuff in C++. This is completely new to me but it seems to work.

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 9119927 [details] [diff] [review]
1608304-pt2-create-ldap-tests-1.diff

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

::: mail/components/addrbook/content/addressbook.js
@@ +141,5 @@
>      ResultsPaneSelectionChanged();
>    },
>    onCountChanged(total) {
>      SetStatusText(total);
> +    window.dispatchEvent(new CustomEvent("countChanged"));

event names are usually alllowercase, and no past tense
Maybe countchange or abviewcountchange? Might as well put total into the event detail

new CustomEvent("countchange", detail: { total });

::: mailnews/addrbook/test/LDAPServer.jsm
@@ +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/. */
> +
> +var EXPORTED_SYMBOLS = ["LDAPServer"];

inside a jsm, so you can make this const

@@ +6,5 @@
> +var PRINT_DEBUG = false;
> +
> +var { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm");
> +
> +var serverSocket;

looks like this should just be a member of LDAPServer

@@ +7,5 @@
> +
> +var { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm");
> +
> +var serverSocket;
> +var LDAPServer = {

please add some more documentation to this jsm while it's fresh in memory.

Comment 5

[Magnus Melin [:mkmelin]]

Comment on attachment 9119930 [details] [diff] [review]
1608304-pt3-fix-replication-1.diff

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

LGTM thx! r=mkmelin

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d85625eb94fb
part 1 - Move test of address book UI from mailnews/addrbook to mail/components/addrbook; r=mkmelin
https://hg.mozilla.org/comm-central/rev/f39320031868
part 2 - Create a mock LDAP server and some tests; r=mkmelin
https://hg.mozilla.org/comm-central/rev/46ed6cdc30b5
part 3 - Fix offline replication of LDAP address books; r=mkmelin