Unit testing an existing ASP.NET MVC controller

Question

I've read more and more about unit testing, and determined to put it to work. I dug out a project which is written with ASP.NET MVC using the repository pattern, dependency injection and EF. My first task was to unit test a controller. Here is a snippet from the controller to test:

 IUserRepository _userRepository;
    IAttachmentRepository _attachmentRepository;
    IPeopleRepository _peopleRepository;
    ICountryRepository _countryRepository;

    public UserController(IUserRepository userRepo, IAttachmentRepository attachRepo, IPeopleRepository peopleRepo, ICountryRepository countryRepo)
    {
        _userRepository = userRepo;
        _attachmentRepository = attachRepo;
        _peopleRepository = peopleRepo;
        _countryRepository = countryRepo;
    }

    public ActionResult Details()
    {
        UserDetailsModel model = new UserDetailsModel();

        foreach (var doc in _attachmentRepository.GetPersonAttachments(Globals.UserID))
        {
            DocumentItemModel item = new DocumentItemModel();
            item.AttachmentID = doc.ID;
            item.DocumentIcon = AttachmentHelper.GetIconFromFileName(doc.StoragePath);
            item.DocumentName = doc.DocumentName;
            item.UploadedBy = string.Format("{0} {1}", doc.Forename, doc.Surname);
            item.Version = doc.VersionID;

            model.Documents.Add(item);
        }

        var person = _peopleRepository.GetPerson();
        var address = _peopleRepository.GetAddress();

        model.PersonModel.DateOfBirth = person.DateOfBirth;
        model.PersonModel.Forename = person.Forename;
        model.PersonModel.Surname = person.Surname;
        model.PersonModel.Title = person.Title;

        model.AddressModel.AddressLine1 = address.AddressLine1;
        model.AddressModel.AddressLine2 = address.AddressLine2;
        model.AddressModel.City = address.City;
        model.AddressModel.County = address.County;
        model.AddressModel.Postcode = address.Postcode;
        model.AddressModel.Telephone = address.Telephone;

        model.DocumentModel.EntityType = 1;
        model.DocumentModel.ID = Globals.UserID;
        model.DocumentModel.NewFile = true;

        var countries = _countryRepository.GetCountries();

        model.AddressModel.Countries = countries.ToSelectListItem(1, c => c.ID, c => c.CountryName, c => c.CountryName, c => c.ID.ToString());

        return View(model);
    }

I want to test the Details method and have the following queries:

1) The Globals.UserID property retrieves the current user from the session object. How can I easily test this (I'm using the built in VS2010 unit testing and Moq)

2) I'm making a call to AttachmentHelper.GetIconFromFileName() here which simply looks at the extension of a file and displays an icon. I'm also making a call to GetPersonAttachments in the attachment repository, a call to GetPerson, GetAddress and GetCountries as well as a call to an extension method created transform a List to IEnumerable of SelectListItem.

Is this controller action an example of bad practice? It's using lots of repositories and also other helper methods. From what I can see, unit testing this single action will require lots and lots of code. Is this counter productive?

Unit testing a simple controller in a test project is one thing, but when you get into real life code such as this it could become a monster.

I guess my question really is should I refactor my code to make it easier to test, or should my tests become much more complex to satisfy the current code?

Answer 1

As already mentioned by @KevinM1, if you're practicing TDD (you have that tag in your question), you're writing the test before the implementation.

You first write a test for your controller's Detail method. When you write this test you notice that you need to map a person to a UserDetailsModel. When writing test you "hide complexity" that don't belong to the actual implementation of what you want to test behind an abstraction. In this case, you'd probably create an IUserDetailModelMapper. When this first test is written, you make it green by creating the controller.

public class UserController
{
   ctor(IUserRepository userRepo, IUserDetailModelMapper mapper){...}

   public ActionResult Details()
   {
      var model = _mapper.Map(_userRepo.GetPerson());
      return View(model);
   }
}

When you later write test for your mapper, you said you need to use some static prop called Globals.UserId. Generally I would avoid static data if possible, but if this is a legacy system you need to "objectify" this to get i testable. One simple way is to hide it behind an interface, something like this...

interface IGlobalUserId
{
  int GetIt();
}

...and do an implementation where you use your static data. From now on, you can inject this interface instead to hide the fact that it's static data.

The same thing goes for "AttachmentHelper". Hide it behind an interface. Generally though, there should be alarm bells for XXXHelpers - I would say it's a sign of not placing method where they should be (part of an object) but rather a mix of all kinds of stuff that been mixed together.

Answer 2

I'd move all of your Model construction code into the constructor of the model itself. I prefer to keep the controllers restricted to a very few simple tasks:

• Selecting the proper view (if the controller action allows for more than one view)
• Selecting the correct view model
• Permissions/security
• View model validation

Thus, your Details controller becomes much simpler and testing becomes more more managable:

public ActionResult Details() {
    return View(new UserDetailsModel(Globals.UserId);
}

Now that your controller is tight and testable, let's look at your model:

    public class UserDetailsModel {
        public UserDetailsModel(int userId) {
           ... instantiation of properties goes here...
         }

        ... public properties/methods ...
    }

Again, the code in your model is encapsulated and only needs to worry specifically about it's properties.

Answer 3

Complex tests are as bad as complex code: they're prone to bugs. So, in order to keep your tests simple it's generally a good idea to refactor your application code to make it easier to test. For example, you should pull out the mapping code from your Details() method into separate helper methods. You can then test those methods very easily and not have to worry so much about testing all the crazy combinations of Details().

I've pulled out the person and address mapping parts below, but you could pull it apart even more. I just wanted to give you an idea of what I meant.

    public ActionResult Details() {
        UserDetailsModel model = new UserDetailsModel();

        foreach( var doc in _attachmentRepository.GetPersonAttachments( Globals.UserID ) ) {
            DocumentItemModel item = new DocumentItemModel();
            item.AttachmentID = doc.ID;
            item.DocumentIcon = AttachmentHelper.GetIconFromFileName( doc.StoragePath );
            item.DocumentName = doc.DocumentName;
            item.UploadedBy = string.Format( "{0} {1}", doc.Forename, doc.Surname );
            item.Version = doc.VersionID;

            model.Documents.Add( item );
        }

        var person = _peopleRepository.GetPerson();
        var address = _peopleRepository.GetAddress();

        MapPersonToModel( model, person );

        MapAddressToModel( model, address );

        model.DocumentModel.EntityType = 1;
        model.DocumentModel.ID = Globals.UserID;
        model.DocumentModel.NewFile = true;

        var countries = _countryRepository.GetCountries();

        model.AddressModel.Countries = countries.ToSelectListItem( 1, c => c.ID, c => c.CountryName, c => c.CountryName, c => c.ID.ToString() );

        return View( model );
    }

    public void MapAddressToModel( UserDetailsModel model, Address address ) {
        model.AddressModel.AddressLine1 = address.AddressLine1;
        model.AddressModel.AddressLine2 = address.AddressLine2;
        model.AddressModel.City = address.City;
        model.AddressModel.County = address.County;
        model.AddressModel.Postcode = address.Postcode;
        model.AddressModel.Telephone = address.Telephone;
    }

    public void MapPersonToModel( UserDetailsModel model, Person person ) {
        model.PersonModel.DateOfBirth = person.DateOfBirth;
        model.PersonModel.Forename = person.Forename;
        model.PersonModel.Surname = person.Surname;
        model.PersonModel.Title = person.Title;
    }

Answer 4

Just wanted to elaborate a little bit on the subj. What we're trying to unit test is the logic. Not too much of it in the controller. So in this particular case I would do the next: extract method that returns the model not the view. Inject mocked repos into the controller object. And after exercising the mapping would insure that all the properties are filled with expected values. Another way of doing that is to generate JSON and insure that all the properties are filled appropriately. However, I would strive to put unit tests on the mapping part itself and then would consider BDD for the integration tests.