Add unit tests for JsonRpcConnection connection handling
#10568
Conversation
MinSeverity only gets updated if the logger is already active, so we have to set severity after the logger has been activated. Also add a Clear() method to clear existing log messages and assert macros to assert both the existence and absence of a pattern in the log.
<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
force-pushed
the
json-rpc-unit-testing
branch
from
0a1079c to
f2876d2
Compare
There was a problem hiding this comment.
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!
|
|
||
| void Clear() | ||
| { | ||
| std::unique_lock lock(m_Mutex); |
There was a problem hiding this comment.
Maybe use std::lock_guard or std::scoped_lock here isntead?
| testLogger->SetSeverity(testLogger->SeverityToString(LogDebug)); | ||
| testLogger->Activate(true); | ||
| testLogger->SetActive(true); | ||
| testLogger->SetSeverity(TestLogger::SeverityToString(LogDebug)); |
There was a problem hiding this comment.
I think you should call SetActive before Activate instead of this change.
|
|
||
| using namespace icinga; | ||
|
|
||
| class JsonRpcConnectionFixture : public TlsStreamFixture, public TestLoggerFixture, IcingaApplicationFixture |
There was a problem hiding this comment.
You don't need to extend IcingaApplicationFixture as it's a globally registered fixture.
| // clang-format off | ||
| Dictionary::Ptr message = new Dictionary({ | ||
| { "jsonrpc", "2.0" }, | ||
| { "method", "test::Test" }, | ||
| { "params", new Dictionary{{"test", "test"}} } | ||
| }); | ||
| // clang-format on |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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. |
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