Jon Galloway and I are embarking on the next phase of our Full Stack project and building a Pomodoro timer. We are (at lest for now) doing this using Test Driven Development.
This immediately raised an interesting issue, which I think I’ve found a solution for but would be eager to hear how others deal with this issue.
Here’s the background. We created a Task object, which so far looks like this (excerpt):
class Task { public int ExpectedPomodoros { get; private set; } public string Name { get; private set; } private Pomodoro _currentPomodoro; public Task( string name, int expectedPomodoros ) { _currentPomodoro = new Pomodoro( TimeSpan.FromMinutes( 20 ) ) ; Name = name; ExpectedPomodoros = expectedPomodoros; }
We can of course have tests for whether the ExpectedPomodoros is greater than or equal to 1, and whether the name is valid, but two problems immediately arise:
- It is tedious to add a test for every member’s validity and
- What do we do about the private member variables such as _currentPomodoro?
Point #2 is that the test framework won’t have visibility into the private member, nor should it, but it would be a pretty big bug to have a null _currentPomodoro – exactly the kind of bug I’d like the Unit Tests to find.
My solution is to reach back 20 years to an old concept in object oriented programming called Invariants. The idea was that each class had a set of invariant conditions that must be true at the start and end of every method (except only at the end of the constructor and, in C+++, only at the start of the destructor). A class was not valid if its invariants were not valid.
Transposing this forward to today’s Silverlight Unit Tests, we get,
public bool Invariants { get { return ! String.IsNullOrEmpty( Name ) && ExpectedPomodoros >= 1 && _currentPomodoro != null } }
and we are now free to write a unit test to test the invariants:
[TestMethod] public void NewTaskIsValid() { Task t1 = new Task("testTask", "a test", 2); Assert.IsTrue( t1.Invariants ); }
One principle of Unit Testing in general and TDD in particular is that each test should test just one thing, and always give an unambiguous answer. This approach violates both of these “rules” — if this test were to fail, and you were testing that 15 things are invariant, you have to figure out which thing was not invariant, and that might involve a bit of work.
My answer is to add only trivial items (e.g., Name is not null or empty) and to add them one by one so if they fail you know which (likely) failed. Further, I don’t see a better way to test private variables.
So, is this brilliant, or old-hat, or just wrong?
I am a huge fan of Code Contracts, which include Invariants, PreConditions and PostConditions. There is also the big advantage of Intellisense for contract (or potential contract) violations.
Based on work done over the past 3 years [the longest period I have metrics for] , in-depth testing (Contracts, TDD, BDD, etc) has resulted in approximately a 35% reduction in overall cost of development. This takes into account the time invested in writing (and maintaining) the tests and the corresponding reduction in defects found later in the development pipeline.
As a side note, I am also in favor of doing the “tedious” testing [which I refer to as Granular Unit Tests or G.U.T.]. It may seem silly to invest time in testing things like automatic properties, but when a decent automation engine is used to create the tests, there are many benefits.
I’m not sure i like it.
You should be testing your private variables through the class’s public interface (but we can’t see that in the example). I think making that invariant is clever, but its going in too much detail on the testing. You end up having strange Invariant properties all around cluttering the production-code.
/Theo
Theo,
I think I agree with you. There is a strong argument that private members should not be tested at all; the unit test should test that the SUT does what it should, not how it does it. Private member variables are part of the “how” and testing them creates a brittle dependency between the test and the implementation.
Another approach would be to code your business logic using Eiffel 😉 for .Net and use C# for the rest. Then you would get Design By Contract(TM) for free and your unit tests would probably look different.
Well, a few weeks ago, i started a project that tries to make it easier to test these kind of things, but taking another approach:
Its a helper that looks at your class instances and warns you (throws an Assertion exception) of what its wrong. If you are interested, take a look at this article that explains in details: http://codemadesimple.wordpress.com/2011/10/27/jnarcissus/. It can’t test private fields, but as for the sake of OOP it should not anyway. 😀
It seems wrong to me. You’re altering the public surface of your object to support your testing methodology which should be a signal that you need to rethink your testing approach.
Code Contracts is the tool that I’d reach for in this case.
Yes but testability is also an important factor to take in account when designing your objects, not sacrificing OOP principles… Dependency Injection is an example of a pattern that should be followed for the sake of (and not only of course) easier Unit Tests.
Wow, didn’t knew this existed… definately a better approach
Agreed with Michael, you should look into Code Contracts invariants, which enforce what should always be true of the object.
On the question of testing private fields/properties/methods, typically you don’t, because it’s an implementation detail. What you need to test is public behavior. In your example, there is nothing to test around currentPomodoro: it has no visible effects to the outside. (you could actually remove it right now…). If you had a public prop that used the field, then a test would be warranted.
I also thought along these lines, where is the behaviour we want to test? The 20 minutes is obviosly meant for a timer, so why not write a test which verifies that the Task will start a timer of 20 minutes?
Invariants and contracts are fine, but a little premature at this point, I think.
Isn’t this what code contracts is for?
You can validate the inputs to your individual methods with Contract.Requires and test that the appropriate exception is raised when you pass invalid parameters. If you need to check object state rather than parameter inputs, you can use the ContractInvariantMethod (see this example http://devlicio.us/blogs/derik_whittaker/archive/2009/07/01/code-contracts-primer-part-5-utilizing-object-invariants.aspx)
Michael.
I’d expect task to take a pomodoro factory:
class Task
{
public int ExpectedPomodoros { get; private set; }
public string Name { get; private set; }
private Pomodoro _currentPomodoro;
public Task( string name, int expectedPomodoros, IPomodoroFactory factory)
{
_currentPomodoro = factory.CreateTwentyMinutePomodoro();
Name = name;
ExpectedPomodoros = expectedPomodoros;
}
and your test would mock the factory and verify the expectation (moq):
[Test]
public void CreatesCurrentPomodoroFromFactory()
{
var factory = new mock();
var uut = new Task("taco", 1, factory.Object);
factory.Verify(v => v.CreateTwentyMinutePomodoro):
}
I like Ritch Melton’s method above, I’m a big BDD fan and his design is similar to how I’ve resolved issues like this in the past.