TDD: Test only constructors
I wrote previously how we'd been doing some work to change the way that we get a 'User' object into our system and one mistake that we made intially was to have another constructor on the 'User' object which was being used in all our unit tests which involved the user in some way.
The original reason that this 'test constructor' was created was to make it easier to construct a 'fake user' which we were using in some of our functional tests but had ended up being used in unit tests as well.
The constructor being exposed for testing was pretty much the same as the private constructor that we have now.
public class User { ... public User(string userName, string customerId) { this.userName = userName; this.customerId = customerId; } }
The problem with this approach to testing is that we aren't actually testing the code which we're using in production so even if the tests pass it doesn't actually tell us very much about our code.
We could be doing everything right in this test constructor and doing something crazy in the static factory method and we wouldn't find out until much later on.
The interesting thing is that the method that we call from the production code isn't as testing friendly as the one we had made public just for testing:
public static User CreateUserFrom(NameValueCollection headers) { var userName = headers["user-name-header-tag"]; var customerId = headers["customer-id-header-tag"]; // and so on return new User(userName, customerId); }
In order to make our test setup code use this method we had to create a NameValueCollection containing key/value pairs with the appropriate keys that reside in the headers of requests coming into our application.
We therefore end up with code similar to this in the test data builder:
public class UserBuilder { ... public User Build() { var headers = new NameValueCollection(); headers.Add("user-name-header-tag", "randomName"); headers.Add("customer-id-header-tag", "customerId"); User.CreateUserFor(headers); } }
This leaks a bit of the implementation of 'CreateUserFrom' into the tests but I prefer this to testing something which is never actually used.
Looks like "headers" is a parameter object. Remember that book club paper?
I think about it as CreateUserFor having a different interface: the interface takes a NameValueCollection which must have certain keys set. The implementation of CreateUserFor also knows about that interface, but I think that is natural.
If we changed CreateUserFor in such a way that it broke the interface, that would be bad. But it would also be our fault. It is unfortunate that these types of interfaces can't be made explicit in some way. I guess that's what Design By Contract is largely about.
DCam
12 Sep 09 at 3:30 am
Surely adding anything to the 'production code' for a testing purposes is a code smell at the minimum and in the worse case demonstrates that the code is not completely testable – can't be decoupled so therefore can't be tested…
Ollie Riches
12 Sep 09 at 6:46 am
Some thoughts:
1) There's nothing wrong with adding a test constructor – as long as you have tests that also test the real constructor. Some objects (particularly key domain concepts) get instantiated a lot by test code – making that easier and cleaner will help the _other_ tests.
2) You can help clean up the leaking by using a builder to create the name value collection. If the same builder is used in the production code, all the better.
Robert Watkins
12 Sep 09 at 1:37 pm
@Robert – I'd have said that hiding the creation behind the builder as you suggest in 2) would make it less necessary to have the test constructor?
I guess in our case we didn't have specific tests around the real constructor although it was being called in our acceptance tests so it was still being tested somewhere.
Mark Needham
12 Sep 09 at 1:46 pm
You can make all your "just for test" methods internal and then access in your test code by using assembly:InternalsVisibleTo attribute.This is tedious but is better insulted then your solution.
Darshan
12 Sep 09 at 1:49 pm
If it was a Java code, i would have preferred using reflection to create the User object in UserBuilder without changing anything in the current User class.
Not sure if it is feasible in C# (assuming the code you mentioned is written in C#)
Uday
13 Sep 09 at 11:45 pm
Think I ran into the same situation when we were writing some Http Handlers based on Asp.Net in our project. In application, the query parameters are passed to a factory which creates object based on the request. In test, we used a QueryParametersBuilder which will construct this for us.
I think it is similar to your approach of having a UserBuilder which abstracts the process of creating the user from the request. It does leak some implementation but I think as @Dcam has pointed it the part of contract…
Sai Venkat
15 Sep 09 at 6:12 am
Wow… And I wonder why people want to try to use things like InternalsVisibleTo or reflections to test this? I think your solution is way more elegant that those
Sai Venkat
15 Sep 09 at 6:15 am
It looks to me as if your User class has too many responsibilities. One is to define how User objects act (part of your application's domain model). The other is to parse header fields that have come from some message or HTTP request or somewhere and interpret how they are used to define a User.
If you move the latter responsibility out of the User and into a package of classes that define your app's network protocol (for example), then you only need one constructor for the User that is used by both the network protocol package and unit tests.
Nat
21 Sep 09 at 7:33 am
[...] a month ago or so I described how we did some work to ensure that we were calling a class the same way in our tests as… and while I think that was a good choice in that situation we came across a similar problem this [...]
Coding: API readability/testability at Mark Needham
10 Oct 09 at 12:25 am
Hi
I also bumped into invariants checking in constructor/factory dilemma.
The main drawback of test-only constructors/factories is that you can not guarantee that none of your colleagues will use them in production code !!!
On the other side, calling production-available (real) constructors/factories in your test can be very painfull…
Invariant checks inside User.CreateUserFor(headers) are quite trivial so it does not seem to be the case but imagine the scenario when class invariants are very complex and hard to satisfy.
Let’s say you want to create triangle object, feeding it’s constructor with 3 angles.
Triangle(int angle1, int angle2, int angle3)
For triangle all interior angles total to 180 degrees.
This invariant check can be done in constructor itself or in Factory..
This approach seems to be very attractive – you will always work with valid triangles, but as i mentioned above it kills testability:
To test any of triangle behavior (for example checking if triangle is right-angled) you first need to instantiate it feeding it with correct angle values.
For triangle example it still does not seem to be the case, but in more complex scenarios instantiating object to be tested can be quite hard.
So having factories/constructor for tests can be quite tempting but how would you enforce your colleagues to not call them in their code ?
By writing appropriate comments (javadoc)?
By using access level modifiers?
miluch
6 Jan 10 at 9:38 am