Take a look at this code:

   1: public class ClassToTest {

   2: 

   3:     public void MethodToTest()

   4:     {

   5:

   6:         IDAL dal = new DAL();

   7: 

   8:         int discount = dal.GetDiscount();

   9: 

  10:         if (discount > 10)

  11:         {

  12:             // Do something

  13:         }

  14: 

  15:     }

  16: }

The tight coupling to DAL just stands out like a sore thumb. One consequence of this tight coupling is that it's hard to test if we don't have an underlying database setup with correct values. Forget about testing this while taking a shower, unless you have a habit of taking your SQL Server box to the bath tub with you.

Take a look at the following approach to making this code testable without having a database:

   1: public class ClassToTest {

   2: 

   3:     public void MethodToTest()

   4:     {

   5:

   6:         DAL dal = new DAL();

   7:

   8:         #if testing_in_shower

   9:         int discount = 3;

  10:         #else

  11:         int discount = dal.GetDiscount();

  12:         #endif

  13:         if (discount > 10)

  14:         {

  15:             // Do something

  16:         }

  17: 

  18:     }

  19: }

Are you still here? Didn't you just fall off of your chair, not knowing whether to laugh or cry in anguish? It's just plain wrong isn't it? Code Smell just takes on a whole new dimension (and consistency?). Something like this shouldn't be done for many reasons. It makes our code testable but it's wrong.

Introducing code for testing purposes

The reason I bring this up now is because in the past 2 weeks I've seen a few references and examples of introducing code just for the sake of testability. Two weeks ago I was skimming over an article that was explaining unit tests. The class the author was trying to test had a dependency that was causing issues during his tests. The solution offered was to introduce a new constructor and inject the dependency in. In our case, the ClassToTest would like:

   1: public class ClassToTest {

   2: 

   3:     public ClassToTest()

   4:     {

   5:

   6:         // Initialize some stuff

   7: 

   8:     }

   9: 

  10:     public ClassToTest(IDAL dal)

  11:     {

  12:         // Initialize some stuff

  13:         _dal = dal;

  14:     }

  15: 

  16: ....

Now that's great. The problem is that the author left both constructors in there, emphasizing that one is for testing purposes and the other for production code. Why that's wrong I'll get to in a moment.

The other day, Scott Hansleman made a blog post on testing some code that had a dependency on the current principal. His solution was to inject IPrincipal into various methods of the Controller, where Controller is in the context of an ASP.NET MVC Controller. Personally I don't agree with the solution but that's another subject (and another post on a different approach I have in the backlog for the same scenario). However, injecting the dependency seemed valid enough. Later on he updated the post with the following:

UPDATE: Phil had an interesting idea. He said, why not make method overloads, one for testing and one for without. I can see how this might be controversial, but it's very pragmatic.

1. [Authorize] 
2. public ActionResult Edit(int id) 
3. { 
4. return Edit(id, User); //This one uses HttpContext
5. } 

	[Authorize]
	public ActionResult Edit(int id)
	{
	return Edit(id, User); //This one uses HttpContext
	}
	

You'd use this one as before at runtime, and call the overload that takes the IPrincipal explicitly for testing.

The same thing has happened as in the case of an overloaded constructor. Code has been introduced into the base just for the sake of testability. In the previous case it was a new constructor, and in this case, a new method.

Why is this wrong?

Introducing code just so that you can test it is wrong for several reasons, including an increase in noise, decrease in readability and more importantly, permitting the possibility of production code that will not be tested. It might seem improbable at first, seeing that the overloaded versions always call the base ones, but as your code base grows, by allowing this habit, you can promote it to other structures such as conditionals, switch statements, etc, and not limit it only to methods or constructors. Then you'll start initializing values that only make sense in tests and gradually your code starts to smell more and more. To top it off, you can throw in some compiler conditional that rips those parts of the code out from the production version.

Code should also serve as documentation. The cleaner you make your code, the less noise and redundancy you add, the easier it is for others to understand and maintain.

Conclusion

Being pragmatic is good, but don't do it at any cost. Try and not introduce code into your code base that is there exclusively for the purpose of testability (and no, dependency injection is not only for testability). Anything that has to do with testing, keep it where it should be, in the test assemblies. Having overloaded methods or constructors, mocks or stubs in your code base is just as bad as having compiler conditionals in your code for testability. If you run into issue where your code is hard to test, step back and re-think your design. Maybe the problem you're facing now that's impeding your tests is actually a can of worms waiting to be opened. And the sooner you open it, the better. That's one of the great advantages of Test Driven Development.

Just reading a comment on Uncle Bob’s blog by a guy called Ted.

What’s with this whole TDD/SOLID and Podcast stuff?

Just because you practiced TDD does not make your code clean nor high-quality.

Let’s face it, most TDD practitioners are “DEVELOPERS”. You are not “QA”, “TESTER”, “SDET”. What do you know about various testing techniques, testing principles, testing metrics, how to test, etc?

Do our field a favor TDD-er, do the following:
1. How We Test Software at Microsoft
2. Go to MS and apply for SDET position (or Google)
3. Read more books about Software Testing
4. Ask yourself if you wrote great TDD code or not

Now for the last question, if the answer is “Yes”, then you’re a Tester. If the answer is “No”, then you’re a developer. And obviously, if you are a developer, you suck at TDD.

It’s just how the brain is wired or perhaps it’s in your training. Developers are almost always bad testers, vice versa.

Maybe TDD should be renamed "Coming up with a good design by first trying to use my code Driven Development". There is an overwhelming amount of information regarding Test Driven Development and yet, still, STILL, people think it’s about testing.

I can't stress how important naming is, in all aspects, not only tests. The first thing someone looks at when viewing code is the name, be it a class, variable or method. Naming even plays a more important role when isolating functionality. If you are following the MVP pattern to separate the view from the presenter, don't go using method names that reference any sort of visual control. Don't make a presenter method called SetActiveTab.  

It's not just a name. It's important.

I was having a conversation with a colleague of mine the other day. We were talking about user interfaces and how a sketch of what the interface should look like would help him have a clearer picture of the design. This is not that odd. Many use the same technique to show customers what a possible interface of the end application could look like. The customer can then evaluate whether he/she likes it or prefers to change some detail (and it's never just a color). 

For this purpose, some use designer tools to sketch out a user interface, while others work with development tools hoping that they don't have to throw out the prototype they printed on paper to show the customer.

A few decide to make hand-made sketches because they believe showing the customer a WinForm/WPF/PNG can mislead them into believing that the work is nearly finished. If that is the case, educate the customer, don't blame the "messenger" (be it whatever electronic format).

Irrelevant of the technique used, many see this as a natural process; it's not peculiar or odd to show a customer what the user interface might look like, despite the fact that behind that user interface there is no functionality.

Take the previous search screen. It was displayed to Joe (a fictitious customer) and he decided he didn't  like it . He said he preferred to have to click on a Search Records button which then would pop up another screen where he would fill in the data and then hit Search (Joe was having a bad day).

The developer took the feedback into account and designed this particular functionality (not what Joe wanted but what the developer thought Joe really needed). Is there anything wrong with that approach? No. It is perfectly valid. Customer feedback is a good thing, especially when it's really taken into account.

Now let's change roles. Let's make the customer be the developer and the user interface be a specific class. What is the developers's user interface to that class? It is the API of the class. It's the properties and method calls. The requirement of class, much in line with that of the form, is to search for particular customers based on a series of fields and return a list of them.

The developer starts out by designing the API like so:

   1: public class CustomerSearch

   2:     {

   3:         public ICollection<Customer> Search(string name, string surname) {

   4:             

   5:             // Call database

   6:             // Add search conditions

   7:             // Filter records

   8:             // return records

   9:  

  10:             return new Collection<Customer>();

  11:         }

  12:         

  13:  

  14:     }

[At this point, you must be wondering WTF is this all about? To be honest, I've lost track myself, but let's hold the thought for a moment...] 

The developer, Jack, then starts to use the interface and as he's a good developer, his first real usage of the interface is a test. As he's writing the test, he realizes that he's made a mistake. He's limited the Search method to two fields, and the customer record has five fields. He then goes back to the class and changes the method to use specifications.

   1: public class CustomerSearch

   2: {

   3:         

   4:     public ICollection<Customer> Search(Criteria criteria) {

   5:     

   6:         // Call database

   7:         // Add search conditions

   8:         // Filter records

   9:         // return records

  10:  

  11:         return new Collection<Customer>();

  12:     }

  13:     

  14:  

  15: }

He then changes the whole code to implement the new functionality. Once he's happy with the result, he then returns to the test and starts to write the test, happy that he's accomplished his objective. Problem is, he threw away 2 hours of work by not thoroughly thinking through the requirements.

But you must be thinking why he hadn't thought of this beforehand. I mean it is pretty obvious, specially if you've done this before. The reason is that you don't know it all up front. It's only when you start using something that it "hits" you. Just like customers don't know everything up front, and they ask for changes as they start to use the program, as developers we don't know it all up front either. As much design as we try to do on paper, it's not until we actually start to use our code that we find we need to change our design.

Jack could have skipped the first implementation altogether. He could have started using the interface before implementing it completely. Wait a second. How could he use the class before it existed? Well let's think back. What was Jack's usage? He started writing his test like so…

   1: [Test]

   2: public void CustomerSearch_When_Name_Is_Joe_Should_Return_All_Customers_Named_Joe()

   3: {

   4:     CustomerSearch customerSearch = new CustomerSearch();

   5:     

   6:     customerSearch.Search(....    

   7: }

It's when he started writing Search when he realized that he only had two fields and that a customer could have up to five fields. This is where he realized he needed more fields, and the initial design of passing in field names was not very scalable.

Welcome to Test Driven Development

If Jack had started to use his own code before implementing it, he would have realized his shortcomings. Here is where TDD comes into play. TDD is not about writing your unit test first, it's about designing your code. It's about you, as the user, defining your user interface. Much in the same way we sketch out a user interface for the customer to give his feedback before we implement the functionality, writing tests is defining the calls you need for your classes before implementing them. As an added value, you end up with a test so that as you re-factor your design to suit your needs, you can validate the functionality.

I know this is old news to many, but still to date, it amazes me at the amount of developers that still think that Test Driven Development is about writing tests first. It is, but not to have your test, but to define your usage.

I was reading a comment in reply to this post on DelphiExtreme, by Ken Knopfli, and I quote:

Delphi programmers tend to be old experienced hands, have seen it all before, and can see hype for what it is.

Most of us have years of experience and have developed our own way of working. A young chap here is a big fan of D/NUnit and now we have two people that do nothing but create and maintain unit tests. Uh huh.

My customers want results and don’t care if I am “behind the times".

I think it's sad and wrong to think like that. We've all used RAD tools for many years, be it Delphi, Visual Basic or Visual Studio. Used like demoware they have one fundamental flaw, they create unmaintainable solutions in more ways than one. Agile practices, among other things, are about trying to prevent that.

Unit testing is not only about testing your code in isolation. It's about regression testing. Creating unit tests allows you to test your code anytime you introduce a change, be it a bug fix, be it a new feature. Unit Testing is about documentation. It's about letting a new member of your team understand the specs of the application one step at a time. Unit test can be and many times is (TDD) about design.

Separation of concerns, single responsibility, loose coupling are not buzzwords. They are fundamental design principles that can save you in the long and short term.

In reference to Lex's comment, I'll be doing talks both at SDC and at EKon (European CodeGear) about these topics, so if you're in Europe and interested, might be worthwhile coming. In any case, these are principles that can be taught irrelevant of the language, be it Delphi, C#, Lisp or Smalltalk. It's about good object orientated programming, it's not about a platform or an environment. Let's stop being the children of the drag'n'drop era.

In a follow-up to my previous post on mocking, here is the same code re-factored so that it does not depend on the internal behavior of GetAllEmployeesByCompanyId

   1: [TestMethod]

   2: public void GetAllEmployeesByCompanyId_When_Employees_Are_Marked_Returns_Non_Marked_Employees()

   3: {

   4:     IEmployeeRepository employeeRepository = MockRepository.GenerateMock<IEmployeeRepository>();

   5:  

   6:     User user = new User { Id = Guid.NewGuid()};

   7:  

   8:     Employee employee = new Employee {Id = Guid.NewGuid()};

   9:  

  10:     Employee deletedEmployee = new Employee { Id = Guid.NewGuid(), DeletedBy = user};

  11:  

  12:     employeeRepository.Stub(x => x.GetEmployeesByCompanyId<Employee>(Guid.Empty)).Return(new List<Employee>

  13:                                                                                              {

  14:                                                                                                  employee,

  15:                                                                                                  deletedEmployee

  16:                                                                                              });

  17:  

  18:     var EmployeeServices = new EmployeeServices(employeeRepository);

  19:  

  20:     var employees = EmployeeServices.GetAllEmployeesBy
CompanyId(Guid.Empty);

  21:  

  22:     Assert.AreEqual(1, employees.Count);

  23: }

I've done quite a bit of re-factoring, taking advantage of some of the C# 3.0 language features. I'm also using RhinoMocks 3.5 which also makes use of extension methods. In general the test is much cleaner, less noise. In fact, apart from two "strange" calls (lines 4 and 12), you wouldn't even notice it's using mocks.

The important change is that its no longer brittle. If you recall correctly, from my previous post, the purpose of the test was to validate that when GetAllEmployeesByCompanyId, only records not marked for deletion were returned. It was not about interaction or the internal behavior of the call. As such, we've removed this tight coupling to the actual implementation by

a) Replacing the strict mock with a dynamic mock [Line 4].
In version 3.5 of RhinoMocks, you no longer need to create a mock repository if you don't want to. As such, you have static methods on the MockRepository class that generate mocks for you. By default, the mock generated is Dynamic. Therefore, the call MockRepository.GenerateMock is a dynamic mock.

b) Removing the call to VerifyAll.
Since we do not set any expectations, we do not really need or want to verify that any specific calls were made. Again, that's not the purpose of this particular test.

In line 12, we've stubbed the call to GetEmployeesByCompanyId of EmployeeRepository so that it returns a list with two employees. If the repository call were to not return a value, we wouldn't even need to explicitly stub it.

As you can see, we are still using mocks, yet we are not making our test volatile to change. We are using mocks to mock out non existent functionality. This doesn't mean to say that we shouldn't use mocks for verifying behavior. But we should only do that if the purpose of the test is that. In this case it isn't.

RhinoMocks 3.5 is pretty cool. It allows a lot of the noise surrounding mocking to be eliminated in tests. Hopefully I'll blog more about it as I use this version more, but be sure to check it out (Don't be lazy…Just Google it!)

Take a look at the following test:

   1: public void EmployeeServicesTest_GetAllEmployeesByCompanyId_When_Employees_Are_Marked_Returns_Non_Marked_Employees()

   2:        {

   3:            var guid = Guid.NewGuid();

   4:            var guid2 = Guid.NewGuid();

   5:            var guidComp= Guid.NewGuid();

   6:  

   7:            MockRepository mocks = new MockRepository();

   8:  

   9:            IEmployeeRepository mockRepository = (IEmployeeRepository)mocks.StrictMock(typeof(IEmployeeRepository));

  10:  

  11:  

  12:            User user = new User();

  13:            user.Id = Guid.NewGuid();

  14:  

  15:            Employee deletedEmployee = new Employee();

  16:            deletedEmployee.Id = guid;

  17:            deletedEmployee.DeletedBy = user;

  18:  

  19:            Employee validEmployee = new Employee();

  20:            validEmployee.Id = guid;

  21:  

  22:            Expect.Call(mockRepository.GetEmployeesByCompanyId<Employee>(guidComp)).Return(new List<Employee>() { deletedEmployee, validEmployee });

  23:  

  24:            mocks.ReplayAll();

  25:  

  26:            var EmployeeServices = new EmployeeServices(mockRepository);

  27:  

  28:            var testList = EmployeeServices.GetAllEmployeesByCompanyId(guidComp);

  29:  

  30:            Assert.AreEqual(1, testList.Count);

  31:  

  32:            mocks.VerifyAll();

  33:        }

I won’t get into the naming conventions since I’ve covered those in previous blog entries. This test has another problem. It’s trying to validate that when you ask for a list of employees, only those that are not marked as deleted are returned. To simulate the data returned, it is making use of a mock repository (RhinoMocks).

Before looking at the issue, let’s decide what the purpose of the test is. We want to test that when a system has two employees, one of whom is marked for deletion, a call to GetAllEmployeeesByCompanyId returns only one employee. Therefore the SUT is that method call.

In line 9, a mock of the repository is created, of type StrictMock. This is a particular call of RhinoMocks, but pretty much all mocking frameworks have the same feature. A strict mock indicates that every expected call should be made. Therefore, when on line 32 a call to VerifyAll is made, this test will fail if mockRepository.GetEmployeesByCompanyId has not been called with those exact arguments. In our case, the test passes and all is fine. Or is it?

The problem however if we change the internal functionality of EmployeeServices.GetAllEmployeesByCompanyId. If some reason or other we make a second call to EmployeeRepository, that doesn’t change the outcome of the method, this test will fail.

Why is that? The reason isn’t so much that we are using StrictMock as opposed to DynamicMock. The problem is that we are trying to test too many things in the same test. In actual fact, in this particular case, only ONE thing should be tested: retrieving one employee out of two. However the introduction of a StrictMock and the VerifyAll call has had the side-effect of introducing an interaction test, something we are not interested in.

Mocks can be great and very powerful. But use them wisely. If you start creating strict mocks (which should only be used for interaction/behavioural testing), then you’re just making your tests very brittle. The slightest change in behaviour, even though it’s encapsulated inside the method being tested can break your test. This pretty much defeats the whole advantage of unit tests. We create tests to isolate functionality and test that functionality in isolation. As soon as we tightly couple our tests to a particular implementation, and when the system under test is not trying to validate that implementation, the whole thing blows up. In another blog post, I’ll re-factor the test to make it much simpler and less volatile.

I try, as much as I can to practice TDD, because I truly believe that it helps me shape the design, and that is the primary objective of TDD, not tests. Testing is a nice side-effect. As such, tests serve as specifications; reading a test should give you a clear understanding of what the SUT does. However, reality is not always the case, so the more help you provide to your audience (including yourself), the better. Part of that resides in how you name your tests.

Action_Result_Condition

Let me first point out that I'm not too fond of method names having underscores, but I do find it no doubt easier to read. Which do you find easier?

GetCustomerShouldReturnActiveCustomer

or

GetCustomer_Should_Return_Active_Customer

Before continuing, let me also say that in the context of non-test methods, if you need to use underscores for naming methods, you're probably doing way too much in the method to begin with and it could probably be re-factored. However, tests are a special case.

A test validates certain functionality under specific conditions (or lack of them). By indicating this in the test name, it makes it not only easier to understand the specification, but to also validate the actual test. Take the following example:

   1: [TestMethod]

   2: public void ValidateGetActiveCustomer() {

   3:  

   4: }

without reading the code, you really don't know what that test is meant to do. All you know is that it validates a GetActiveCustomer. But under what conditions? Since the only reference you have is the code, if it's wrong, that is, the test is written incorrectly, you're pretty much lost. You're running that test and thinking all is ok when really it isn't.

Now let's take a look at the same test but with a different name

   1: [TestMethod]

   2: public void GetActiveCustomer_Returns_Customer_When_Given_Valid_Id() {

   3:  

   4: }

This clearly indicates the intentions of the test. At first sight, without even looking at the code, you know what the test is trying to accomplish, and thus providing you a basis by which to validate the actual test code.

You could say that the name is too long. So? You short on bytes? Or on display settings? You could also argue that the Returns_Active_Customer is redundant. Sure, it might be in this case, but that's because GetActiveCustomer more or less indicates what that particular action should do. But there are cases when this does not apply. Let's look at another example

   1: [TestMethod]        

   2: public void ValidateDeleteCustomer() {

   3:         

   4: }

ValidateDeleteCustomer, what does it do? When does it fail? When does it pass?

On the other hand, the following test is telling me not only under what conditions DeleteCustomer should work, but is also clearly indicating a requirement of the program: when a customer has no orders, you can delete it.

   1: [TestMethod]

   2: public void DeleteCustomer_Deletes_Customer_When_Customer_Has_No_Orders()

   3: {

   4:     

   5: }

Here's another example:

   1: [TestMethod]

   2: [ExpectedException(typeof(Exception))]

   3: public void DeleteCustomer_When_Customer_Has_Orders_Throws_Exception() {

   4:  

   5: }

   6:  

Again, as with GetActiveCustomer, we know that this test throws an exception by looking at the test attribute. However, this is something we don't see in a test view/run.

Irrelevant of whether you use underscores or not, use long names or short names, what is also very important is to establish a convention and stick to it. At the very least, all tests in a project should follow the same convention, and if you can get a consensus with your team across different projects, even better.