When you encounter a bug, do you first write a failing test before fixing it? You should. But the vast majority of us sometimes do not. External pressure (read Customer, Teammates or Management) move us in the opposite direction. Everything we’ve learnt about unit testing and good software practices go out the window, and the sheer pressure forces us to fix the issue as fast as possible! We focus more on getting the job done* and deploying than worrying about adding another test to our suite.

Adding that test is not about increasing our code coverage or patting ourselves on the back, with an optional tweet, that we are good developers. It’s about putting regression tests in place.

Of course, we could always just apply the fix, and then later, calmly add a test. However, there’s always another urgent bug fix, feature, or customer breathing down our necks, so things just get putt off. 

It’s hard to not be tempted to just fix something then and there. Fight the temptation though if you can. Write that failing test first!

[*Implicitly changing the definition of Done on the go]

A question popped up today about someone having trouble mocking the UserAgent property of a Controller. There's enough information on the Internet with solutions to this problem, so I'm going to describe how to figure it out if you don't have an Internet connection.

Let's assume we need to test that our Index action in our HomeController displays the UserAgent information:

   1: public ActionResult Index() 

   2: {

   3:     // Don't use ViewData at home kids! This is just a demo

   4:     ViewData["UserAgent"] = Request.UserAgent;

   5:  

   6:     return View();

   7: }

Our test would look something like:

   1: [TestMethod]

   2: public void Index() 

   3: {

   4:  

   5:     HomeController controller = new HomeController();

   6:  

   7:     ViewResult result = controller.Index() as ViewResult;

   8:  

   9:     ViewDataDictionary viewData = result.ViewData;

  10:     Assert.AreEqual("Mozilla/5.0 ....", viewData["UserAgent"]);

  11: }

If we run this, it will fail with an exception, reason being that Request is null in our test. This makes sense because we're not coming in from a browser, we're just calling a method on a class. Therefore, what we need to do is somehow inject a fake Request in there with some value assigned to the UserAgent. The problem is how....

Reflector is your friend 

Now, ASP.NET MVC comes with source code that you can download from Codeplex. Having the source code makes it easy to figure out how things work. But we're assuming that you don't have an Internet connection remember? And you forgot to download the source code. So let's fire up Reflector and see what it shows us.

We open up Reflector and load System.Web.Mvc, and locate the Controller class. Since we're trying to access the Request property of the Controller, that's the first thing we want to look up:

Hmm, it looks like the Request property is really coming from the HttpContext property. Clicking on that gives us:

We see that the HttpContext is being obtained from the ControllerContext. So let's continue to drill down to ControllerContext:

Aha! Looks like we've reached the end. ControllerContext is a read-write property. This means that if we provide a fake ControllerContext in our test, one that contains a fake HttpContext which in turn contains a fake Request with our UserAgent, we're all set.

Working our way backwards

I'm going to use Moq as my mocking framework, but you can use any other framework, such as RhinoMocks.  Instead of trying to declare a whole bunch of mocks first, in a true TDD fashion let's work our way backwards. First thing we need is a mock ControllerContext. In Moq, the actual mocked object is accessed via the Object property (I'm using screenshots so you can better contemplate the process):

As we can see, we've added a controllerContext mock object and so the next step is to create this object:

Now, our controllerContext needs the HttpContext property set:

The lines boxed in red are what we've added new. Now that we have our mocked ControllerContext and our HttpContext, the only thing left is to setup the UserAgent property of our Request. But to do that, we need another mock, the HttpRequest object:

We therefore create the new mock object and setup the UserAgent property:

Our complete test now looks like this:

And running it should give us a successful pass!

Refactoring

Now that we've understood exactly what's required, let's see if we can clean up the test a bit:

See what's happened? We've removed the mock objects for both the HttpRequest and the HttpContext. The reason we can do this is because of how Moq works, creating proxies for reference types. Therefore, HttpContext is actually not null, and nor is Request even though we haven't specifically created a mock for them. This allows us to easily access the UserAgent property and set it to the value required. Saves quite a bit of code

Summary

Mocking can sometimes seem complicated, not knowing what you're meant to do to get something to work. But all you need to do is take a step back, analyze what is it you're trying to mock and work your way up. Having tools like Reflector make this easier!

I'm coming across a lot of tests with names like this:

AddEmployee_Should_Pass_When_Not_Duplicate() {.....}

AddEmployee_Should_Fail_When_Duplicate() {....}

For the purpose of this post, I don't care about anything other than what's in bold and red.

The word Pass in there is completely irrelevant and useless. The word Fail is just mind-blowing. What should fail? The test? What if the test passes? Does that mean it has really failed? Or when it fails it means it passes?

Now we all know what Fail means here. It means the test will pass but the call will fail. But it doesn't indicate to us what failure consists of and therefore it's not adding information. Someone looking at the test cannot validate it.

When naming, indicate what the test is trying to validate, not what the outcome is; the outcome is already known: pass or failure.

Something along the lines of Add_Employee_Should_Throw_Exception_When_Employee_Already_Exists would add substantial information for the person reading your test to validate the functionality. But more importantly, when the test fails, they would know what has failed and have valuable information for debugging. 

Two teams work on two different components of a project. Team A works on component A, and to keep things simple, Team B works on component B. The system needs both components to function.

There's an additional catch. Team A needs component B to test A. Team B needs component A to test B.

So what to do? Well they would do what any good team would. Team A goes on holiday while Team B finishes and then Team B relaxes. Unfortunately that isn't always possible. 

Instead, they would use mocks, whether they create their own stubs/fakes or use an existing framework. To verify that component A works, Team A uses mocks to replace component B's functionality. Team B uses mocks to replace component A's functionality. All tests pass.

The next step would be to integrate the two subsystem and put it in production. I mean surely if all subsystem tests pass, it should work......the reality is it probably won't.

The moral of the story: Two halves don't necessarily make a whole. Don't skip your integration tests. It's not a waste of time. And integration tests aren't just about running all your unit tests on a continuous integration server. They are integration tests, not isolated unit tests.

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.GetAllEmployeesByCompanyId(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.

I'm testing out a new IoC container framework (more on that later), and so I decided to setup some tests to see how it works. As a "newbie" to MSTest (MbUnit / xUnit rules!), I wrote my tests, tried to run them and was presented with an error message:
Failed to queue test run 'Hadi Hariri 2008-07-08 10:06:31': Test Run deployment issue: The location of the file or directory 'd:\.....\someAssembly.dll' is not trusted.
Now D is my local drive, so go figure why suddenly an assembly I reference on my local disk is not trusted. Wel lit turns out that Vista, in all it's glory (I have a love-hate relationship with Vista, I love hating it all the time), decided to block this assembly and all other files I extracted from a zip file I downloaded.
So if it happens to you, go to the assembly and unblock it. And of course, this is not something you can do by selecting multiple files at once. No, that would be WAY too dangerous! You need to do it one by one. Alternatively, delete the whole folder, unblock the zip file and then extract it.