33

I hope this question gives some interesting answers because it's one that's bugged me for a while.

Is there any real value in unit testing a controller in ASP.NET MVC?

What I mean by that is, most of the time, (and I'm no genius), my controller methods are, even at their most complex something like this:

public ActionResult Create(MyModel model)
{
    // start error list
    var errors = new List<string>();

    // check model state based on data annotations
    if(ModelState.IsValid)
    {
        // call a service method
        if(this._myService.CreateNew(model, Request.UserHostAddress, ref errors))
        {
            // all is well, data is saved, 
            // so tell the user they are brilliant
            return View("_Success");
        }
    }

    // add errors to model state
    errors.ForEach(e => ModelState.AddModelError("", e));

    // return view
    return View(model);
}

Most of the heavy-lifting is done by either the MVC pipeline or my service library.

So maybe questions to ask might be:

  • what would be the value of unit testing this method?
  • would it not break on Request.UserHostAddress and ModelState with a NullReferenceException? Should I try to mock these?
  • if I refractor this method into a re-useable "helper" (which I probably should, considering how many times I do it!), would testing that even be worthwhile when all I'm really testing is mostly the "pipeline" which, presumably, has been tested to within an inch of it's life by Microsoft?

I think my point really is, doing the following seems utterly pointless and wrong

[TestMethod]
public void Test_Home_Index()
{
    var controller = new HomeController();
    var expected = "Index";
    var actual = ((ViewResult)controller.Index()).ViewName;
    Assert.AreEqual(expected, actual);
}

Obviously I'm being obtuse with this exaggeratedly pointless example, but does anybody have any wisdom to add here?

Looking forward to it... Thanks.

LiverpoolsNumber9
  • 788
  • 1
  • 6
  • 13
  • I think the RoI (Return on Investment) on that particular test is not worth the effort, unless you have infinite time and money. I would write tests that Kevin points out for checking things that are more likely to break or will help you in refactoring something with confidence or ensuring error propagation is happening as expected. Pipeline tests if needed can be done at a more global/infrastructure level and at individual methods level will be of little value. Not saying they are no value, but "little". So if it provides a good RoI in your case, go for it, else, catch the bigger fish first! – Mrchief Feb 05 '16 at 16:44

3 Answers3

18

Even for something so simple, a unit test will serve multiple purposes

  1. Confidence, what was written conforms to expected output. It may seem trivial to verify that it returns the correct view, but the result is objective evidence that the requirement was met
  2. Regression testing. Should the Create method need to change, you still have a unit test for the expected output. Yes, the output could change along and that results in a brittle test but it still is a check against un-managed change control

For that particular action I'd test for the following

  1. What happens if _myService is null?
  2. What happens if _myService.Create throws an Exception, does it throw specific ones to handle?
  3. Does a successful _myService.Create return the _Success view?
  4. Are errors propagated up to ModelState?

You pointed out checking Request and Model for NullReferenceException and I think the ModelState.IsValid will take care of handling NullReference for Model.

Mocking out the Request allows you to guard against a Null Request which is generally impossible in production I think, but can happen in a Unit Test. In an Integration Test it would allow you to provide different UserHostAddress values (A request is still user input as far as the control is concerned and should be tested accordingly)

Kevin
  • 798
  • 3
  • 7
3

My controllers are very small as well. Most of the "logic" in controllers is handled using filter attributes (built-in and hand-written). So my controller usually only has a handful of jobs:

  • Create models from HTTP query strings, form values, etc.
  • Perform some basic validation
  • Call into my data or business layer
  • Generate a ActionResult

Most of the model binding is done automatically by ASP.NET MVC. DataAnnotations handle most of the validation for me, too.

Even with so little to test, I still typically write them. Basically, I test that my repositories are called and that the correct ActionResult type is returned. I have a convenience method for ViewResult for making sure the right view path is returned and the view model looks the way I expect it to. I have another for checking the right controller/action is set for RedirectToActionResult. I have other tests for JsonResult, etc. etc.

An unfortunate result of sub-classing the Controller class is that it provides a lot of convenience methods that use the HttpContext internally. This makes it hard to unit test the controller. For this reason, I typically put HttpContext-dependent calls behind an interface and pass that interface to the controller's constructor (I use Ninject web extension to create my controllers for me). This interface is usually where I stick helper properties for accessing session, configuration settings, the IPrinciple and URL helpers.

This takes a lot of due diligence, but I think it is worth it.

Travis Parks
  • 2,495
  • 1
  • 19
  • 23
  • Thanks for taking the time to answer but 2 issues straight away. Firstly, "helper methods" in unit tests are v.dangerous. Secondly, "test that my repositories are called" - do you mean via dependency injection? – LiverpoolsNumber9 Mar 15 '13 at 17:12
  • Why would convenience methods be dangerous? I have a `BaseControllerTests` class where they all live. I mock out my repositories. I plug them in using Ninject. – Travis Parks Mar 15 '13 at 17:30
  • What happens if you've made an error or an incorrect assumption in your helper/s? My other point was that, only an integration test (ie end-to-end) could "test" whether your repositories are called. In a unit test you would "new-up" or mock your repositories manually anyway. – LiverpoolsNumber9 Mar 17 '13 at 21:07
  • You pass the repository to the constructor. You mock it during test. You make sure the mock is acted upon as expected. The helpers simply deconstruct `ActionResult`s to inspect passed URLs, models, etc. – Travis Parks Mar 18 '13 at 01:12
  • Ok fair enough - I slightly misunderstood what you meant by "test that my repositories are called". – LiverpoolsNumber9 Mar 18 '13 at 14:34
2

Obviously some controllers are much more complex than that but based purely on your example:

What happens if myService throws an exception?

As a side note.

Also, I'd question the wisdom of passing a list by reference (it's unnecessary since c# passes by reference anyway but even if it wasn't) - passing an errorAction action (Action) that the service can then use to pump error messages to which could then be handled however you want (maybe you want to add it to the list, maybe you want to add a model error, maybe you want to log it).

In your example:

instead of ref errors, do (string s) => ModelState.AddModelError("", s) for example.

Michael
  • 2,979
  • 19
  • 13
  • Worth mentioning, this does assume your service is residing in the same application otherwise serialization issues will come into play. – Michael Mar 15 '13 at 16:15
  • The service would be in a separate dll. But anyway, you're probably right re the "ref". On your other point, it doesn't matter if myService throws an exception. I'm not testing myService - I would test the methods therein separately. I'm talking about purely testing the ActionResult "unit" with (probably) a mocked myService. – LiverpoolsNumber9 Mar 15 '13 at 16:18
  • Do you have a 1:1 mapping between your service and your controller? If not, do some controllers use multiple service calls? If so, you could test those interactions? – Michael Mar 15 '13 at 16:26
  • No. At the end of the day, the service methods take input (usually a view model or even just strings / ints), they "do stuff", then return a bool / errors if false. There's no "direct" link between the controllers and the service layer. The are completely separated. – LiverpoolsNumber9 Mar 15 '13 at 16:30
  • Yes, I understand that, I'm trying to understand the relational model between the controllers and the service layer - assuming that each controller doesn't have a corresponding service method then it would stand to reason that some controllers might need to make use of more than one service method? – Michael Mar 15 '13 at 16:38
  • Yes. But even still, the point of a unit test is to test, given a certain input, what the output would be. I know the answer to that already because I will have already tested my service class methods. If you're going to test every "bit" of an individual method, you would have to test "native" MS methods too! I know what you're saying, but that isn't really my question. Or maybe it is?? – LiverpoolsNumber9 Mar 15 '13 at 16:42
  • Yes but if your controller calls into multiple service methods you haven't tested the interaction between those service methods for valid inputs to the controller methods. – Michael Mar 15 '13 at 16:45
  • Can you edit your answer to give an example?? – LiverpoolsNumber9 Mar 15 '13 at 16:50
  • @radarbob - I didn't realise ref prevented you from doing that ... – Michael Mar 28 '13 at 17:02
  • using the "ref" method parameter is not "stuttering" on the part of .NET. With it the called method can point your passed in reference to a different object. Without "ref" the method is getting a copy of your passed-in reference (pointing to that same object). Yes, our "standard" way of passing reference-types is actually "pass by value." – radarbob May 01 '13 at 17:37