[geary] Make Geary.App.Conversation unit testable, add some tests.
- From: Michael Gratton <mjog src gnome org>
- To: commits-list gnome org
- Cc:
- Subject: [geary] Make Geary.App.Conversation unit testable, add some tests.
- Date: Mon, 18 Dec 2017 03:49:31 +0000 (UTC)
commit 57f40ffec3c45466c854fd009c441c44470b93cd
Author: Michael James Gratton <mike vee net>
Date: Wed Dec 6 17:02:20 2017 +1100
Make Geary.App.Conversation unit testable, add some tests.
* src/engine/app/app-conversation.vala (Conversation): We currently can't
easily unit test Conversation instances since the ctor requires a
ConversationMonitor argument, which is complicated. Replace the monitor
arg with a base folder (which is what it actually needs the monitor for
anyway) and remove the signals in favour of adding and modifying the
API to allow folder paths to be explicitly updated. Remove
clear_owner() method and dtor now it's useless. Update call sites. Add
unit tests to make sure the add/remove and multi-path related code
still works at least.
src/engine/app/app-conversation-monitor.vala | 10 +-
src/engine/app/app-conversation.vala | 202 ++++++++++----------
.../conversation-monitor/app-conversation-set.vala | 11 +-
test/CMakeLists.txt | 1 +
test/engine/app/app-conversation-test.vala | 141 ++++++++++++++
test/test-engine.vala | 1 +
6 files changed, 251 insertions(+), 115 deletions(-)
---
diff --git a/src/engine/app/app-conversation-monitor.vala b/src/engine/app/app-conversation-monitor.vala
index c974a99..5f4da5c 100644
--- a/src/engine/app/app-conversation-monitor.vala
+++ b/src/engine/app/app-conversation-monitor.vala
@@ -179,15 +179,7 @@ public class Geary.App.ConversationMonitor : BaseObject {
this.required_fields = required_fields | REQUIRED_FIELDS;
_min_window_count = min_window_count;
}
-
- ~ConversationMonitor() {
- if (is_monitoring)
- debug("Warning: Conversations object destroyed without stopping monitoring");
-
- // Manually detach all the weak refs in the Conversation objects
- conversations.clear_owners();
- }
-
+
protected virtual void notify_monitoring_started() {
monitoring_started();
}
diff --git a/src/engine/app/app-conversation.vala b/src/engine/app/app-conversation.vala
index cb9b2c8..56a79d0 100644
--- a/src/engine/app/app-conversation.vala
+++ b/src/engine/app/app-conversation.vala
@@ -40,12 +40,15 @@ public class Geary.App.Conversation : BaseObject {
private static int next_convnum = 0;
+ /** Folder from which the conversation originated. */
+ public Folder base_folder { get; private set; }
+
private Gee.HashMultiSet<RFC822.MessageID> message_ids = new Gee.HashMultiSet<RFC822.MessageID>();
-
+
private int convnum;
- private weak Geary.App.ConversationMonitor? owner;
+
private Gee.HashMap<EmailIdentifier, Email> emails = new Gee.HashMap<EmailIdentifier, Email>();
-
+
// this isn't ideal but the cost of adding an email to multiple sorted sets once versus
// the number of times they're accessed makes it worth it
private Gee.SortedSet<Email> sent_date_ascending = new Gee.TreeSet<Email>(
@@ -75,30 +78,16 @@ public class Geary.App.Conversation : BaseObject {
* Fired when the flags of an email in this conversation have changed.
*/
public signal void email_flags_changed(Geary.Email email);
-
- public Conversation(Geary.App.ConversationMonitor owner) {
- convnum = next_convnum++;
- this.owner = owner;
-
- owner.email_flags_changed.connect(on_email_flags_changed);
- owner.folder.account.email_discovered.connect(on_email_discovered);
- owner.folder.account.email_removed.connect(on_email_removed);
- }
-
- ~Conversation() {
- clear_owner();
- }
-
- internal void clear_owner() {
- if (owner != null) {
- owner.email_flags_changed.disconnect(on_email_flags_changed);
- owner.folder.account.email_discovered.disconnect(on_email_discovered);
- owner.folder.account.email_removed.disconnect(on_email_removed);
- }
-
- owner = null;
+
+
+ /**
+ * Constructs a conversation relative to the given base folder.
+ */
+ internal Conversation(Geary.Folder base_folder) {
+ this.convnum = Conversation.next_convnum++;
+ this.base_folder = base_folder;
}
-
+
/**
* Returns the number of emails in the conversation.
*/
@@ -187,12 +176,24 @@ public class Geary.App.Conversation : BaseObject {
return email;
}
+ /**
+ * Determines if the given id is in the conversation's base folder.
+ */
public bool is_in_current_folder(Geary.EmailIdentifier id) {
- Gee.Collection<Geary.FolderPath>? paths = path_map.get(id);
+ Gee.Collection<Geary.FolderPath>? paths = this.path_map.get(id);
+ return (paths != null && paths.contains(this.base_folder.path));
+ }
- return (paths != null &&
- owner != null &&
- paths.contains(owner.folder.path));
+ /**
+ * Determines if the given id is in the conversation's base folder.
+ */
+ public uint get_folder_count(Geary.EmailIdentifier id) {
+ Gee.Collection<Geary.FolderPath>? paths = this.path_map.get(id);
+ uint count = 0;
+ if (paths != null) {
+ count = paths.size;
+ }
+ return count;
}
/**
@@ -218,62 +219,88 @@ public class Geary.App.Conversation : BaseObject {
public Gee.Collection<Geary.EmailIdentifier> get_email_ids() {
return emails.keys;
}
-
+
/**
- * Add the email to the conversation if it wasn't already in there. Return
- * whether it was added.
+ * Add the email to the conversation if not already present.
+ *
+ * The value of `known_paths` should contain all the known {@link
+ * FolderPaths} this email is contained within.
*
- * known_paths should contain all the known FolderPaths this email is contained in.
- * Conversation will monitor Account for additions and removals as they occur.
+ * Returns if the email was added, else false if already present
+ * and only `known_paths` were merged.
*/
internal bool add(Email email, Gee.Collection<Geary.FolderPath> known_paths) {
- if (emails.has_key(email.id))
- return false;
-
- emails.set(email.id, email);
- sent_date_ascending.add(email);
- sent_date_descending.add(email);
- recv_date_ascending.add(email);
- recv_date_descending.add(email);
-
- Gee.Set<RFC822.MessageID>? ancestors = email.get_ancestors();
- if (ancestors != null)
- message_ids.add_all(ancestors);
-
+ // Add the known paths to the path map regardless of whether
+ // the email is already in the conversation or not, so that it
+ // remains complete
foreach (Geary.FolderPath path in known_paths)
- path_map.set(email.id, path);
-
- appended(email);
-
- return true;
+ this.path_map.set(email.id, path);
+
+ bool added = false;
+ if (!emails.has_key(email.id)) {
+ this.emails.set(email.id, email);
+ this.sent_date_ascending.add(email);
+ this.sent_date_descending.add(email);
+ this.recv_date_ascending.add(email);
+ this.recv_date_descending.add(email);
+
+ Gee.Set<RFC822.MessageID>? ancestors = email.get_ancestors();
+ if (ancestors != null)
+ message_ids.add_all(ancestors);
+
+ appended(email);
+ added = true;
+ }
+ return added;
}
-
- // Returns the removed Message-IDs
+
+ /**
+ * Unconditionally removes an email from the conversation.
+ *
+ * Returns all Message-IDs that should be removed as result of
+ * removing this message, or `null` if none were removed.
+ */
internal Gee.Set<RFC822.MessageID>? remove(Email email) {
- emails.unset(email.id);
- sent_date_ascending.remove(email);
- sent_date_descending.remove(email);
- recv_date_ascending.remove(email);
- recv_date_descending.remove(email);
- path_map.remove_all(email.id);
-
- Gee.Set<RFC822.MessageID> removed_message_ids = new Gee.HashSet<RFC822.MessageID>();
-
- Gee.Set<RFC822.MessageID>? ancestors = email.get_ancestors();
- if (ancestors != null) {
- foreach (RFC822.MessageID ancestor_id in ancestors) {
- // if remove() changes set (i.e. it was present) but no longer present, that
- // means the ancestor_id was the last one and is formally removed
- if (message_ids.remove(ancestor_id) && !message_ids.contains(ancestor_id))
- removed_message_ids.add(ancestor_id);
+ Gee.Set<RFC822.MessageID>? removed_ids = null;
+
+ if (emails.unset(email.id)) {
+ this.sent_date_ascending.remove(email);
+ this.sent_date_descending.remove(email);
+ this.recv_date_ascending.remove(email);
+ this.recv_date_descending.remove(email);
+ this.path_map.remove_all(email.id);
+
+ Gee.Set<RFC822.MessageID>? ancestors = email.get_ancestors();
+ if (ancestors != null) {
+ removed_ids = new Gee.HashSet<RFC822.MessageID>();
+ foreach (RFC822.MessageID ancestor_id in ancestors) {
+ // if remove() changes set (i.e. it was present) but no longer present, that
+ // means the ancestor_id was the last one and is formally removed
+ if (message_ids.remove(ancestor_id) &&
+ !message_ids.contains(ancestor_id)) {
+ removed_ids.add(ancestor_id);
+ }
+ }
+
+ if (removed_ids.size == 0) {
+ removed_ids = null;
+ }
}
+
+
+ trimmed(email);
}
-
- trimmed(email);
-
- return (removed_message_ids.size > 0) ? removed_message_ids : null;
+
+ return removed_ids;
}
-
+
+ /**
+ * Removes the target path from the known set for the given id.
+ */
+ internal void remove_path(Geary.EmailIdentifier id, FolderPath path) {
+ this.path_map.remove(id, path);
+ }
+
/**
* Returns true if *any* message in the conversation is unread.
*/
@@ -322,7 +349,7 @@ public class Geary.App.Conversation : BaseObject {
public Geary.Email? get_latest_recv_email(Location location) {
return get_single_email(Ordering.RECV_DATE_DESCENDING, location);
}
-
+
private Geary.Email? get_single_email(Ordering ordering, Location location) {
// note that the location-ordering preferences are treated as ANYWHERE by get_emails()
Gee.Collection<Geary.Email> all = get_emails(ordering, location);
@@ -370,26 +397,7 @@ public class Geary.App.Conversation : BaseObject {
private bool is_missing_flag(Geary.NamedFlag flag) {
return check_flag(flag, false);
}
-
- private void on_email_flags_changed(Conversation conversation, Geary.Email email) {
- if (conversation == this)
- email_flags_changed(email);
- }
-
- private void on_email_discovered(Geary.Folder folder, Gee.Collection<Geary.EmailIdentifier> ids) {
- // only add to the internal map if a part of this Conversation
- foreach (Geary.EmailIdentifier id in ids) {
- if (emails.has_key(id))
- path_map.set(id, folder.path);
- }
- }
-
- private void on_email_removed(Geary.Folder folder, Gee.Collection<Geary.EmailIdentifier> ids) {
- // To be forgiving, simply remove id without checking if it's a part of this Conversation
- foreach (Geary.EmailIdentifier id in ids)
- path_map.remove(id, folder.path);
- }
-
+
public string to_string() {
return "[#%d] (%d emails)".printf(convnum, emails.size);
}
diff --git a/src/engine/app/conversation-monitor/app-conversation-set.vala
b/src/engine/app/conversation-monitor/app-conversation-set.vala
index 62f3aff..16c6afc 100644
--- a/src/engine/app/conversation-monitor/app-conversation-set.vala
+++ b/src/engine/app/conversation-monitor/app-conversation-set.vala
@@ -54,12 +54,7 @@ private class Geary.App.ConversationSet : BaseObject {
public Conversation? get_by_email_identifier(Geary.EmailIdentifier id) {
return email_id_map.get(id);
}
-
- public void clear_owners() {
- foreach (Conversation conversation in _conversations)
- conversation.clear_owner();
- }
-
+
// Returns a Collection of zero or more Conversations that have Message-IDs associated with
// the ancestors of the supplied Email ... if more than one, then add_email() should not be
// called
@@ -105,7 +100,7 @@ private class Geary.App.ConversationSet : BaseObject {
conversation = Collection.get_first<Conversation>(associated);
if (conversation == null) {
- conversation = new Conversation(monitor);
+ conversation = new Conversation(monitor.folder);
_conversations.add(conversation);
added_conversation = true;
@@ -283,8 +278,6 @@ private class Geary.App.ConversationSet : BaseObject {
if (!_conversations.remove(conversation))
error("Conversation %s already removed from set", conversation.to_string());
-
- conversation.clear_owner();
}
private Conversation? remove_email_by_identifier(Geary.EmailIdentifier id,
diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
index fb39f5d..77ae73d 100644
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -11,6 +11,7 @@ set(TEST_ENGINE_SRC
engine/api/geary-email-identifier-test.vala
engine/api/geary-folder-test.vala
engine/api/geary-folder-path-test.vala
+ engine/app/app-conversation-test.vala
engine/imap/command/imap-create-command-test.vala
engine/imap/response/imap-namespace-response-test.vala
engine/imap/transport/imap-deserializer-test.vala
diff --git a/test/engine/app/app-conversation-test.vala b/test/engine/app/app-conversation-test.vala
new file mode 100644
index 0000000..c189705
--- /dev/null
+++ b/test/engine/app/app-conversation-test.vala
@@ -0,0 +1,141 @@
+/*
+ * Copyright 2017 Michael Gratton <mike vee net>
+ *
+ * This software is licensed under the GNU Lesser General Public License
+ * (version 2.1 or later). See the COPYING file in this distribution.
+ */
+
+class Geary.App.ConversationTest : Gee.TestCase {
+
+
+ Conversation? test = null;
+ Folder? base_folder = null;
+
+ public ConversationTest() {
+ base("Geary.App.ConversationTest");
+ add_test("add_basic", add_basic);
+ add_test("add_duplicate", add_duplicate);
+ add_test("add_multipath", add_multipath);
+ add_test("remove_basic", remove_basic);
+ add_test("remove_nonexistent", remove_nonexistent);
+ }
+
+ public override void set_up() {
+ this.base_folder = new MockFolder(
+ null,
+ null,
+ new MockFolderRoot("test"),
+ SpecialFolderType.NONE,
+ null
+ );
+ this.test = new Conversation(this.base_folder);
+ }
+
+ public void add_basic() {
+ Geary.Email e1 = new Email(new MockEmailIdentifer(1));
+ Geary.Email e2 = new Email(new MockEmailIdentifer(2));
+ uint appended = 0;
+ this.test.appended.connect(() => {
+ appended++;
+ });
+
+ assert(this.test.add(e1, singleton(this.base_folder.path)) == true);
+ assert(this.test.is_in_current_folder(e1.id) == true);
+ assert(this.test.get_folder_count(e1.id) == 1);
+ assert(appended == 1);
+ assert(this.test.get_count() == 1);
+
+ assert(this.test.add(e2, singleton(this.base_folder.path)) == true);
+ assert(this.test.is_in_current_folder(e2.id) == true);
+ assert(this.test.get_folder_count(e2.id) == 1);
+ assert(appended == 2);
+ assert(this.test.get_count() == 2);
+ }
+
+ public void add_duplicate() {
+ Geary.Email e1 = new Email(new MockEmailIdentifer(1));
+ uint appended = 0;
+ this.test.appended.connect(() => {
+ appended++;
+ });
+
+ assert(this.test.add(e1, singleton(this.base_folder.path)) == true);
+ assert(appended == 1);
+ assert(this.test.get_count() == 1);
+
+ assert(this.test.add(e1, singleton(this.base_folder.path)) == false);
+ assert(appended == 1);
+ assert(this.test.get_count() == 1);
+ }
+
+ public void add_multipath() {
+ Geary.Email e1 = new Email(new MockEmailIdentifer(1));
+ this.test.add(e1, singleton(this.base_folder.path));
+
+ Geary.Email e2 = new Email(new MockEmailIdentifer(2));
+ this.test.add(e2, singleton(this.base_folder.path));
+
+ FolderRoot other_path = new MockFolderRoot("other");
+ Gee.LinkedList<FolderRoot> other_paths = new Gee.LinkedList<FolderRoot>();
+ other_paths.add(other_path);
+
+ assert(this.test.add(e1, other_paths) == false);
+ assert(this.test.is_in_current_folder(e1.id) == true);
+ assert(this.test.get_folder_count(e1.id) == 2);
+
+ assert(this.test.is_in_current_folder(e2.id) == true);
+ assert(this.test.get_folder_count(e2.id) == 1);
+
+ this.test.remove_path(e1.id, other_path);
+ assert(this.test.is_in_current_folder(e1.id) == true);
+ assert(this.test.get_folder_count(e1.id) == 1);
+ }
+
+ public void remove_basic() {
+ Geary.Email e1 = new Email(new MockEmailIdentifer(1));
+ this.test.add(e1, singleton(this.base_folder.path));
+
+ Geary.Email e2 = new Email(new MockEmailIdentifer(2));
+ this.test.add(e2, singleton(this.base_folder.path));
+
+ uint trimmed = 0;
+ this.test.trimmed.connect(() => {
+ trimmed++;
+ });
+
+ assert(this.test.remove(e1) == null);
+ assert(trimmed == 1);
+ assert(this.test.get_count() == 1);
+
+ assert(this.test.remove(e2) == null);
+ assert(trimmed == 2);
+ assert(this.test.get_count() == 0);
+ }
+
+ public void remove_nonexistent() {
+ Geary.Email e1 = new Email(new MockEmailIdentifer(1));
+ Geary.Email e2 = new Email(new MockEmailIdentifer(2));
+
+ uint trimmed = 0;
+ this.test.trimmed.connect(() => {
+ trimmed++;
+ });
+
+ assert(this.test.remove(e2) == null);
+ assert(trimmed == 0);
+ assert(this.test.get_count() == 0);
+
+ this.test.add(e1, singleton(this.base_folder.path));
+
+ assert(this.test.remove(e2) == null);
+ assert(trimmed == 0);
+ assert(this.test.get_count() == 1);
+ }
+
+ private Gee.Collection<E> singleton<E>(E element) {
+ Gee.LinkedList<E> collection = new Gee.LinkedList<E>();
+ collection.add(element);
+ return collection;
+ }
+
+}
diff --git a/test/test-engine.vala b/test/test-engine.vala
index e01dfd2..28122f8 100644
--- a/test/test-engine.vala
+++ b/test/test-engine.vala
@@ -26,6 +26,7 @@ int main(string[] args) {
engine.add_suite(new Geary.EngineTest().get_suite());
engine.add_suite(new Geary.IdleManagerTest().get_suite());
engine.add_suite(new Geary.TimeoutManagerTest().get_suite());
+ engine.add_suite(new Geary.App.ConversationTest().get_suite());
engine.add_suite(new Geary.HTML.UtilTest().get_suite());
engine.add_suite(new Geary.Imap.DeserializerTest().get_suite());
engine.add_suite(new Geary.Imap.CreateCommandTest().get_suite());
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]
