Recently a Schalk Cronjé forwarded me a tweet from Joshua Lewis about some unit tests he’d written.
. @ysb33r I’m interested in your opinion on how expressive these tests are as documentation https://t.co/YpB1A3snUV /@DeveloperUG
— Joshua Lewis (@joshilewis) December 3, 2015
I took a quick look and thought I may as well turn my comments into a blog post. You can see the full code on github.
Comment 1 – what a lot of member variables
Why would we use member variables in a test fixture? The fixture is recreated before each test, so it’s not to communicate between the tests (thankfully).
In this case it’s because there’s a lot of code in the setup() method (see comment 2) that initialises them, so that they can be used by the actual tests.
At least it’s well laid out, with comments and everything. If you like comments – and guess what – I don’t. And they are wrapped in a #region so we don’t even have to look at them, if our IDE understands it properly.
Comment 2 – what a big setup() you have
I admit it, I don’t like setup()s – they move important information out of the test, damaging locality of reference, and forcing me to either remember what setup happened (and my memory is not good) or to keep scrolling to the top of the page. Of course I could use a fancy split screen IDE and keep the setup() method in view too, but that just seems messy.
Why is there so much in this setup()? Is it all really necessary? For every test? Looking at the method, it’s hard to tell. I guess we’ll find out.
Comment 3 – AcceptConnectionForUnconnectedUsersWithNoPendingRequestsShouldSucceed
Ok, so I’m an apostate – I prefer test names to be snake_case. I just find it so much easier to parse.
The name, though, is pretty good. Very descriptive, although I’m still not sure what ShouldSucceed really means.
The problem starts when I try to relate the comment (C):
///GIVEN User1 exists AND User2 exists AND they are not connected AND User1 has requested to Connect to User2
with the name of the test (N), which incorporates the text: WithNoPendingRequests
and the code in the test (T):
Given(user1Registers, user2Registers, user1RequestsConnectionToUser2); When(user2AcceptsRequestFrom1); Then(succeed); AndEventsSavedForAggregate<User>(user1Id, user1Registered, connectionRequestedFrom1To2, connectionCompleted); AndEventsSavedForAggregate<User>(user2Id, user2Registered, connectionRequestFrom1To2Received, connectionAccepted);
So, the name (N) says that:
- the users should not be connected, and
- they (?) should have no pending requests [this is probably a cut and paste error]
The comment (C) says that:
- the users should not be connected, and
- user1 has requested a connection to user2
And the test (T) says:
- nothing explicit about whether they are already connected
- nothing explicit about pending requests [although there is a request connection command, which is implicitly pending]
- plenty of assertions on events that have nothing directly to do with identifying a successful connection
Why is any of this a problem? Well, in the first place the tests (a.k.a. specification) should be consistent and easy to understand. These tests are way better than many that I see, but still it’s worth thinking about how they can be even better.
And, secondly, tests should only assert on the behaviour that they are actually interested in. Over-specifying a test makes it brittle in the face of change, and one thing we can do without is brittle test suites.
Criticism is easy – is there an alternative?
- Use builders to create instances at the point you need them.
- Use methods on the builder to express attributes that are important for the behaviour being validated in the test.
- Only assert on the event(s) that are directly related to the behaviour being validated in the test (that may require the writing of more tests)
This is one way you could write it in Gherkin, using Roger (the requester) and Andrea (the accepter):
Given Roger and Andrea are not connected And Roger has made a connection request to Andrea When Andrea accepts the connection request Then Roger and Andrea are connected
So here’s my re-write. Note that there are utility classes/methods that would need to be written to allow this to compile:
[Test] public void AcceptingConnectionRequestFromUnconnectedUserShouldSucceed { User roger = userBuilder.withNoConnections().build(); User andrea = userBuilder.withNoConnections().build(); roger.requestsConnectionWith(andrea); andrea.acceptsConnectionRequestFrom(roger); assertThatConnectionExistsBetween(roger, andrea); }
I’ve dropped the Given/When/Then format, because, though it was concise, I don’t find that it adds much when we’re at the implementation level. In fact, the very cleverness that allows a list of commands or events to be handled by the G/W/T doesn’t work for me – I find that it obscures what’s going on.
Instead, I’ve stuck to old school arrange/act/assert structure delineated by white space. I’ve posited the existence of a user builder object and a user class for use in the test code. Is this an overhead? Sure, but it will get used over and over again, and it localises the behaviour in a single place, so that when the flow of events changes, or new flows are identified, there’s only a single place in the code that needs maintenance.
I’ve also suggested the withNoConnections() method – which will likely be a no-op – to emphasise that being unconnected is important. You may consider this overkill in this situation, since there’s it’s unlikely that two freshly created users will be connected. I prefer to be explicit about these things.
A question I’m still left with is “how should we implement assertThatConnectionExistsBetween()“. My initial thought would be that it would check that the connectionCompleted() event had been recorded, but that really only checks that the event has been emitted, not that the connection has actually taken place. Without digging deeper into the domain it’s hard to know which approach is more appropriate.
Leave a Reply to Joshua Lewis Cancel reply