Skip to content

Conversation

jschmidt-icinga
Copy link
Contributor

For now, this just tests the connection handling part, since doing the PKI stuff here felt out of place. I'll probably add that to the ApiListener tests once I get to it.

Closes #10522

Copy link
Member

@yhabteab yhabteab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't review every detail of the implementation yet, but I left some inline comments that caught my eyes while skimming through the changes. Also, I'm not convinced by all the assertion macros you added; for example, the logging related ones just call a non-static member function of a fixture class, but don't require an instance of that class as an argument or the like. This makes it harder to understand where those macros can be used, and where not. I personally don't see a big win in terms of conciseness as opposed to just calling a member function of the fixture class directly. But that's a matter of taste, I guess. Just my 2c!

Comment on lines 115 to 129
// clang-format off
Dictionary::Ptr message = new Dictionary({
{ "jsonrpc", "2.0" },
{ "method", "test::Test" },
{ "params", new Dictionary{{"test", "test"}} }
});
// clang-format on
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've quite a bunch of boilerplate of this kind in the tests below, thus I would add a parameterized function somewhere or maybe in the JsonRpcConnectionFixture class and use that everywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather not. There's small differences to all of these and I personally prefer to avoid the MakeMessage("test", true, 1, true) pattern whenever I can, because it's very hard to read. Now, if C++ had keyworded arguments like Lisp that would be different, and for larger boilerplate segments I'd build a wrapper class with setter methods, but in this case I'm fine with a few lines of slightly different boilerplate per test case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's small differences to all of these and I personally prefer to avoid the MakeMessage("test", true, 1, true) pattern whenever I can, because it's very hard to read.

Seriously? Isn't the point of having functions to avoid repeating yourself? Yes, all the repetitions differ a bit, but nothing that a simple function parameter can't solve. That's how functions work, so I don't see the problem here and using the missing named arguments feature in C++ shouldn't be an argument for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the point of having functions to avoid repeating yourself?

Not at the cost of hurting readability. Also, in this case we already have a function, the constructor of the object. And all that wrapping it in a function would achieve is hiding the actual dictionary parameters behind switches that don't translate 1:1 and you'd need to read the function body to understand. We use functions to not repeat ourselves when an actual, meaningful abstraction is possible, which it isn't here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not at the cost of hurting readability.

Is this diff so unreadable to you?

Expand Me
diff --git a/test/remote-jsonrpcconnection.cpp b/test/remote-jsonrpcconnection.cpp
index bc7cc8a92..69622e173 100644
--- a/test/remote-jsonrpcconnection.cpp
+++ b/test/remote-jsonrpcconnection.cpp
@@ -69,6 +69,21 @@ public:

 	void JoinWaitgroup(const JsonRpcConnection::Ptr& conn) { m_Connections[conn]->Join(); }

+	static Dictionary::Ptr MakeMessage(const Dictionary::Ptr& params = new Dictionary{{"test", "test"}}, double ts = 0.0, String id = "")
+	{
+		Dictionary::Ptr message = new Dictionary{{"jsonrpc", "2.0"}, {"method", "test::Test"}};
+		if (params != nullptr) {
+			message->Set("params", params);
+		}
+		if (ts != 0.0) {
+			message->Set("ts", ts);
+		}
+		if (id != Empty) {
+			message->Set("id", std::move(id));
+		}
+		return message;
+	}
+
 private:
 	std::map<JsonRpcConnection::Ptr, StoppableWaitGroup::Ptr> m_Connections;
 };
@@ -120,14 +135,6 @@ BOOST_AUTO_TEST_CASE(sendmessage)
 {
 	auto [aToB, bToA] = ConnectEndpointPair("a", "b");

-	// clang-format off
-	Dictionary::Ptr message = new Dictionary({
-		{ "jsonrpc", "2.0" },
-		{ "method", "test::Test" },
-		{ "params", new Dictionary{{"test", "test"}} }
-	});
-	// clang-format on
-
 	std::promise<TestApiHandler::Message> msgPromise;
 	auto msgFuture = msgPromise.get_future();
 	TestApiHandler::RegisterTestFn("test", [&](TestApiHandler::Message msg) {
@@ -135,6 +142,8 @@ BOOST_AUTO_TEST_CASE(sendmessage)
 		return Empty;
 	});

+	auto message = MakeMessage();
+
 	// Test sending a regular message from a->b
 	aToB->SendMessage(message);

@@ -154,14 +163,6 @@ BOOST_AUTO_TEST_CASE(sendmessageraw)
 {
 	auto [aToB, bToA] = ConnectEndpointPair("a", "b");

-	// clang-format off
-	auto message = JsonEncode(new Dictionary{{
-		{ "jsonrpc", "2.0" },
-		{ "method", "test::Test" },
-		{ "params", new Dictionary{{"test", "test"}} }
-	}});
-	// clang-format on
-
 	std::promise<TestApiHandler::Message> msgPromise;
 	auto msgFuture = msgPromise.get_future();
 	TestApiHandler::RegisterTestFn("test", [&](TestApiHandler::Message msg) {
@@ -169,6 +170,8 @@ BOOST_AUTO_TEST_CASE(sendmessageraw)
 		return Empty;
 	});

+	auto message = JsonEncode(MakeMessage());
+
 	// Test sending a raw message from b->a
 	bToA->SendRawMessage(message);

@@ -190,14 +193,7 @@ BOOST_AUTO_TEST_CASE(old_message)
 	BOOST_REQUIRE_EQUAL(conn->GetEndpoint()->GetRemoteLogPosition(), Timestamp{});

 	auto ts = Utility::GetTime();
-	// clang-format off
-	Dictionary::Ptr message = new Dictionary{{
-		{ "jsonrpc", "2.0" },
-		{ "method", "test::Test" },
-		{ "ts", ts },
-		{ "params", new Dictionary{{"test", "test"}} }
-	}};
-	// clang-format on
+	auto message = MakeMessage(new Dictionary{{"test", "test"}}, ts);

 	BOOST_REQUIRE_NO_THROW(JsonRpc::SendMessage(client, message));
 	client->flush();
@@ -222,18 +218,9 @@ BOOST_AUTO_TEST_CASE(messageresult)
 {
 	auto conn = ConnectEndpoint(server, "test");

-	// clang-format off
-	Dictionary::Ptr message = new Dictionary({
-		{ "jsonrpc", "2.0" },
-		{ "method", "test::Test" },
-		{ "id", "test1" },
-		{ "params", new Dictionary{{"test", "test"}} }
-	});
-	// clang-format on
-
 	TestApiHandler::RegisterTestFn("test", [&](const TestApiHandler::Message&) { return "Success"; });

-	BOOST_REQUIRE_NO_THROW(JsonRpc::SendMessage(client, message));
+	BOOST_REQUIRE_NO_THROW(JsonRpc::SendMessage(client, MakeMessage(new Dictionary{{"test", "test"}}, 0.0, "test1")));
 	client->flush();

 	auto msg = JsonRpc::DecodeMessage(JsonRpc::ReadMessage(client));
@@ -249,21 +236,13 @@ BOOST_AUTO_TEST_CASE(messageresult_noparams)
 {
 	auto conn = ConnectEndpoint(server, "test");

-	// clang-format off
-	Dictionary::Ptr message = new Dictionary({
-		{ "jsonrpc", "2.0" },
-		{ "method", "test::Test" },
-		{ "id", "test1" }
-	});
-	// clang-format on
-
 	std::atomic_bool handlerCalled = false;
 	TestApiHandler::RegisterTestFn("test", [&](const TestApiHandler::Message&) {
 		handlerCalled = true;
 		return Empty;
 	});

-	BOOST_REQUIRE_NO_THROW(JsonRpc::SendMessage(client, message));
+	BOOST_REQUIRE_NO_THROW(JsonRpc::SendMessage(client, MakeMessage(nullptr, 0.0, "test1")));
 	client->flush();

 	// Ensure the message has been processed.
@@ -285,13 +264,8 @@ BOOST_AUTO_TEST_CASE(call_to_nonexistant_function)
 {
 	auto conn = ConnectEndpoint(server, "test");

-	// clang-format off
-	Dictionary::Ptr message = new Dictionary({
-		{ "jsonrpc", "2.0" },
-		{ "method", "test::FooBar" },
-		{ "params", new Dictionary{} }
-	});
-	// clang-format on
+	auto message = MakeMessage({});
+	message->Set("method", "test::FooBar");

 	BOOST_REQUIRE_NO_THROW(JsonRpc::SendMessage(client, message));
 	client->flush();
@@ -370,14 +344,7 @@ BOOST_AUTO_TEST_CASE(wg_join_during_send, *boost::unit_test::disabled{})
 		return "Response";
 	});

-	// clang-format off
-	Dictionary::Ptr message = new Dictionary({
-		{ "jsonrpc", "2.0" },
-		{ "method", "test::Test" },
-		{ "id", "wgTest1" },
-		{ "params", new Dictionary{{"test", "test"}} }
-	});
-	// clang-format on
+	auto message = MakeMessage(new Dictionary{{"test", "test"}}, 0.0, "wgTest1");

 	BOOST_REQUIRE_NO_THROW(JsonRpc::SendMessage(client, message));
 	client->flush();
@@ -410,20 +377,11 @@ BOOST_AUTO_TEST_CASE(send_after_wg_join)
 		return Empty;
 	});

-	// clang-format off
-	Dictionary::Ptr message = new Dictionary({
-		{ "jsonrpc", "2.0" },
-		{ "method", "test::Test" },
-		{ "id", "wgTest1" },
-		{ "params", new Dictionary{{"test", "test"}} }
-	});
-	// clang-format on
-
 	// Join the wait-group even before sending the message.
 	JoinWaitgroup(conn);

 	// The message should be received without error, but it should not be processed.
-	BOOST_REQUIRE_NO_THROW(JsonRpc::SendMessage(client, message));
+	BOOST_REQUIRE_NO_THROW(JsonRpc::SendMessage(client, MakeMessage(new Dictionary{{"test", "test"}}, 0.0, "wgTest1")));
 	client->flush();

 	// Wait until the message has been processed.

It even eliminates the need for the super strange clang-format off/on comments everywhere!

Copy link
Contributor Author

@jschmidt-icinga jschmidt-icinga Oct 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this diff so unreadable to you?

Yes!

Without looking at MakeMessage() first, the first few usages of it are fine and I was beginning to think "maybe this isn't so bad", but then I came across MakeMessage(nullptr, 0.0, "test1")) and the other ones, which exactly highlight the issue. How is someone who hasn't read what MakeMessage() does supposed to get anything from that? Ok, then they scroll up, read what it does, and piece together how it prefills the message based on those arguments and their default values. Then they shake their head about how the 0.0 was passed as a timestamp, but 0.0 is a special value that gets ignored because you needed to get past that slot in the argument list to get to id string. Then they scroll one invocation down and now we rewrite the "method" manually on the default message. Is that changing a default or just an attribute MakeMessage() doesn't cover? so they need to look at the function again to see that the default is "test::Test" and we're changing it to "test::FooBar"...

So yes, while just writing out the entire dictionary is a bit verbose, it's also giving you an instant mental image of what the message we're testing with looks like, as opposed to your version, where you have to piece it together in your head every single time.

I get your point that you shouldn't repeat yourself, which is why I did use factory functions for constructing the connection objects for example. In this case I disagree. It's not any more readable and it doesn't make it easier to maintain since it's much more likely you'll add an additional test with additional differences to the default, than an additional default argument to all the tests.

It even eliminates the need for the super strange clang-format off/on comments everywhere!

I agree they're ugly to look at, but I can remove them before merge. And there's a solution I've described in #10520 that may make them unnecessary in the future once I/we have clang-21.

@jschmidt-icinga
Copy link
Contributor Author

jschmidt-icinga commented Sep 26, 2025

Also, I'm not convinced by all the assertion macros you added; for example, the logging related ones just call a non-static member function of a fixture class, but don't require an instance of that class as an argument or the like.

I'm happy to take suggestions on the naming and interface of the timed asserts. But they're definitely something that's useful (at least to me), because I kept running into this pattern and that would actually be boilerplate if we had to repeat it for every test-suite/case.

The logging related ones are more for visual consistency (because to be useful you'd have to always wrap the methods in a BOOST_(TEST|CHECK|REQUIRE) anyways) than anything else. I don't think implicitly requiring the fixture in whose header they're defined in is a problem. Worst case they'd print a pretty clear compiler error and the user would just add the fixture to their test case. I can even add an #error with a special message if that's your fear. But I'm open to other suggestions or ultimately removing them entirely.

If the logger is started with `Activate()` before `SetActive()`, it won't
log anything, as the logger updates the "min severity" value of loggers
only when starting them, and if they're not active at that point, they
will just be ignored, so the min severity remains at info.
@jschmidt-icinga jschmidt-icinga force-pushed the json-rpc-unit-testing branch 4 times, most recently from b18956f to 3a584a0 Compare September 29, 2025 13:23
Copy link
Member

@yhabteab yhabteab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to take suggestions on the naming and interface of the timed asserts. But they're definitely something that's useful (at least to me), because I kept running into this pattern and that would actually be boilerplate if we had to repeat it for every test-suite/case.

I just failed to see the usefulness of the *_DONE_* macros. They're basically the same as the other *_WITHIN_* macros, but they just differ in the result message they produce. Can't you just use the later ones everywhere and be done with it? If the difference is indeed required, then I'm just confused.

if (anonymousTimeout > 1s) {
msg << anonymousTimeout.count() / 1000 << " seconds.";
} else {
msg << anonymousTimeout.count() << " milliseconds";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be consistent with its sibling ;).

Suggested change
msg << anonymousTimeout.count() << " milliseconds";
msg << anonymousTimeout.count() << " milliseconds.";

Also, if you really need the extra scoping for the log, I would also move the remote variable into that new scope.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, if you really need the extra scoping for the log

Without it, the message would get printed after the message in Disconnect().

if (m_LivenessTimeout > 1s) {
msg << m_LivenessTimeout.count() / 1000 << " seconds.";
} else {
msg << m_LivenessTimeout.count() << " milliseconds";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here!

Comment on lines 451 to 452

{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant empty line after the oddly wrapped multiline if condition.

}
if (fn()) {
boost::test_tools::assertion_result retVal{false};
retVal.message() << "Condtion (" << cond << ") was true before " << falseUntil.count() << "s";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
retVal.message() << "Condtion (" << cond << ") was true before " << falseUntil.count() << "s";
retVal.message() << "Condition (" << cond << ") was true before " << falseUntil.count() << "s";

Comment on lines 63 to 64
static boost::test_tools::assertion_result AssertWithTimeout(const std::function<bool()>& fn,
const std::chrono::duration<double>& timeout, std::string_view cond)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
static boost::test_tools::assertion_result AssertWithTimeout(const std::function<bool()>& fn,
const std::chrono::duration<double>& timeout, std::string_view cond)
static boost::test_tools::assertion_result AssertWithTimeout(
const std::function<bool()>& fn,
const std::chrono::duration<double>& timeout,
std::string_view cond
)

Your clang-format should also be happy with this I think, since you've wrapped the parameters just like this in the remote-jsonrpcconnection file. There are other function within this file, so please adjust them accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, in fact that's what it did now after I ran it again with the changes I recently made in #10520.

Comment on lines 115 to 129
// clang-format off
Dictionary::Ptr message = new Dictionary({
{ "jsonrpc", "2.0" },
{ "method", "test::Test" },
{ "params", new Dictionary{{"test", "test"}} }
});
// clang-format on
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not at the cost of hurting readability.

Is this diff so unreadable to you?

Expand Me
diff --git a/test/remote-jsonrpcconnection.cpp b/test/remote-jsonrpcconnection.cpp
index bc7cc8a92..69622e173 100644
--- a/test/remote-jsonrpcconnection.cpp
+++ b/test/remote-jsonrpcconnection.cpp
@@ -69,6 +69,21 @@ public:

 	void JoinWaitgroup(const JsonRpcConnection::Ptr& conn) { m_Connections[conn]->Join(); }

+	static Dictionary::Ptr MakeMessage(const Dictionary::Ptr& params = new Dictionary{{"test", "test"}}, double ts = 0.0, String id = "")
+	{
+		Dictionary::Ptr message = new Dictionary{{"jsonrpc", "2.0"}, {"method", "test::Test"}};
+		if (params != nullptr) {
+			message->Set("params", params);
+		}
+		if (ts != 0.0) {
+			message->Set("ts", ts);
+		}
+		if (id != Empty) {
+			message->Set("id", std::move(id));
+		}
+		return message;
+	}
+
 private:
 	std::map<JsonRpcConnection::Ptr, StoppableWaitGroup::Ptr> m_Connections;
 };
@@ -120,14 +135,6 @@ BOOST_AUTO_TEST_CASE(sendmessage)
 {
 	auto [aToB, bToA] = ConnectEndpointPair("a", "b");

-	// clang-format off
-	Dictionary::Ptr message = new Dictionary({
-		{ "jsonrpc", "2.0" },
-		{ "method", "test::Test" },
-		{ "params", new Dictionary{{"test", "test"}} }
-	});
-	// clang-format on
-
 	std::promise<TestApiHandler::Message> msgPromise;
 	auto msgFuture = msgPromise.get_future();
 	TestApiHandler::RegisterTestFn("test", [&](TestApiHandler::Message msg) {
@@ -135,6 +142,8 @@ BOOST_AUTO_TEST_CASE(sendmessage)
 		return Empty;
 	});

+	auto message = MakeMessage();
+
 	// Test sending a regular message from a->b
 	aToB->SendMessage(message);

@@ -154,14 +163,6 @@ BOOST_AUTO_TEST_CASE(sendmessageraw)
 {
 	auto [aToB, bToA] = ConnectEndpointPair("a", "b");

-	// clang-format off
-	auto message = JsonEncode(new Dictionary{{
-		{ "jsonrpc", "2.0" },
-		{ "method", "test::Test" },
-		{ "params", new Dictionary{{"test", "test"}} }
-	}});
-	// clang-format on
-
 	std::promise<TestApiHandler::Message> msgPromise;
 	auto msgFuture = msgPromise.get_future();
 	TestApiHandler::RegisterTestFn("test", [&](TestApiHandler::Message msg) {
@@ -169,6 +170,8 @@ BOOST_AUTO_TEST_CASE(sendmessageraw)
 		return Empty;
 	});

+	auto message = JsonEncode(MakeMessage());
+
 	// Test sending a raw message from b->a
 	bToA->SendRawMessage(message);

@@ -190,14 +193,7 @@ BOOST_AUTO_TEST_CASE(old_message)
 	BOOST_REQUIRE_EQUAL(conn->GetEndpoint()->GetRemoteLogPosition(), Timestamp{});

 	auto ts = Utility::GetTime();
-	// clang-format off
-	Dictionary::Ptr message = new Dictionary{{
-		{ "jsonrpc", "2.0" },
-		{ "method", "test::Test" },
-		{ "ts", ts },
-		{ "params", new Dictionary{{"test", "test"}} }
-	}};
-	// clang-format on
+	auto message = MakeMessage(new Dictionary{{"test", "test"}}, ts);

 	BOOST_REQUIRE_NO_THROW(JsonRpc::SendMessage(client, message));
 	client->flush();
@@ -222,18 +218,9 @@ BOOST_AUTO_TEST_CASE(messageresult)
 {
 	auto conn = ConnectEndpoint(server, "test");

-	// clang-format off
-	Dictionary::Ptr message = new Dictionary({
-		{ "jsonrpc", "2.0" },
-		{ "method", "test::Test" },
-		{ "id", "test1" },
-		{ "params", new Dictionary{{"test", "test"}} }
-	});
-	// clang-format on
-
 	TestApiHandler::RegisterTestFn("test", [&](const TestApiHandler::Message&) { return "Success"; });

-	BOOST_REQUIRE_NO_THROW(JsonRpc::SendMessage(client, message));
+	BOOST_REQUIRE_NO_THROW(JsonRpc::SendMessage(client, MakeMessage(new Dictionary{{"test", "test"}}, 0.0, "test1")));
 	client->flush();

 	auto msg = JsonRpc::DecodeMessage(JsonRpc::ReadMessage(client));
@@ -249,21 +236,13 @@ BOOST_AUTO_TEST_CASE(messageresult_noparams)
 {
 	auto conn = ConnectEndpoint(server, "test");

-	// clang-format off
-	Dictionary::Ptr message = new Dictionary({
-		{ "jsonrpc", "2.0" },
-		{ "method", "test::Test" },
-		{ "id", "test1" }
-	});
-	// clang-format on
-
 	std::atomic_bool handlerCalled = false;
 	TestApiHandler::RegisterTestFn("test", [&](const TestApiHandler::Message&) {
 		handlerCalled = true;
 		return Empty;
 	});

-	BOOST_REQUIRE_NO_THROW(JsonRpc::SendMessage(client, message));
+	BOOST_REQUIRE_NO_THROW(JsonRpc::SendMessage(client, MakeMessage(nullptr, 0.0, "test1")));
 	client->flush();

 	// Ensure the message has been processed.
@@ -285,13 +264,8 @@ BOOST_AUTO_TEST_CASE(call_to_nonexistant_function)
 {
 	auto conn = ConnectEndpoint(server, "test");

-	// clang-format off
-	Dictionary::Ptr message = new Dictionary({
-		{ "jsonrpc", "2.0" },
-		{ "method", "test::FooBar" },
-		{ "params", new Dictionary{} }
-	});
-	// clang-format on
+	auto message = MakeMessage({});
+	message->Set("method", "test::FooBar");

 	BOOST_REQUIRE_NO_THROW(JsonRpc::SendMessage(client, message));
 	client->flush();
@@ -370,14 +344,7 @@ BOOST_AUTO_TEST_CASE(wg_join_during_send, *boost::unit_test::disabled{})
 		return "Response";
 	});

-	// clang-format off
-	Dictionary::Ptr message = new Dictionary({
-		{ "jsonrpc", "2.0" },
-		{ "method", "test::Test" },
-		{ "id", "wgTest1" },
-		{ "params", new Dictionary{{"test", "test"}} }
-	});
-	// clang-format on
+	auto message = MakeMessage(new Dictionary{{"test", "test"}}, 0.0, "wgTest1");

 	BOOST_REQUIRE_NO_THROW(JsonRpc::SendMessage(client, message));
 	client->flush();
@@ -410,20 +377,11 @@ BOOST_AUTO_TEST_CASE(send_after_wg_join)
 		return Empty;
 	});

-	// clang-format off
-	Dictionary::Ptr message = new Dictionary({
-		{ "jsonrpc", "2.0" },
-		{ "method", "test::Test" },
-		{ "id", "wgTest1" },
-		{ "params", new Dictionary{{"test", "test"}} }
-	});
-	// clang-format on
-
 	// Join the wait-group even before sending the message.
 	JoinWaitgroup(conn);

 	// The message should be received without error, but it should not be processed.
-	BOOST_REQUIRE_NO_THROW(JsonRpc::SendMessage(client, message));
+	BOOST_REQUIRE_NO_THROW(JsonRpc::SendMessage(client, MakeMessage(new Dictionary{{"test", "test"}}, 0.0, "wgTest1")));
 	client->flush();

 	// Wait until the message has been processed.

It even eliminates the need for the super strange clang-format off/on comments everywhere!

Comment on lines 165 to 164
std::promise<TestApiHandler::Message> msgPromise;
auto msgFuture = msgPromise.get_future();
TestApiHandler::RegisterTestFn("test", [&](TestApiHandler::Message msg) {
msgPromise.set_value(std::move(msg));
return Empty;
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that you're not registering this handler for all the test cases, but I would still register this within the fixture class itself and make the promise available to all the test cases. You could then either always verify the result or ignore it in the test cases that don't need to.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't immediately see a good way to do this, but it would be nice. I will think about this a bit more.

BOOST_REQUIRE_THROW(aToB->SendMessage(message), std::runtime_error);
}

BOOST_AUTO_TEST_CASE(sendmessageraw)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
BOOST_AUTO_TEST_CASE(sendmessageraw)
BOOST_AUTO_TEST_CASE(send_raw_message)

Same also for the other sendmessage and messageresult test cases. Sometimes you're separating the test names with _ and sometimes not, thus I would use consistent names and separate the words with _.

@jschmidt-icinga
Copy link
Contributor Author

I just failed to see the usefulness of the *_DONE_* macros. They're basically the same as the other *_WITHIN_* macros, but they just differ in the result message they produce. Can't you just use the later ones everywhere and be done with it? If the difference is indeed required, then I'm just confused.

Again, the naming is debatable, I was just out of ideas and it could be improved, but the two groups do completely different things.

The DONE ones wait for a blocking/long-running operation to complete and then check if it has finished within the asserted boundaries, the other ones sample a condition over and over to get the exact time when it changes from false to true (or vice versa) within the asserted boundaries.

Also add a Clear() function to clear existing log content.
<level>_WITHIN asserts if the given condition becomes true
within the given timeout.

<level>_EDGE_WITHIN aserts if the given condition becomes
true no sooner than time 1 but not after time 2.

<level>_DONE_WITHIN asserts the execution time of the given
expression is under the given duration.

<level>_DONE_BETWEEN asserts the execution time of the given
expression is between the given durations.
@jschmidt-icinga jschmidt-icinga marked this pull request as draft October 2, 2025 07:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add unit-testing for JsonRpcConnection and JsonRpc utility class
2 participants