I’ve been reading a lot of code lately. When I’m doing this, I find it very important to have some unit tests that makes it easier for me to comprehend the actual production code. In order to do that the unit tests have to very readable.
Something that I see quite a lot in the projects that I’m involved with is what I call ‘Betrayal of Context’. In short, this means that some BDD style unit tests (specifications) are not eligible for the context that has been set up for them. This results in unit tests that are more verbose than they should be which makes them harder to read.
As a piece of code can say more than a thousand words, let me show you the simplest example I could think of.
public class ApplicationRequest
{
private ApplicationRequestStatus Status { get; set; }
public Boolean IsApproved()
{
return ApplicationRequestStatus.Approved == Status;
}
public void ApproveUsing(IStrictRegulation
strictRegulation)
{
if(ApplicationRequestStatus.Pending != Status)
throw new InvalidOperationException("Oeps");
if(strictRegulation.Complies(this))
{
Status = ApplicationRequestStatus.Approved;
}
else
{
Status = ApplicationRequestStatus.Rejected;
}
}
}
public enum ApplicationRequestStatus
{
Pending = 0,
Approved = 1,
Rejected = 2
}
public interface IStrictRegulation
{
Boolean Complies(ApplicationRequest request);
}
What we have here is an utterly useless domain that handles application requests, but it will do for our example. In order to get approved, an application request needs to comply to some strict regulations.
Now that we’ve got ourselves acquainted with the subject-under-test, let me show you some example of what I consider ‘Betrayal of Context’.
[TestFixture]
[Category("ApplicationRequestTestFixture")]
public class When_approving_an_application_request
: InstanceSpecification<ApplicationRequest>
{
protected override void Establish_context()
{
StrictRegulationStub = MockRepository
.GenerateStub<IStrictRegulation>();
StrictRegulationStub.Stub(strictRegulation =>
strictRegulation.Complies(null))
.IgnoreArguments()
.Return(true);
}
[Test]
public void Then_it_should_be_approved_if_it_meets_strict_regulations()
{
SUT.ApproveUsing(StrictRegulationStub);
Assert.That(SUT.IsApproved());
}
[Test]
public void Then_it_should_be_rejected_if_it_does_not_meet_strict_regulations()
{
StrictRegulationStub.BackToRecord();
StrictRegulationStub.Stub(strictRegulation
=> strictRegulation.Complies(null))
.IgnoreArguments()
.Return(false);
SUT.ApproveUsing(StrictRegulationStub);
Assert.That(SUT.IsApproved(), Is.False);
}
[Test]
[ExpectedException(typeof(InvalidOperationException))]
public void Then_an_exception_should_be_thrown_if_its_status_is_not_pending()
{
SUT.ApproveUsing(StrictRegulationStub);
SUT.ApproveUsing(StrictRegulationStub);
}
protected override ApplicationRequest Create_subject_under_test()
{
return new ApplicationRequest();
}
private IStrictRegulation StrictRegulationStub
{ get; set; }
}
I don’t know about you, but I have issues with this code. Two out of three specifications have nothing to do with the context setup in the Establish_context method. In fact, the second unit test needs to redo the entire setup for the stub object. All too often I see this happening, which is bad for my heart (at least that’s what my doctor keeps telling me 🙂 ). By organizing unit tests this way, we are also missing out on the ‘Because’ goodness I’ll show you later on.
The thing that disturbs me the most is that these ‘shortcuts’ add clutter which makes them less readable than they should be. That’s what our craft is all about. Communicating! Not only with the compiler, but most importantly with the poor fellow that comes after you (in this case, me!) and needs to understand what you have been doing.
So let us refactor these specifications and put them in the right context.
public abstract class behaves_like_an_application_request_that_meets_strict_regulations
: InstanceSpecification<ApplicationRequest>
{
protected override void Establish_context()
{
StrictRegulationStub = MockRepository
.GenerateStub<IStrictRegulation>();
StrictRegulationStub.Stub(strictRegulation
=> strictRegulation.Complies(null))
.IgnoreArguments()
.Return(true);
}
protected override void Because()
{
SUT.ApproveUsing(StrictRegulationStub);
}
protected override ApplicationRequest Create_subject_under_test()
{
return new ApplicationRequest();
}
protected IStrictRegulation StrictRegulationStub
{ get; set; }
}
[TestFixture]
[Category("ApplicationRequestTestFixture")]
public class When_approving_a_pending_application_request_that_meets_strict_regulations
: behaves_like_an_application_request_that_meets_strict_regulations
{
[Test]
public void Then_it_should_get_approved()
{
Assert.That(SUT.IsApproved());
}
}
[TestFixture]
[Category("ApplicationRequestTestFixture")]
public class When_approving_an_application_request_that_is_not_pending
: behaves_like_an_application_request_that_meets_strict_regulations
{
[Test]
[ExpectedException(typeof(InvalidOperationException))]
public void Then_an_exception_should_be_thrown()
{
SUT.ApproveUsing(StrictRegulationStub);
}
}
[TestFixture]
[Category("ApplicationRequestTestFixture")]
public class When_approving_a_pending_application_request_that_does_not_meet_strict_regulations
: InstanceSpecification<ApplicationRequest>
{
protected override void Establish_context()
{
_strictRegulationStub = MockRepository
.GenerateStub<IStrictRegulation>();
_strictRegulationStub.Stub(strictRegulation
=> strictRegulation.Complies(null))
.IgnoreArguments()
.Return(false);
}
protected override void Because()
{
SUT.ApproveUsing(_strictRegulationStub);
}
[Test]
public void Then_it_should_not_get_approved()
{
Assert.That(SUT.IsApproved(), Is.False);
}
protected override ApplicationRequest Create_subject_under_test()
{
return new ApplicationRequest();
}
private IStrictRegulation _strictRegulationStub;
}
Oh no, you’ve started out with only one test fixture and now you’ve got three of them and one base class? How can this be better? Well, I think it is better.
Notice how the context setup code that leaked into the specifications is now moved to where it belongs, namely the Establish_context method where everything is arranged.
By putting each specification in the right context (which is represented by a test fixture), I’ve been able to act on the subject-under-test in a single reusable method named ‘Because’. This saves a lot of copy/paste kung fu when we have a real-world scenario with more than one specification per context.
Also notice that the specifications themselves are now reduced to a single line of code that only asserts the outcome. The fact that there is no more than a single line of code ensures me that I can’t get the specifications any simpler than that, which makes everything very comprehensible.
I’ve moved some common context code into a base class. This is probably overkill for this simple example, but I wanted to show this because it can be a life saver whenever the complexity starts to increase.
Anyway, some last advice for 2008: stay faithful to your context.
How is this any different than using a [SetUp] method with a private member variable of the fixture for the SUT? All of your “Establish_context” and “Because” code goes in the setup method and you’re done…why make it so complicated? Perhaps a base class like FixtureFor with a SUT property would be handy, but beyond that – what’s the point?
@Matt The point is readability (as always). Seperating AAA into seperate methods Arrange (Establish_context), Act (Because), Assert (Then_ … _should methods) makes everything a lot more clear. Unit test code doesn’t need to be deciphered anymore, they can just be read.
If you noticed, I’m using a base class for this. I posted the code of this class in one of my previous posts:
https://elegantcode.com/2008/10/25/refining-contextspecification-bdd-using-rhino-mocks-35/
@Jan:
Have you ever used “Arrange” and “Act” and “Assert” as method names instead of “Establish_context” and “Because”?
I think they are more grokkable.
@Olof: I’m trying to describe what the code should do instead of using technical terms that only developers understand.
@Jan: I’m with you, but “Arrange” does read a lot easier to non-developers than “Establish_context”, don’t you think?
And “Act” is easier than “Because” (Because is really non-intuitive IMHO..).
Of course these are opinions – but I did see you use the words Arrange and Act in your comments about Establish_context and Because — often comments use easier language than code your writing. It’s a smell of a rename-refactoring coming to life so to speak 😉
Cya!
@Olof: You’ve got a point there. I’ll think about your naming suggestion.
@olof AAA and the semantics around it are developer cnstructs. The goal is not that non-developers should be able to “grok” your tests, the goal is to make your tests scannable so that developers can verify the behavior of the system.
The test and class names, on the other hand, can be used to express more naturally what is happening, and you could use a tool to parse them into a report, so that a customer or business analyst can see:
When approving an application that does not meet strict regulations:
– It should not get approved.
This has meaning and should match the domain language. The tests are for developers however.
@jan “Betraying the context” I like that a lot. I see this antipattern time and again, and I share your belief that refactoring it into further fixtures is the proper way to handle it. If you haven’t used your established context in a test, that should be a red flag that you are doing something wrong.
@Scott Not using the establish context method is indeed a major red flag. This can be prevented though by making the establish context method abstract in the base class. I’ve also seen ‘context betrayal’ when the establish context method is used, which makes everything more confusing.
@Jan, no I didn’t mean the establish_context() method, I mean, if you have established a context in your fixture, and you write a test that doesn’t use it at all, or overrides a significant portion of it, then it’s a smell for sure. Hence the need to refactor to a new fixture (which is, in essence, a new context).
How about simply using Given() instead of Establish_context()?
I am trying to find a structure that reads well to the developer and to a report in a class hierarchy. Elusive problem.