Riddle #5: ABCDE
We have a message processor that can perform some operations on messages. It has some concrete lifecycle, in which the processor itself first needs to be started. After that, a message can be activated and validated.
#include <stdexcept>
#include <gtest/gtest.h>
#include <gmock/gmock.h>
struct Message {};
class MessageProcessor {
public:
void start() {
started = true;
// ...
}
void activate(Message& message) {
activeMessage = &message;
// ...
}
bool validate() {
assert(started);
if (activeMessage == nullptr) {
throw std::runtime_error("A message needs to be activated before validation");
}
// ...
return true;
}
private:
bool started { false };
Message* activeMessage { nullptr };
};
TEST(MessageProcessorTest, startThenValidate_throwsException) {
MessageProcessor processor;
processor.start();
EXPECT_THROW(processor.validate(), std::runtime_error);
}
#include <stdexcept>
#include <gtest/gtest.h>
#include <gmock/gmock.h>
struct Message {};
class MessageProcessor {
public:
void start() {
started = true;
// ...
}
void activate(Message& message) {
activeMessage = &message;
// ...
}
bool validate() {
assert(started);
if (activeMessage == nullptr) {
throw std::runtime_error("A message needs to be activated before validation");
}
// ...
return true;
}
private:
bool started { false };
Message* activeMessage { nullptr };
};
TEST(MessageProcessorTest, validateWithoutAnActiveMessage_throwsException) {
MessageProcessor processor;
processor.start();
EXPECT_THROW(processor.validate(), std::runtime_error);
}
Can you spot a weakness in the test?
Give me a hint
What if the contract of the MessageProcessor was extended with an extra step, e.g. loadRules()? Show me.
The lifecycle now changes from
start() -> [activate() -> validate()]*
to
start() -> [loadRules()]* -> [activate() -> validate()]*.
Give me a hint
Does the startThenValidate_throwsException test need to change?
Reveal the answer
Diagnosis
The test name repeats fairly literally the steps performed by the test scenario, rather than capturing the intention of the test. This flaw makes the maintenance more expensive: the test name is little helpful and the original intent has to be reverse-engineered from the implementation.
Reveal the remedy
Remedy
Rename the test to capture the intention. The test intentionally skips a step in the lifecycle (activate()) and verifies how the implementation deals with it. The name of the test should reflect it, e.g. validateWithoutAnActiveMessage_throwsException, show me. Roughly same effort, but a world of difference in maintenance. Such a name helps answering the question how does this test case need to change in response to adding an extra lifecycle method to the processor. Namely, continuing the example of loadRules(), it quite clear that the constraint of throwing when attempting to validate a message without its activation should throw regardless of if and how many rule sets is loaded.
Same applies to helper functions, actually. It is common to spot code duplication in tests and extract a sequence of operations into a shared routine. It is also a common mistake for the name to simply capture all the steps it takes in its name, e.g. startProcessorAndActivateAndValidateMessage. Such routine should rather reflect the meaning of these operations, just like you would expect from the production code. To continue the example, a better name could be processMessage.
Feedback on this riddle? 3c6120687265663d226d61696c746f3a666565646261636b407375737461696e61626c6563707074657374696e672e636f6d3f7375626a6563743d5375737461696e61626c6520432b2b2054657374696e672c20726964646c65233520666565646261636b223e446f2067657420696e20746f756368213c2f613e