Not Taking My Own Advice
by Ted M. Young
The other day I was pairing with a friend of mine on an app I call "Kid-Bank" (tracking how much money my son has, since he doesn't have his own bank account). We were making nice progress, doing basically the BDD->TDD->BDD cycle: discuss a user story ("show current balance"), make an integration test (using MockMvc from Spring Boot) that fails, then drop a level to test-drive the implementation of the core Domain code, popping back up when we think we're done and the integration test should pass. All good stuff.
Until...
We got to a point where an integration test failed and I assumed it was because there was state being held between runs of the individual test methods (we had an Account object that was passed directly into our UI Controller class). On other words, the tests weren't isolated. Since my partner didn't have much experience with Spring Boot integration tests, he took my assumption at face value.
Given our current implementation of the controller directly accessing the Account object, we had no easy way of clearing/resetting its state between tests. We wrote the following in the commit message:
Domain tests pass, but integration tests fail because of interactions between individual test methods and there's no way to reset Account so that it has no transactions.
Next step is to create a Repository, even though there's currently only one account, it will provide a level of indirection that we need to properly isolate the test methods.
One idea was to have a "reset" method on Account, but since the domain logic didn't need it (we had no unit test requiring it), adding a direct way to clear it would have been "cheating". We talked back and forth about creating an AccountRepository, which is a very typical pattern to use, but my partner wasn't happy with this because there was only one account in the entire system: my son's. Trying to abide by the YAGNI (You Ain't Gonna Need It) principle, we decided that implementing an "AccountSupplier" would provide just enough of a level of indirection (the answer to all problems in computer science) to allow us to have a fresh new Account for each test run.
Integration Tests Still Fail?
So we implemented the AccountSupplier, changed the references to get everything to compile and then fixed everything to get the domain (unit) tests to pass again. We then ran the integration tests, and...the same integration tests still failed.
This integration test, which was basically testing the AccountController, ensures that when the system starts up, the Account is empty, i.e., there are no transactions:
@Test
public void newAccountViewHasNoTransactions() throws Exception {
MvcResult mvcResult = mockMvc.perform(get("/"))
.andReturn();
Collection transactions = (Collection) mvcResult
.getModelAndView()
.getModel()
.get("transactions");
assertThat(transactions)
.isEmpty();
}
And when we ran it, it failed with this error:
java.lang.AssertionError:
Expecting empty but was:<[TransactionView(date=01/05/2005, action=Cash Deposit, amount=$12.45, source=Birthday gift)]>
Expected Failure
Now when I teach Java, especially to less experienced folks, I emphasize how important it is to understand why tests fail. Was the failure expected? If so, did it fail in exactly the way you expected? If it failed differently, that's something to investigate. If it didn't fail, that's also a problem.
If a test fails (or passes!) unexpectedly, the next step I teach is to come up with a hypothesis as to why it failed, before trying to fix it. In this case, the hypothesis was that another test had run and left a transaction in the Account. However, we just implemented the AccountSupplier, so we knew this couldn't be the case, because we created a new Account before each test method. That means our hypothesis was wrong.
Once we eliminated that possibility (an artifact of the tests not being isolated), we took a closer look at the AccountController code. Here's what we found:
model.addAttribute("transactions",
Collections.singleton(
new TransactionView("01/05/2005", "Cash Deposit",
"$12.45", "Birthday gift")));Yep, we were hard-coding a response (the new TransactionView), because we had a test that was expecting just that response. Therefore, the individual "newAccountViewHasNoTransactions" test could never have passed, even on its own.
So the mistake I made here was not validating the "tests aren't isolated" hypothesis directly by running that test on its own. I immediately jumped to a more complex explanation, which was one I'd seen before. I suffered from a "curse of expertise", where I assumed it was a problem I saw before because the behavior matched, instead of taking smaller steps to validate the thinking.
Generate Multiple Hypotheses
The best scenario would have been to check the hypothesis by just running the test in isolation, seeing it fail, and immediately fixing the problem.
Another way to go would have been to generate at least two (or more) hypotheses and test the cheapest one first. That doesn't always work, sometimes there's only one reasonable hypothesis, but sometimes forcing yourself to think of other hypotheses can help break you out of having tunnel vision. Thinking is hard (our brains are wired to save us from heavy thinking), so anything you can do to help force yourself to think just a little bit deeper is useful.
Mistakes and Sunk Costs
Once we realized what the real problem was, we could have left the AccountSupplier implementation in place and just fixed the bug in the AccountController, however, the only reason we had added the AccountSupplier was to get around a problem that didn't exist. So why keep the code?
It's easy to justify keeping the code, because it's already written (Sunk Cost), and we're pretty sure we'll need it (or something like it), but we added complexity to the system for no currently needed reason (YAGNI principle). We quickly agreed that there was no reason to keep it, and reverted to the prior commit, undoing the work we had done.
Now we're back to a simpler codebase and can continue around the loop to start on the next User Story.