Closed Bug 1608304 Opened 4 years ago Closed 4 years ago

Create a mock LDAP server and some tests


(MailNews Core :: LDAP Integration, task)

Not set


(Not tracked)

Thunderbird 74.0


(Reporter: darktrojan, Assigned: darktrojan)



(3 files)

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.

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

Attachment #9119925 - Flags: review?(mkmelin+mozilla)

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.

Attachment #9119927 - Flags: review?(mkmelin+mozilla)

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.
Attachment #9119930 - Flags: review?(mkmelin+mozilla)
Attachment #9119925 - Flags: review?(mkmelin+mozilla) → review+
Comment on attachment 9119927 [details] [diff] [review]

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 */
> +
> +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.
Attachment #9119927 - Flags: review?(mkmelin+mozilla) → review+
Comment on attachment 9119930 [details] [diff] [review]

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

LGTM thx! r=mkmelin
Attachment #9119930 - Flags: review?(mkmelin+mozilla) → review+

Pushed by
part 1 - Move test of address book UI from mailnews/addrbook to mail/components/addrbook; r=mkmelin
part 2 - Create a mock LDAP server and some tests; r=mkmelin
part 3 - Fix offline replication of LDAP address books; r=mkmelin

Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 74.0
You need to log in before you can comment on or make changes to this bug.