Uncle Bob defines abstraction as “amplification of the essential and elimination of the irrelevant.” Abstraction is as important in your tests as it is in object-oriented design. Since you want to be able to read your tests as documentation, they must cut to the chase, declaring their intent as clearly and simply as possible.
From a simplistic stance, you can increase abstraction in your tests by making them more cohesive (One Assert per Test), focusing on better naming (for the test itself as well as the code within), and abstracting away the rest of the cruft (using perhaps fixture helper functions or SetUp).
We’ll work through identifying nine different test smells and cleaning up test code accordingly.
Let’s start with one of the tests for a LineReader class. The test names don’t tell us much about how LineReader works. We hope that cleaning up the tests will help.
c7/3/linereader/LineReaderTest.cpp | |
| TEST(LineReaderTest, OneLine) { |
* | const int fd = TemporaryFile(); |
* | write(fd, "a", 1); |
* | lseek(fd, 0, SEEK_SET); |
| LineReader reader(fd); |
| |
| const char *line; |
| unsigned len; |
| ASSERT_TRUE(reader.GetNextLine(&line, &len)); |
| ASSERT_EQ(len, (unsigned)1); |
| ASSERT_EQ(line[0], 'a'); |
| ASSERT_EQ(line[1], 0); |
| reader.PopLine(len); |
| |
| ASSERT_FALSE(reader.GetNextLine(&line, &len)); |
| |
| close(fd); |
| } |
The highlighted three lines appear to create a temporary file, populate it with a single character ("a"), and reset the file pointer to its beginning. This bloated construction requires the reader to wade through unnecessary test setup details. We can replace the bloat with a single-line abstraction.
c7/4/linereader/LineReaderTest.cpp | |
| TEST(LineReaderTest, OneLine) { |
* | const int fd = WriteTemporaryFile("a"); |
| LineReader reader(fd); |
| |
| const char *line; |
| unsigned len; |
| ASSERT_TRUE(reader.GetNextLine(&line, &len)); |
| ASSERT_EQ(len, (unsigned)1); |
| ASSERT_EQ(line[0], 'a'); |
| ASSERT_EQ(line[1], 0); |
| reader.PopLine(len); |
| |
| ASSERT_FALSE(reader.GetNextLine(&line, &len)); |
| |
| close(fd); |
| } |
The test is a couple lines shorter and hides the implementation details required to create a file with a small amount of data. We won’t usually care, and in the rare case we do, we can simply navigate to see what WriteTemporaryFile really does.
The test first creates a temporary file. The original programmer, being a good coding citizen, made sure the file was closed at the end of the test.
c7/5/linereader/LineReaderTest.cpp | |
| TEST(LineReaderTest, OneLine) { |
* | const int fd = WriteTemporaryFile("a"); |
| LineReader reader(fd); |
| |
| const char *line; |
| unsigned len; |
| ASSERT_TRUE(reader.GetNextLine(&line, &len)); |
| ASSERT_EQ(len, (unsigned)1); |
| ASSERT_EQ(line[0], 'a'); |
| ASSERT_EQ(line[1], 0); |
| reader.PopLine(len); |
| |
| ASSERT_FALSE(reader.GetNextLine(&line, &len)); |
| |
* | close(fd); |
| } |
The call to close is clutter, another detail that distracts from understanding the test. We can take advantage of the TearDown hook to ensure the file gets closed. We can also eliminate the type information from the variable declaration for fd (file descriptor, presumably), moving it too into the fixture.
c7/6/linereader/LineReaderTest.cpp | |
| class LineReaderTest: public testing::Test { |
| public: |
* | int fd; |
| void TearDown() { |
* | close(fd); |
| } |
| }; |
| |
| TEST_F(LineReaderTest, OneLine) { |
* | fd = WriteTemporaryFile("a"); |
| LineReader reader(fd); |
| |
| const char *line; |
| unsigned len; |
| ASSERT_TRUE(reader.GetNextLine(&line, &len)); |
| ASSERT_EQ(len, (unsigned)1); |
| ASSERT_EQ(line[0], 'a'); |
| ASSERT_EQ(line[1], 0); |
| reader.PopLine(len); |
| |
| ASSERT_FALSE(reader.GetNextLine(&line, &len)); |
| } |
The temporary now seems of little use. We collapse the creation of the LineReader into a single line.
c7/7/linereader/LineReaderTest.cpp | |
| TEST_F(LineReaderTest, OneLine) { |
* | LineReader reader(WriteTemporaryFile("a")); |
| |
| const char *line; |
| unsigned len; |
| ASSERT_TRUE(reader.GetNextLine(&line, &len)); |
| ASSERT_EQ(len, (unsigned)1); |
| ASSERT_EQ(line[0], 'a'); |
| ASSERT_EQ(line[1], 0); |
| reader.PopLine(len); |
| |
| ASSERT_FALSE(reader.GetNextLine(&line, &len)); |
| } |
Hmm...there’s a slight problem. We’re no longer closing the temporary file in TearDown (we’re instead attempting to close using the uninitialized file descriptor fd). We choose to improve the design of the LineReader by supporting RAII and closing the file itself on destruction. (See http://en.wikipedia.org/wiki/Resource_Acquisition_Is_Initialization for further information about the RAII idiom.) Code details left to the reader! (Or, you can look at the supplied source.)
The test still contains details we don’t need to see most of the time—two lines declare the line and len variables. They’re also replicated throughout several other LineReader tests. Let’s get rid of the clutter and duplication.
c7/8/linereader/LineReaderTest.cpp | |
| class LineReaderTest: public testing::Test { |
| public: |
| int fd; |
* | const char *line; |
* | unsigned len; |
| }; |
| |
| TEST_F(LineReaderTest, OneLine) { |
| LineReader reader(WriteTemporaryFile("a")); |
| |
| ASSERT_TRUE(reader.GetNextLine(&line, &len)); |
| ASSERT_EQ(len, (unsigned)1); |
| ASSERT_EQ(line[0], 'a'); |
| ASSERT_EQ(line[1], 0); |
| reader.PopLine(len); |
| |
| ASSERT_FALSE(reader.GetNextLine(&line, &len)); |
| } |
Many developers too often overlook the opportunity to create simple abstractions. Despite seeming like extra effort for questionable gain, extracting small chunks of code to helper methods and classes is win-win-win. First, it amplifies your code’s expressiveness, potentially eliminating the need for an explanatory comment. Second, it promotes reuse of those small chunks of code, which in turn can help you eliminate sizeable amounts of duplicative code. Third, it makes subsequent tests easier to write.
Our test currently requires three lines of code to verify the results of getting the next line from the reader.
c7/9/linereader/LineReaderTest.cpp | |
| TEST_F(LineReaderTest, OneLine) { |
| LineReader reader(WriteTemporaryFile("a")); |
| |
| ASSERT_TRUE(reader.GetNextLine(&line, &len)); |
* | ASSERT_EQ(len, (unsigned)1); |
* | ASSERT_EQ(line[0], 'a'); |
* | ASSERT_EQ(line[1], 0); |
| reader.PopLine(len); |
| |
| ASSERT_FALSE(reader.GetNextLine(&line, &len)); |
| } |
A helper function reduces the three assertions to a single, more abstract declaration. (You could also introduce a custom assertion on the matcher if your unit testing tool supports it.)
c7/10/linereader/LineReaderTest.cpp | |
| void ASSERT_EQ_WITH_LENGTH( |
| const char* expected, const char* actual, unsigned length) { |
| ASSERT_EQ(length, strlen(actual)); |
| ASSERT_STREQ(expected, actual); |
| } |
| |
| TEST_F(LineReaderTest, OneLine) { |
| LineReader reader(WriteTemporaryFile("a")); |
| |
| ASSERT_TRUE(reader.GetNextLine(&line, &len)); |
* | ASSERT_EQ_WITH_LENGTH("a", line, len); |
| reader.PopLine(len); |
| |
| ASSERT_FALSE(reader.GetNextLine(&line, &len)); |
| } |
We’ve whittled down the test to a couple statements and three assertions. We take advantage of One Assert per Test, creating three tests, each with a clear name that summarizes its one goal.
c7/11/linereader/LineReaderTest.cpp | |
| TEST_F(GetNextLinefromLineReader, UpdatesLineAndLenOnRead) { |
| LineReader reader(WriteTemporaryFile("a")); |
| reader.GetNextLine(&line, &len); |
| ASSERT_EQ_WITH_LENGTH("a", line, len); |
| } |
| |
| TEST_F(GetNextLinefromLineReader, AnswersTrueWhenLineAvailable) { |
| LineReader reader(WriteTemporaryFile("a")); |
| bool wasLineRead = reader.GetNextLine(&line, &len); |
| ASSERT_TRUE(wasLineRead); |
| } |
| |
| TEST_F(GetNextLinefromLineReader, AnswersFalseWhenAtEOF) { |
| LineReader reader(WriteTemporaryFile("a")); |
| reader.GetNextLine(&line, &len); |
| reader.PopLine(len); |
| bool wasLineRead = reader.GetNextLine(&line, &len); |
| ASSERT_FALSE(wasLineRead); |
| } |
Reviewing the new tests and their names, it should be apparent that we are missing tests. The behavior of PopLine isn’t adequately explained without some analysis and intuition, and we wonder what happens when GetNextLine gets called twice in succession. We can add the missing tests AdvancesToNextLineAfterPop and RepeatedlyReturnsCurrentRecord (an exercise again left to the reader).
Continually reviewing your whole set of test names will help you find the holes in your specifications.
Data used in a test should help tell its story. Embedded literals are otherwise a distraction or, worse, a puzzle. If a function call requires arguments but they have no relevance to the test at hand, you can often get away with passing 0 or similar values indicating emptiness (such as "" for string literals). To a reader, these literals should suggest “nothing interesting to see here.” (If zero is a meaningful value, introduce a constant to help explain why.)
Sometimes you’ll have no choice but to pass a nonzero or nonempty value. A simple constant can quickly tell a reader all they need to know. In the test AnswersTrueWhenLineAvailable, we don’t care about the file contents, so we replace the literal "a" passed to WriteTemporaryFile with an intention-revealing name.
c7/12/linereader/LineReaderTest.cpp | |
| TEST_F(GetNextLinefromLineReader, AnswersTrueWhenLineAvailable) { |
* | LineReader reader(WriteTemporaryFile(ArbitraryText)); |
| |
| bool wasLineRead = reader.GetNextLine(&line, &len); |
| |
| ASSERT_TRUE(wasLineRead); |
| } |
After a few passes through an overblown test, we’ve ended up with three concise tests, each a handful of lines or less. We clearly understand what each test does in a matter of seconds. And we now better understand the behaviors LineReader supports.
To sniff out a few more smells, we’ll take a look at some other not-so-clean tests—the LineReader tests are good enough for now.
Some elements don’t belong in tests at all. This section discusses a few code constructs you can remove outright from your tests.
Seg faults are no fun. If you dereference a null pointer, you’re going have a bad time as the rest of your test run crashes. Coding defensively is an understandable reaction.
c7/12/libraryTest/PersistenceTest.cpp | |
| TEST_P(PersistenceTest, AddedItemCanBeRetrievedById) |
| { |
| persister->Add(*objectWithId1); |
| |
| auto found = persister->Get("1"); |
| |
* | ASSERT_THAT(found, NotNull()); |
| ASSERT_THAT(*found, Eq(*objectWithId1)); |
| } |
But remember, you are designing tests either to drive happy-path behavior or to generate and expect failure. For the persister code, a test already exists that demonstrates when the Get call can return null.
c7/12/libraryTest/PersistenceTest.cpp | |
| TEST_P(PersistenceTest, ReturnsNullPointerWhenItemNotFound) |
| { |
| ASSERT_THAT(persister->Get("no id there"), IsNull()); |
| } |
AddedItemCanBeRetrievedById is a happy-path test. Once we get it working, it should always work...barring a defect that someone codes in the future or a failure to allocate memory. As such, the null check (ASSERT_THAT(found, NotNull())) will unlikely ever fail for this happy-path test.
We’ll eliminate the assert not null statement. (We should really avoid raw pointers at all to make this a nonissue.) It adds no documentation value to the test and acts only as a safeguard. The downside is if we remove the guard clause and the pointer ends up null, we get a seg fault. We’re willing to take that trade-off—in the worst, very unlikely case, we acknowledge the seg fault, add a null check, and rerun the test to verify our assumption. If our tests are fast, it’s not a big deal.
If you’re reluctant to let an occasional test run crash with a seg fault, some unit test frameworks provide alternate solutions that don’t require an entire additional line to verify each pointer. Google Test, for example, supplies the Pointee matcher.
c7/13/libraryTest/PersistenceTest.cpp | |
| TEST_P(PersistenceTest, AddedItemCanBeRetrievedById) |
| { |
| persister->Add(*objectWithId1); |
| |
| auto found = persister->Get("1"); |
| |
* | ASSERT_THAT(found, Pointee(*objectWithId1)); |
| } |
When doing TDD, it’s conceivable you coded an assertion for a null check as an incremental step. It’s OK to take that small step. However, once you think a test is complete, take a look back and eliminate any elements that don’t add documentation value. Usually, not null assertions fall into this category.
If the code you’re test-driving can generate an exception, you need to test-drive a case that documents how that happens. In the library system, the function to add a branch can throw an exception in at least one case.
c7/13/libraryTest/BranchServiceTest.cpp | |
| TEST_F(BranchServiceTest, AddThrowsWhenNameNotUnique) |
| { |
| service.Add("samename", ""); |
| |
| ASSERT_THROW(service.Add("samename", ""), DuplicateBranchNameException); |
| } |
Since the add function can throw an exception, some programmers want to protect themselves in other tests that call add.
c7/13/libraryTest/BranchServiceTest.cpp | |
| TEST_F(BranchServiceTest, AddGeneratesUniqueId) |
| { |
| // Don't do this! |
| // Eliminate try/catch in tests that should |
| // not generate exceptions |
| |
| try |
| { |
| string id1 = service.Add("name1", ""); |
| string id2 = service.Add("name2", ""); |
| ASSERT_THAT(id1, Ne(id2)); |
| } |
| catch (...) { |
| FAIL(); |
| } |
| } |
You design most of your tests for the happy path that should never generate an exception. If you similarly add a try/catch block to ten other tests that call add, you’ve added sixty lines of exception handling code. The unnecessary exception handling code only detracts from readability and increases maintenance cost.
The relevant test code is so much clearer when it’s not buried amid exception handling clutter.
c7/14/libraryTest/BranchServiceTest.cpp | |
| TEST_F(BranchServiceTest, AddGeneratesUniqueId) |
| { |
| string id1 = service.Add("name1", ""); |
| string id2 = service.Add("name2", ""); |
| |
| ASSERT_THAT(id1, Ne(id2)); |
| } |
Not all unit test frameworks support Hamcrest-style notation (ASSERT_THAT). Or your codebase may be a little older, still using classic form assertions (ASSERT_TRUE, for example; see Classic Form Assertions). (Or you might not find much value in Hamcrest.)
Hamcrest-style assertions have a bonus benefit of improved failure messages. In contrast, when a simple ASSERT_TRUE fails, the resulting failure message may not instantly tell you all you want to know. Some frameworks, such as CppUnit, let you provide an additional argument representing a message to display if the assertion fails.
| CPPUNIT_ASSERT_MESSAGE(service.Find(*eastBranch), |
| "unable to find the east branch"); |
My recommendation is to omit such assertion failure comments. If added to every assertion, they result in considerable clutter that detracts from the ability to easily read the test and increases the amount of code that must be maintained. The assertion without the message reads well.
| CPPUNIT_ASSERT(service.Find(*eastBranch)); |
The assertion with the message doesn’t add anything useful. Like normal code comments, you should strive to obviate the need for them. If your assertion doesn’t make sense without an assertion failure comment, address the other problems in your test first.
If a test unexpectedly fails in the midst of a suite run, the reason might not be immediately clear from the failure output. Usually you’ll get the information you need, though, to be able to pinpoint the failing line of test code. If necessary, add a temporary failure message and run again.
If you must add comments to explain what your test does, you’ve missed the point of using tests as documentation. Rework your test, focusing on better naming and improved cohesion.
You might at times see test code that looks like this:
c7/15/libraryTest/BranchServiceTest.cpp | |
| // test that adding a branch increments the count |
| TEST_F(BranchServiceTest, AddBranchIncrementsCount) |
| { |
| // first branch |
| service.Add(*eastBranch); // East |
| ASSERT_THAT(service.BranchCount(), Eq(1)); |
| |
| // second branch |
| service.Add(*westBranch); // West |
| ASSERT_THAT(service.BranchCount(), Eq(2)); // count now 2 |
| } |
Some folks find it helpful, but comments shouldn’t restate what code already clearly states—or could clearly state if it was better structured. Find a way to eliminate comments while retaining test expressiveness. You can lose all of the comments in the prior example without losing any meaning.
c7/16/libraryTest/BranchServiceTest.cpp | |
| TEST_F(BranchServiceTest, AddBranchIncrementsCount) |
| { |
| service.Add(*eastBranch); |
| ASSERT_THAT(service.BranchCount(), Eq(1)); |
| service.Add(*westBranch); |
| ASSERT_THAT(service.BranchCount(), Eq(2)); |
| } |
“Why does this test assert what it does?” You want readers of your tests to be able to answer that question without wasting time on careful analysis of the test or production code.
You will often move details from the test into SetUp or another helper method. But be careful not to hide too much; otherwise, you’ll make test readers dig about for answers. Proper function and variable naming can go a long way toward keeping the test’s meaning explicit.
The following test requires a little bit of reading between the lines:
c7/16/libraryTest/BranchServiceTest.cpp | |
| TEST_F(ABranchService, ThrowsWhenDuplicateBranchAdded) |
| { |
| ASSERT_THROW(service.Add("east", ""), DuplicateBranchNameException); |
| } |
We might surmise that code in SetUp inserts the East branch into the system. Perhaps all of the tests in the fixture require the existence of one branch, so eliminating the duplication of adding East in SetUp is a good idea. But why require readers to make the extra effort?
We can clarify the test by changing its name and introducing a fixture variable with a meaningful name.
c7/17/libraryTest/BranchServiceTest.cpp | |
| TEST_F(ABranchServiceWithOneBranchAdded, ThrowsWhenDuplicateBranchAdded) |
| { |
| ASSERT_THROW(service.Add(alreadyAddedBranch->Name(), ""), |
| DuplicateBranchNameException); |
| } |
Here’s a simple example of a test that requires the reader to dig deep into detail and count the number of days between two dates:
c7/17/libraryTest/HoldingTest.cpp | |
| TEST_F(AMovieHolding, AnswersDateDueWhenCheckedOut) |
| { |
| movie->CheckOut(date(2013, Mar, 1)); |
| |
| date due = movie->DueDate(); |
| |
| ASSERT_THAT(due, Eq(date(2013, Mar, 8))); |
| } |
Asserting against a simple expression can dramatically increase understanding.
c7/18/libraryTest/HoldingTest.cpp | |
| TEST_F(AMovieHolding, AnswersDateDueWhenCheckedOut) |
| { |
| date checkoutDate(2013, Mar, 1); |
| movie->CheckOut(checkoutDate); |
| date due = movie->DueDate(); |
| ASSERT_THAT(due, Eq(checkoutDate + date_duration(Book::MOVIE_CHECKOUT_PERIOD))); |
| } |
Correlating expected output with test context is a bit of an art. You’ll need to be creative from time to time. You’ll also need to remind yourself that you know the intimate details of what’s going on in the test you just designed, but others wom’t.
Once you get into the habit of organizing your tests using Arrange-Act-Assert/Given-When-Then and expecting to see it in other tests, you slow down a little when you encounter a noncompliant test. Immediate understanding disappears because you must work a little harder to figure out what’s test setup and what’s actual functionality. How long does it take you to discern the goal of the following test (with a deliberately useless name)?
c7/18/libraryTest/HoldingServiceTest.cpp | |
| TEST_F(HoldingServiceTest, X) |
| { |
| HoldingBarcode barcode(THE_TRIAL_CLASSIFICATION, 1); |
| string patronCardNumber("p5"); |
| CheckOut(barcode, branch1, patronCardNumber); |
| date_duration oneDayLate(Book::BOOK_CHECKOUT_PERIOD + 1); |
| holdingService.CheckIn(barcode.AsString(), |
| *arbitraryDate + oneDayLate, branch2->Id()); |
| ASSERT_THAT(FindPatronWithId(patronCardNumber).FineBalance(), |
| Eq(Book::BOOK_DAILY_FINE)); |
| } |
Here is the code reworked using AAA to emphasize what’s relevant:
c7/19/libraryTest/HoldingServiceTest.cpp | |
| TEST_F(HoldingServiceTest, X) |
| { |
| HoldingBarcode barcode(THE_TRIAL_CLASSIFICATION, 1); |
| string patronCardNumber("p5"); |
| CheckOut(barcode, branch1, patronCardNumber); |
| date_duration oneDayLate(Book::BOOK_CHECKOUT_PERIOD + 1); |
| holdingService.CheckIn(barcode.AsString(), |
| *arbitraryDate + oneDayLate, branch2->Id()); |
| ASSERT_THAT(FindPatronWithId(patronCardNumber).FineBalance(), |
| Eq(Book::BOOK_DAILY_FINE)); |
| } |
With the execution statement isolated, it’s immediately clear that the test focuses on check-ins. Further reading across the execution line adds the suggestion that the test is concerned with late check-ins. With that understanding of what gets invoked in the system, you can quickly move to the assert portion of the test and determine that the behavior being verified is that a patron’s fine balance has been updated accordingly.
Your time to understand existing code is one of the larger expenses in software development. Every little thing you do can help diminish some of that effort, and the beauty of AAA is that it costs almost nothing.
Naming things well is one of the most important things you can do in software design. A good name is often the solution to a test correlation problem (see Implicit Meaning). You’ll also find that the inability to derive a concise name indicates a possible design problem.
Tests are no different. A test exhibiting confusing names, or lacking a name where one is needed to explain, does not provide useful documentation.
c7/19/libraryTest/PersistenceTest.cpp | |
| TEST_P(PersistenceTest, RetrievedItemIsNewInstance) |
| { |
| persister->Add(*obj); |
| |
| ASSERT_FALSE(obj == persister->Get("1").get()); |
| } |
The simple change that makes all the difference for the reader is as follows:
c7/20/libraryTest/PersistenceTest.cpp | |
| TEST_P(PersistenceTest, RetrievedItemIsNewInstance) |
| { |
| persister->Add(*objectWithId1); |
| |
| ASSERT_FALSE(objectWithId1 == persister->Get("1").get()); |
| } |
You don’t need to name every relevant piece of data, as long as it can be readily correlated to something else. Here’s an example:
c7/19/libraryTest/PatronTest.cpp | |
| TEST_F(PatronTest, AddFineUpdatesFineBalance) |
| { |
| jane->AddFine(10); |
| |
| ASSERT_THAT(jane->FineBalance(), Eq(10)); |
| } |
It’s obvious that the argument 10 passed to AddFine corresponds to the expected fine balance of 10.
3.145.107.100