8
\$\begingroup\$

Suppose the following example unit test for an ASP.NET MVC controller:

[Test]
public void Delete_Will_Redirect_To_Index_1()
{
    bookRepository.Add(someBook);
    var result = booksController.Delete(someBook.Id) as RedirectToRouteResult;
    Assert.That(result, Is.Not.Null);
    Assert.That(result.RouteValues["Action"], Is.EqualTo("Index"));
}

Resharper unwittingly complains on the fourth line of code that result.RouteValues may cause a possible NullReferenceException. I hate those squiggly lines, so I figured I could just as easily write it as follows:

[Test]
public void Delete_Will_Redirect_To_Index_2()
{
    bookRepository.Add(someBook);
    var result = (RedirectToRouteResult)booksController.Delete(someBook.Id);
    Assert.That(result.RouteValues["Action"], Is.EqualTo("Index"));
}

This has some advantages:

  • One LoC less;
  • More to the point;

It also has one main disadvantage:

  • Code will fail with a cast exception if the result is of unexpected type. This seems(?) less clear than a failed assert.

Some other alternatives I've considered include :

[Test]
public void Delete_Will_Redirect_To_Index_3()
{
    bookRepository.Add(someBook);
    var result = booksController.Delete(someBook.Id);
    Assert.That(result, Is.InstanceOf<RedirectToRouteResult>());
    var redirectResult = (RedirectToRouteResult)result;
    Assert.That(redirectResult.RouteValues["Action"], Is.EqualTo("Index"));
}

This is five lines of code, but has two very clear asserts.

I've even considered this version:

[Test]
public void Delete_Will_Redirect()
{
    bookRepository.Add(someBook);
    var result = booksController.Delete(someBook.Id);
    Assert.That(result, Is.InstanceOf<RedirectToRouteResult>());
}

[Test]
public void Delete_Will_Redirect_To_Index_4()
{
    bookRepository.Add(someBook);
    var redirectResult = (RedirectToRouteResult)booksController.Delete(someBook.Id);
    Assert.That(redirectResult.RouteValues["Action"], Is.EqualTo("Index"));
}

All in all I see the following points in play to judge the various forms:

  • number of lines of code;
  • readability of the code;
  • clarity of failing-unit-test feedback;
  • how pragmatic and painless it is to write a test;

Did I miss any important points? Did I miss a scenario that has the best of all points? What can I do to get the best of all scenarios?

PS. I'd prefer a framework-agnostic answer, but if it matters, I'm using C# NUnit tests for an ASP.NET MVC5 controller.

\$\endgroup\$
8
\$\begingroup\$

You could make a strong argument that you need 2 tests.

General theory has a unit test (as opposed to an integration test) containing only one assert, see e.g. Programmers.StackExchange

So you probably want:

[Test]
public void Delete_WithValidBook_ReturnsRedirectToRouteResult()
{
    bookRepository.Add(someBook);
    var result = booksController.Delete(someBook.Id);
    Assert.That(result.Type, Is.EqualTo(typeof(RedirectToRouteResult)));
}

[Test]
public void Delete_WithValidBook_RedirectsToValidIndex()
{
    bookRepository.Add(someBook);
    var result = (RedirectToRouteResult)booksController.Delete(someBook.Id);
    Assert.That(result.RouteValues["Action"], Is.EqualTo("Index"));
}

This has the added advantage that it is much easier to find the bug later based on the name of the failed test alone.

For example, if your type test fails, presumably your redirect test will also fail, but I know which I'd look at first during debugging!

If, on the other hand, the Delete method is guaranteed (by the framework) to return a RedirectToRouteResult, then you only want one test:

[Test]
public void Delete_WithValidBook_RedirectsToValidIndex()
{
    bookRepository.Add(someBook);
    var result = (RedirectToRouteResult)booksController.Delete(someBook.Id);
    Assert.That(result.RouteValues["Action"], Is.EqualTo("Index"));
}

If you have multiple tests, or a guard assertion, the reader (i.e. you at some point in the future) will assume that it must be possible to get, say, a null result, which will mean you may feel the need to add null checks, etc, to other parts of the code that may not be required.

\$\endgroup\$
  • \$\begingroup\$ Thanks for your answer, I was leaning towards this as well, even though it is the most work (and: is the tradeoff worth it?). One question though: the linked Programmers.SE topic has a second comment on Guard Assertions, isn't my case very similar (though probably not the same), thus being an exception to the rule? \$\endgroup\$ – Jeroen Apr 9 '14 at 5:58
  • \$\begingroup\$ @Jeroen It depends, I'm not super familiar with asp, but you certainly shouldn't be unit testing the compiler. I.e. if the compiler/framework guarantees the result type, the heuristic I apply is not to test it... \$\endgroup\$ – Byron Ross Apr 9 '14 at 10:47
  • \$\begingroup\$ Hmm? I fail to see how this relates to "testing the compiler" (which indeed would be unnecessary). What I meant was, my version Delete_Will_Redirect_To_Index_3 has two asserts, but the first assert is very much like a Guard Assertion from mentioned article, in which case the article you linked to would condone having it as a second assert in a single test. \$\endgroup\$ – Jeroen Apr 9 '14 at 11:57
  • 1
    \$\begingroup\$ @Jeroen updated the answer. \$\endgroup\$ – Byron Ross Apr 9 '14 at 21:55
6
\$\begingroup\$

I totally agree with @ByronRoss' answer, you need 2 tests here.

I'll address another aspect here: naming.

public void Delete_Will_Redirect()
public void Delete_Will_Redirect_To_Index()

Test methods are methods, test classes are classes. I'm surprised ReSharper isn't complaining about all these underscores, it normally enforces PascalCase naming for public methods (I run it with default settings - indeed, no complain with the underscores).

I would expect the method names in the rest of your code to look like this:

public void DeleteWillRedirect()
public void DeleteWillRedirectToIndex()

I don't see a reason for test methods to follow their own naming convention1.


Edit

I think a compromise could be a standard pattern like [TestedMethod_DescriptiveOutcome], which would make the method names look like this:

public void Delete_WillRedirect()
public void Delete_WillRedirectToIndex()

I find it gives the underscore a stronger meaning (delimiting between the tested method and the actual expected outcome) than just having it everywhere - that's just my opinion though.


1 of course that doesn't hold if that naming convention is consistent across the entire code base.

\$\endgroup\$
  • 1
    \$\begingroup\$ Hey Mat, thanks for your review! Resharper was indeed complaining, and after some pondering I've purposely added an exception for my unit test method names. I prefer having rather verbose test method names that read like a (business) requirement, and so my method names can become rather long (longer than mentioned example). I find that the underscores here are worth it, improving readability and giving me instant clarity about the problem when looking at test reports / test runners. \$\endgroup\$ – Jeroen Apr 10 '14 at 5:39
  • 1
    \$\begingroup\$ The use of underscores in naming test classes and methods is the standard way of doing it. It is more readable with longer more descriptive names usually associated with tests. Because tests should be describing behavior, rather than simply "being a thing". I see no reason not to differentiate betwen your tests and code in a visual way like this either, as can help with the context switching. \$\endgroup\$ – Mitchell Lee Apr 10 '14 at 12:19
  • \$\begingroup\$ see also osherove.com/blog/2005/4/3/naming-standards-for-unit-tests.html \$\endgroup\$ – Byron Ross Apr 29 '14 at 11:17
  • \$\begingroup\$ It looks like newer versions of ReSharper no longer complain about test names. I don't actually recall it ever behaving like that :/ \$\endgroup\$ – Byron Ross Nov 20 '14 at 21:08
4
\$\begingroup\$

Honestly, I would say there is nothing wrong with your original answer. It's very descriptive, and easy to read. I don't see any business value for creating two tests just to check "instanceOf". The cast method also works as a null reference in a 3 line test method with only one cast is pretty obvious what's failing (even if the error message isn't "should not be null")

Resharper is a great tool for many things, but it's not always right. You should never blindly refactor code to appease your tools unless you understand why you are doing so. In this case resharper is trying to warn you that you are doing a cast, and using a value without checking for null in the "traditional" sense. You are however, checking for null with an assertion, and as such resharpers concern is not applicable in this situation.

If you want the squiggly to go away, you can surpress the message in resharper for this particular case.

\$\endgroup\$
  • \$\begingroup\$ +1, this was one of my thoughts too. On a side note, of course I realize Resharper is just a tool, not The Law. My hate for squiggly lines was meant to come across with a ;-) \$\endgroup\$ – Jeroen Apr 10 '14 at 12:28
  • 1
    \$\begingroup\$ Thank you for backing your downvote with an additional answer (on top of a comment), this (having more than just 1 answer on that question and encouraging more point of views [points of view?] to be brought up) is exactly why I posted mine in the first place. I'm currently out of ammo, but I'll make sure to come back here when votes reload, and upvote this answer - I completely agree with ReSharper suggestions being nothing more than suggestions. \$\endgroup\$ – Mathieu Guindon Apr 10 '14 at 14:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.