lib: improve test coverage

Add a bunch of tests

Makefile.meson

@@ -88,9 +88,10 @@ coverage: $(COVERAGE_BUILDDIR)
 	$(NINJA) -C $(COVERAGE_BUILDDIR) test
 	lcov --capture --directory . --output-file $(covfile)
 	@lcov --remove $(covfile) '/usr/*' '*guile*' '*thirdparty*' '*/tests/*' '*mime-object*' --output $(covfile)
+	@lcov --remove $(covfile) '*mu/*' --output $(covfile)
 	@mkdir -p $(COVERAGE_BUILDDIR)/meson-logs/coverage
 	@genhtml $(covfile) --output-directory $(COVERAGE_BUILDDIR)/meson-logs/coverage/
-	@echo "coverage report at: file://$(COVERAGE_BUILDDIR)/meson/logs/coverage/index.html"
+	@echo "coverage report at: file://$(COVERAGE_BUILDDIR)/meson-logs/coverage/index.html"
 dist: $(BUILDDIR)
 	@cd $(BUILDDIR); $(MESON) dist
 

lib/message/mu-contact.cc

@@ -169,6 +169,30 @@ test_encode()
 }
 
 
+static void
+test_sender()
+{
+	Contact c{"aa@example.com", "Anders Ångström",
+		Contact::Type::Sender, 54321};
+
+	assert_equal(c.email, "aa@example.com");
+	assert_equal(c.name, "Anders Ångström");
+	g_assert_false(c.personal);
+	g_assert_cmpuint(c.frequency,==,1);
+	g_assert_cmpuint(c.message_date,==,54321);
+
+	g_assert_false(!!c.field_id());
+}
+
+
+static void
+test_misc()
+{
+	g_assert_false(!!contact_type_from_field_id(Field::Id::Subject));
+
+}
+
+
 int
 main(int argc, char* argv[])
 {
@@ -180,6 +204,9 @@ main(int argc, char* argv[])
 	g_test_add_func("/message/contact/ctor-cleanup", test_ctor_cleanup);
 	g_test_add_func("/message/contact/encode", test_encode);
 
+	g_test_add_func("/message/contact/sender", test_sender);
+	g_test_add_func("/message/contact/misc", test_misc);
+
 	return g_test_run();
 }
 #endif /*BUILD_TESTS*/

lib/message/mu-message.cc

@@ -124,12 +124,17 @@ Message::Message(const std::string& text, const std::string& path,
 		 Message::Options opts):
 	priv_{std::make_unique<Private>(opts)}
 {
+	if (text.empty())
+		throw Error{Error::Code::InvalidArgument, "text must not be empty"};
+
 	if (!path.empty()) {
 		auto xpath{to_string_opt_gchar(g_canonicalize_filename(path.c_str(), {}))};
 		if (xpath)
 			priv_->doc.add(Field::Id::Path, std::move(xpath.value()));
 	}
 
+	priv_->ctime = ::time({});
+
 	priv_->doc.add(Field::Id::Size, static_cast<int64_t>(text.size()));
 
 	init_gmime();
@@ -727,9 +732,10 @@ fill_document(Message::Private& priv)
 		case Field::Id::XBodyHtml:
 			doc.add(field.id, priv.body_html);
 			break;
-			/* ignore */
+		/* LCOV_EXCL_START */
 		case Field::Id::_count_:
 			break;
+		/* LCOV_EXCL_STOP */
 		}
 #pragma GCC diagnostic pop
 
@@ -739,27 +745,21 @@ fill_document(Message::Private& priv)
 Option<std::string>
 Message::header(const std::string& header_field) const
 {
-	if (!load_mime_message())
-		return Nothing;
-
+	load_mime_message();
 	return priv_->mime_msg->header(header_field);
 }
 
 Option<std::string>
 Message::body_text() const
 {
-	if (!load_mime_message())
-		return {};
-
+	load_mime_message();
 	return priv_->body_txt;
 }
 
 Option<std::string>
 Message::body_html() const
 {
-	if (!load_mime_message())
-		return {};
-
+	load_mime_message();
 	return priv_->body_html;
 }
 

lib/message/mu-message.hh

@@ -43,8 +43,8 @@ public:
 	enum struct Options {
 		None	     = 0,	/**< Defaults */
 		Decrypt	     = 1 << 0,	/**< Attempt to decrypt */
-		RetrieveKeys = 1 << 1, /**< Auto-retrieve crypto keys (implies network
-					* access) */
+		RetrieveKeys = 1 << 1,  /**< Auto-retrieve crypto keys (implies network
+					   * access) */
 	};
 
 	/**
@@ -77,9 +77,13 @@ public:
 		return Ok(Message{path,opts});
 	} catch (Error& err) {
 		return Err(err);
-	} catch (...) {
-		return Err(Mu::Error(Error::Code::Message, "failed to create message"));
 	}
+	/* LCOV_EXCL_START */
+	catch (...) {
+		return Err(Mu::Error(Error::Code::Message,
+				     "failed to create message from path"));
+	}
+	/* LCOV_EXCL_STOP */
 
 	/**
 	 * Construct a message based on a Xapian::Document
@@ -92,13 +96,18 @@ public:
 		return Ok(Message{std::move(doc)});
 	} catch (Error& err) {
 		return Err(err);
-	} catch (...) {
-		return Err(Mu::Error(Error::Code::Message, "failed to create message"));
 	}
+	/* LCOV_EXCL_START */
+	catch (...) {
+		return Err(Mu::Error(Error::Code::Message,
+				     "failed to create message from document"));
+	}
+	/* LCOV_EXCL_STOP */
+
 
 	/**
 	 * Construct a message from a string. This is mostly useful for testing.
-
+	 *
 	 * @param text message text
 	 * @param path path to message - optional; path does not have to exist.
 	 * @param opts options
@@ -111,10 +120,13 @@ public:
 		return Ok(Message{text, path, opts});
 	} catch (Error& err) {
 		return Err(err);
-	} catch (...) {
+	}
+	/* LCOV_EXCL_START */
+	catch (...) {
 		return Err(Mu::Error(Error::Code::Message,
 				     "failed to create message from text"));
 	}
+	/* LCOV_EXCL_STOP */
 
 	/**
 	 * DTOR
@@ -233,9 +245,11 @@ public:
 	}
 
 	/**
-	 * get the last change-time (ctime) for this message
+	 * get the last change-time this message. For path/document-based
+	 * messages this corresponds with the ctime of the underlying file; for
+	 * the text-based ones (as used for testing) it is the creation time.
 	 *
-	 * @return chnage time or 0 if unknown
+	 * @return last-change time or 0 if unknown
 	 */
 	::time_t changed() const {
 		return static_cast<::time_t>(document().integer_value(Field::Id::Changed));

lib/message/test-mu-message.cc

@@ -65,7 +65,6 @@ goto * instructions[pOp->opcode];
 			test_message_1,
 			"/home/test/Maildir/inbox/cur/1649279256.107710_1.evergrey:2,S")};
 	g_assert_true(!!message);
-
 	assert_equal(message->path(),
 		     "/home/test/Maildir/inbox/cur/1649279256.107710_1.evergrey:2,S");
 	g_assert_true(message->maildir().empty());
@@ -99,6 +98,10 @@ goto * instructions[pOp->opcode];
 	g_assert_true(message->priority() == Priority::Low);
 	g_assert_cmpuint(message->size(),==,::strlen(test_message_1));
 
+	/* text-based message use time({}) as their changed-time */
+	g_assert_cmpuint(message->changed() - ::time({}), >=, 0);
+	g_assert_cmpuint(message->changed() - ::time({}), <=, 2);
+
 	g_assert_true(message->references().empty());
 
 	assert_equal(message->subject(),
@@ -177,7 +180,7 @@ World!
 
 	auto message{Message::make_from_text(msg_text)};
 	g_assert_true(!!message);
-
+	g_assert_true(message->has_mime_message());
 	g_assert_true(message->path().empty());
 
 	g_assert_true(message->bcc().empty());
@@ -202,6 +205,10 @@ World!
 	g_assert_true(message->priority() == Priority::Normal);
 	g_assert_cmpuint(message->size(),==,::strlen(msg_text));
 
+	/* text-based message use time({}) as their changed-time */
+	g_assert_cmpuint(message->changed() - ::time({}), >=, 0);
+	g_assert_cmpuint(message->changed() - ::time({}), <=, 2);
+
 	assert_equal(message->subject(), "ättächmeñts");
 
 	const auto cache_path{message->cache_path()};
@@ -565,6 +572,20 @@ Moi,
 }
 
 
+static void
+test_message_fail ()
+{
+	{
+		const auto msg = Message::make_from_path("/root/non-existent-path-12345");
+		g_assert_false(!!msg);
+	}
+
+	{
+		const auto msg = Message::make_from_text("", "");
+		g_assert_false(!!msg);
+	}
+}
+
 int
 main(int argc, char* argv[])
 {
@@ -582,6 +603,9 @@ main(int argc, char* argv[])
 			test_message_multipart_mixed_rfc822);
 	g_test_add_func("/message/message/detect-attachment",
 			test_message_detect_attachment);
+	g_test_add_func("/message/message/fail",
+			test_message_fail);
+
 
 	return g_test_run();
 }

lib/mu-contacts-cache.cc

@@ -308,9 +308,6 @@ ContactsCache::for_each(const EachContactFunc& each_contact) const
 {
 	std::lock_guard<std::mutex> l_{priv_->mtx_};
 
-	if (!each_contact)
-		return; // nothing to do
-
 	// first sort them for 'rank'
 	ContactSet sorted;
 	for (const auto& item : priv_->contacts_)
@@ -409,6 +406,39 @@ test_mu_contacts_cache_personal()
 }
 
 
+static void
+test_mu_contacts_cache_foreach()
+{
+	Mu::ContactsCache ccache("");
+	ccache.add(Mu::Contact{"a@example.com", "a", 123, true, 1000, 0});
+	ccache.add(Mu::Contact{"b@example.com", "b", 456, true, 1000, 0});
+
+	{
+		size_t n{};
+		g_assert_false(ccache.empty());
+		g_assert_cmpuint(ccache.size(),==,2);
+		ccache.for_each([&](auto&& contact) { ++n; return false; });
+		g_assert_cmpuint(n,==,1);
+	}
+
+	{
+		size_t n{};
+		g_assert_false(ccache.empty());
+		g_assert_cmpuint(ccache.size(),==,2);
+		ccache.for_each([&](auto&& contact) { ++n; return true; });
+		g_assert_cmpuint(n,==,2);
+	}
+
+	{
+		size_t n{};
+		ccache.clear();
+		g_assert_true(ccache.empty());
+		g_assert_cmpuint(ccache.size(),==,0);
+		ccache.for_each([&](auto&& contact) { ++n; return true; });
+		g_assert_cmpuint(n,==,0);
+	}
+}
+
 
 
 static void
@@ -418,6 +448,7 @@ test_mu_contacts_cache_sort()
 		std::string str;
 		if (g_test_verbose())
 			g_print("contacts-cache:\n");
+
 		ccache.for_each([&](auto&& contact) {
 			if (g_test_verbose())
 				g_print("\t- %s\n", contact.display_name().c_str());
@@ -427,7 +458,6 @@ test_mu_contacts_cache_sort()
 		return str;
 	};
 
-
 	const auto now{std::time({})};
 
 	// "first" means more relevant
@@ -473,6 +503,7 @@ main(int argc, char* argv[])
 
 	g_test_add_func("/lib/contacts-cache/base", test_mu_contacts_cache_base);
 	g_test_add_func("/lib/contacts-cache/personal", test_mu_contacts_cache_personal);
+	g_test_add_func("/lib/contacts-cache/for-each", test_mu_contacts_cache_foreach);
 	g_test_add_func("/lib/contacts-cache/sort", test_mu_contacts_cache_sort);
 
 	g_log_set_handler(

lib/mu-query-results.hh

@@ -383,7 +383,6 @@ public:
 	 *
 	 * @return iterator
 	 */
-	iterator       begin() { return QueryResultsIterator(mset_.begin(), query_matches_); }
 	const iterator begin() const { return QueryResultsIterator(mset_.begin(), query_matches_); }
 
 	/**
@@ -391,7 +390,6 @@ public:
 	 *
 	 * @return iterator
 	 */
-	iterator       end() { return QueryResultsIterator(mset_.end(), query_matches_); }
 	const_iterator end() const { return QueryResultsIterator(mset_.end(), query_matches_); }
 
 	/**

lib/mu-store.hh

@@ -70,9 +70,13 @@ public:
 
 	} catch (const Mu::Error& me) {
 		return Err(me);
-	} catch (...) {
+	}
+ /* LCOV_EXCL_START */
+	catch (...) {
 		return Err(Error::Code::Internal, "failed to create store");
 	}
+ /* LCOV_EXCL_STOP */
+
 
 	struct Config {
 		size_t max_message_size{};
@@ -99,9 +103,12 @@ public:
 
 	} catch (const Mu::Error& me) {
 		return Err(me);
-	} catch (...) {
-		return Err(Error::Code::Internal, "failed to create store");
 	}
+ /* LCOV_EXCL_START */
+	catch (...) {
+		return Err(Error::Code::Internal, "failed to create new store");
+	}
+ /* LCOV_EXCL_STOP */
 
 	/**
 	 * Move CTOR

lib/tests/test-mu-maildir.cc

@@ -278,6 +278,77 @@ test_determine_target_ok(void)
 }
 
 
+
+static void
+test_determine_target_fail(void)
+{
+	struct TestCase {
+		std::string old_path;
+		std::string root_maildir;
+		std::string target_maildir;
+		Flags new_flags;
+		bool new_name;
+		std::string expected;
+	};
+	const std::vector<TestCase> testcases = {
+		 TestCase{ /* fail: no absolute path */
+			"../foo/Maildir/test/cur/123456:2,FR-not-absolute",
+			"/home/foo/Maildir",
+			{},
+			Flags::Seen | Flags::Passed,
+			false,
+			"/home/foo/Maildir/test/cur/123456:2,PS"
+		},
+
+		 TestCase{ /* fail: no absolute root */
+			"/home/foo/Maildir/test/cur/123456:2,FR",
+			"../foo/Maildir-not-absolute",
+			{},
+			Flags::New,
+			false,
+			"/home/foo/Maildir/test/new/123456"
+		},
+
+		 TestCase{ /* fail: maildir must start with '/' */
+			"/home/foo/Maildir/test/cur/123456",
+			"/home/foo/Maildir",
+			"mymaildirwithoutslash",
+			Flags::Seen | Flags::Flagged,
+			false,
+			"/home/foo/Maildir/test/cur/123456:2,FS"
+		 },
+
+		 TestCase{ /* fail: path must be below maildir */
+			"/home/foo/Maildir/test/cur/123456:2,FR",
+			"/home/bar/Maildir",
+			"/test2",
+			Flags::Flagged | Flags::Replied,
+			false,
+			"/home/foo/Maildir/test2/cur/123456:2,FR"
+		},
+		 TestCase{ /* fail: New cannot be combined */
+			"/home/foo/Maildir/test/new/123456",
+			"/home/foo/Maildir",
+			{},
+			Flags::New | Flags::Replied,
+			false,
+			"/home/foo/Maildir/test/cur/123456:2,"
+		},
+	};
+
+	for (auto&& testcase: testcases) {
+		const auto res = maildir_determine_target(
+			testcase.old_path,
+			testcase.root_maildir,
+			testcase.target_maildir,
+			testcase.new_flags,
+			testcase.new_name);
+		g_assert_false(!!res);
+	}
+}
+
+
+
 static void
 test_maildir_get_new_path_01(void)
 {
@@ -428,6 +499,8 @@ main(int argc, char* argv[])
 
 	g_test_add_func("/mu-maildir/mu-maildir-determine-target-ok",
 			test_determine_target_ok);
+	g_test_add_func("/mu-maildir/mu-maildir-determine-target-fail",
+			test_determine_target_fail);
 
 	// /* get/set flags */
 	g_test_add_func("/mu-maildir/mu-maildir-get-new-path-01", test_maildir_get_new_path_01);

lib/tests/test-mu-store.cc

@@ -139,6 +139,12 @@ goto * instructions[pOp->opcode];
 	assert_valid_result(docid);
 	g_assert_cmpuint(store->size(),==, 1);
 
+	/* ensure 'update' dtrt, i.e., nothing. */
+	const auto docid2 = store->update_message(*message, *docid);
+	assert_valid_result(docid2);
+	g_assert_cmpuint(store->size(),==, 1);
+	g_assert_cmpuint(*docid,==,*docid2);
+
 	auto msg2{store->find_message(*docid)};
 	g_assert_true(!!msg2);
 	assert_equal(message->path(), msg2->path());
@@ -228,6 +234,11 @@ World!
 	// for (auto&& term = msg2->document().xapian_document().termlist_begin();
 	//      term != msg2->document().xapian_document().termlist_end(); ++term)
 	//	g_message(">>> %s", (*term).c_str());
+
+	const auto stats{store->statistics()};
+	g_assert_cmpuint(stats.size,==,store->size());
+	g_assert_cmpuint(stats.last_index,==,0);
+	g_assert_cmpuint(stats.last_change,>=,::time({}));
 }
 
 
@@ -319,6 +330,23 @@ Yes, that would be excellent.
 }
 
 
+static void
+test_store_fail()
+{
+	{
+		const auto store = Store::make("/root/non-existent-path/12345");
+		g_assert_false(!!store);
+	}
+
+	{
+		const auto store = Store::make_new("/../../root/non-existent-path/12345",
+						   "/../../root/non-existent-path/54321",
+						   {}, {});
+		g_assert_false(!!store);
+
+	}
+}
+
 
 int
 main(int argc, char* argv[])
@@ -331,8 +359,8 @@ main(int argc, char* argv[])
 			test_message_mailing_list);
 	g_test_add_func("/store/message/attachments",
 			test_message_attachments);
-	g_test_add_func("/store/index/move",
-			test_index_move);
+	g_test_add_func("/store/index/move", test_index_move);
+	g_test_add_func("/store/index/fail", test_store_fail);
 
 	if (!g_test_verbose())
 		g_log_set_handler(

lib/utils/mu-utils.cc

@@ -531,7 +531,8 @@ Mu::parse_size(const std::string& val, bool is_first)
 	if (val.empty())
 		return is_first ? 0 : std::numeric_limits<int64_t>::max();
 
-	rx = g_regex_new("(\\d+)(b|k|kb|m|mb|g|gb)?", G_REGEX_CASELESS, (GRegexMatchFlags)0, NULL);
+	rx = g_regex_new("^(\\d+)(b|k|kb|m|mb|g|gb)?$",
+			 G_REGEX_CASELESS, (GRegexMatchFlags)0, NULL);
 	minfo = NULL;
 	if (g_regex_match(rx, val.c_str(), (GRegexMatchFlags)0, &minfo)) {
 

lib/utils/tests/test-utils.cc

@@ -65,7 +65,7 @@ test_date_basic()
 	}
 
 	g_setenv("TZ", hki, TRUE);
-	constexpr std::array<std::tuple<const char*, bool, int64_t>, 9> cases = {{
+	constexpr std::array<std::tuple<const char*, bool/*is_first*/, int64_t>, 13> cases = {{
 			{"2015-09-18T09:10:23", true, 1442556623},
 			{"1972-12-14T09:10:23", true, 93165023},
 			{"1854-11-18T17:10:23", true, 0},
@@ -73,6 +73,12 @@ test_date_basic()
 			{"2000-02-31T09:10:23", true, 951861599},
 			{"2000-02-29T23:59:59", true, 951861599},
 
+			{"20220602", true, 1654117200},
+			{"20220605", false, 1654462799},
+
+			{"202206", true, 1654030800},
+			{"202206", false, 1656622799},
+
 			{"2016", true, 1451599200},
 			{"2016", false, 1483221599},
 
@@ -94,44 +100,55 @@ test_date_basic()
 static void
 test_date_ymwdhMs(void)
 {
-	struct {
-		std::string expr;
-		long        diff;
-		int         tolerance;
-	} tests[] = {{"3h", 3 * 60 * 60, 1},
-		     {"21d", 21 * 24 * 60 * 60, 3600 + 1},
-		     {"2w", 2 * 7 * 24 * 60 * 60, 3600 + 1},
-
-		     {"2y", 2 * 365 * 24 * 60 * 60, 24 * 3600 + 1},
-		     {"3m", 3 * 30 * 24 * 60 * 60, 3 * 24 * 3600 + 1}};
-
-	for (auto i = 0; i != G_N_ELEMENTS(tests); ++i) {
-		const auto diff = ::time({}) -
-			parse_date_time(tests[i].expr, true).value_or(-1);
+	struct testcase {
+		std::string	expr;
+		int64_t		diff;
+		int		tolerance;
+	};
+
+	std::array<testcase, 7> cases = {{
+		{"7s", 7, 1},
+		{"3M", 3 * 60, 1},
+		{"3h", 3 * 60 * 60, 1},
+		{"21d", 21 * 24 * 60 * 60, 3600 + 1},
+		{"2w", 2 * 7 * 24 * 60 * 60, 3600 + 1},
+		{"2y", 2 * 365 * 24 * 60 * 60, 24 * 3600 + 1},
+		{"3m", 3 * 30 * 24 * 60 * 60, 3 * 24 * 3600 + 1}
+		}};
+
+	for (auto&& tcase: cases) {
+		const auto date = parse_date_time(tcase.expr, true);
+		g_assert_true(date);
+		const auto diff = ::time({}) - *date;
 		if (g_test_verbose())
-			std::cerr << tests[i].expr << ' ' << diff << ' ' << tests[i].diff
-				  << std::endl;
+			std::cerr << tcase.expr << ' ' << diff << ' ' << tcase.diff << '\n';
 
-		g_assert_true(tests[i].diff - diff <= tests[i].tolerance);
+		g_assert_true(tcase.diff - diff <= tcase.tolerance);
 	}
 
-	//g_assert_true(strtol(Mu::date_to_time_t_string("-1y", true).c_str(), NULL, 10) == 0);
+	// note: perhaps it'd be nice if we'd detect this error;
+	// currently we're being rather tolerant
+	// g_assert_false(!!parse_date_time("25q", false));
 }
 
 static void
 test_parse_size()
 {
-	constexpr std::array<std::tuple<const char*, bool, int64_t>, 5> cases = {{
+	constexpr std::array<std::tuple<const char*, bool, int64_t>, 6> cases = {{
 			{ "456", false, 456 },
 			{ "", false, G_MAXINT64 },
 			{ "", true, 0 },
 			{ "2K", false, 2048 },
-			{ "2M", true, 2097152 }
+			{ "2M", true, 2097152 },
+			{ "5G", true, 5368709120 }
 		}};
 	for(auto&& test: cases) {
 		g_assert_cmpint(parse_size(std::get<0>(test), std::get<1>(test))
 				.value_or(-1), ==, std::get<2>(test));
 	}
+
+	g_assert_false(!!parse_size("-1", true));
+	g_assert_false(!!parse_size("scoobydoobydoo", false));
 }
 
 static void