The xUnit team removed my favourite feature… and made me a convert.

So I was helping one of the senior devs here with a code review – let’s call him Pete. Because his name is Pete, so that’ll make things easier.
Anyway Pete asked me for a second opinion on a couple of tests checking for the existence and non-existence of exceptions.

This seemingly ultra-specific topic actually revolved around a few super important general principles – like readability; semantics; and Arrange, Act, Assert.
Its generally accepted that if you ignore these principles your test code quickly turns claggy and unmaintainable.

I was writing up our discussion when I discovered that the xUnit team had removed support for my-favorite-assertion-you-never-heard-of. I’m annoyed of course… but not because they removed it. I’ve been advocating NUnit at my work for some time; we’ve been converting MSTest projects over to it because it’s more expressive.
All of that hard work may need to be redone – because those clever bastards at xUnit had such a damn good reason for doing it they’ve only gone and made me a convert.

I’ll get around to explaining that, but it doesn’t make sense without comparing the tools each framework provides, so bear with me.

Do you know how to test for exceptions in an elegant and expressive way?
Let’s look at some code and see if your implementation comes up.

[TestFixture]
public class When_validating_an_email
{
    [Test]
    public void with_an_invalid_email_it_should_throw()
    {
        var sut = new EmailValidator();
        
        try
        {
            sut.ValidateEmail("foo.bar.com");
            Assert.Fail("it should throw");
        }
        catch (EmailInvalidException e)
        {
        }
    }
}

The test is easy to understand and the naming while not my style is clear enough… but is everything about the test optimal or do we have choices here?

Of course we do.

Our SUT is a validator that is supposed to throw an exception when it is given an invalid email.

If the SUT throws the wrong type of exception, it will not be caught and will bubble to the test runner which will fail the test.
If the SUT fails to throw an exception, execution will continue to the “Assert.Fail” line and fail there.
And finally, if the SUT behaves as expected, then the “Assert.Fail” will be skipped, and we’ll end up swallowing the exception in the catch block.

Actually that’s pretty complicated when you think about it. That’s three very plausible execution paths, and our happy case involves skipping code (the fail statement).

Let’s jump straight to the next option, the most common way I’ve seen this solved.

    [Test, ExpectedException(typeof(EmailInvalidException))]
    public void with_an_invalid_email_it_should_throw()
    {
        var sut = new EmailValidator();
        sut.ValidateEmail("foo.bar.com");
    }

Well we’ve saved a lot of lines of code.

This implementation is courtesy of MsTest and NUnit’s “ExpectedException” attribute.

Unfortunately, most real world test implementations will be a little longer than this test, and the one thing this test doesn’t do is care _which line_ threw the exception… So for example if the constructor throws (which would lead to a very confusing bug) the test actually passes.

A less important, but still valid concern is that the assertion for the test has floated up and away from the test body to exactly where we don’t expect it to be. I like to understand tests within just a few seconds; this layout gives me eye strain. Actually the xUnit team is quite nicely opinionated on this point, and have explicitly chosen not to implement ExpectedException – (but this was never my favorite-assertion-you-never-heard-of – you’ll have to keep reading for that.)

We can do better.

    [Test]
    public void with_an_invalid_email_it_should_throw()
    {
        var sut = new EmailValidator();
        Assert.Throws<EmailInvalidException>(
            () => sut.ValidateEmail("foo.bar.com") );
    }

We have localised the exception to the method we are testing, we have the exact type of exception constrained, and our assertion is sitting at the end of the test where we expect it to be.

I’m not the only one who prefers it. Programmers frustrated with MsTest’s lack of support for this feature have implemented it themselves.

This method is supported by NUnit and xUnit.

As a final note on “Assert.Throws”, take a look at the first test’s ugly brother.

    [Test]
    public void 
with_an_invalid_email_it_should_throw_an_exception_containing_the_email()
    {
        string result = null;
        
        var sut = new EmailValidator();
        
        try
        {
            sut.ValidateEmail("foo.bar.com");
        }
        catch (EmailInvalidException e)
        {
            result = e.Email;
        }
        
        Assert.AreEqual("foo.bar.com", result);
    }

Yuck. {Code blocks} in a test method are a smell that something has gone wrong.

How about this.

    [Test]
    public void 
with_an_invalid_email_it_should_throw_an_exception_containing_the_email()
    {
        var sut = new EmailValidator();
        
        var e = Assert.Throws<EmailInvalidException>(
            () => sut.ValidateEmail("foo.bar.com") );
            
        Assert.AreEqual("foo.bar.com", e.Email);
    }

This is how I currently code this scenario.
xUnit offers an alternative, but we’ll get back to that when we’ve looked at how we can test for no exception.

Every sut that you test throws on invalid input must have a corresponding test for valid input, right?

Let’s see some ways to do that, starting with the one Pete showed me.

    [Test]
    public void with_an_valid_email_it_should_succeed()
    {
        var sut = new EmailValidator();
        
        try
        {
            sut.ValidateEmail("foo@bar.com");
        }
        catch (EmailInvalidException e)
        {
            Assert.Fail("it should not throw");
        }
    }

This is quite interesting, because the author has done something strictly unnecessary in the noble pursuit of having an expressive test.

Compare with this:

    [Test]
    public void with_an_valid_email_it_should_succeed()
    {
        var sut = new EmailValidator();
        sut.ValidateEmail("foo@bar.com");
    }

These tests pass and fail identically… but the first test has code documenting the idea that we anticipate that an exception could happen and we don’t want it to. This idea of documenting that an exception could happen is a good one, but not done very succinctly in the first example and not done at all in the second example.

In fact, if you read the second test, would you even know that the pass condition was “no exception thrown?” There is no assertion in the test, so it looks like the author forgot to write one – perhaps they were interrupted. One could argue the test isn’t doing anything and has no value, and remove the test – accidentally removing an important test case in some code that uses exceptions.

Minor improvement:

    [Test]
    public void with_an_valid_email_it_should_succeed()
    {
        var sut = new EmailValidator();
        sut.ValidateEmail("foo@bar.com");
        Assert.Pass("No exception thrown.");
    }

Now we can tell that we are testing for an exception. Assert.Pass and Assert.Fail are important to know about – but unless you have exceptions with try-catch (which can cause code to be skipped) or control statements in your test you shouldn’t need them for a finished test. I often use Assert.Fail to stub out a failing test just before I am forced to context switch, to remind me what I had to do next.

Let’s get rid of that Assert.Pass. Again NUnit offers an assertion that takes an Action.

    [Test]
    public void with_an_valid_email_it_should_succeed()
    {
        var sut = new EmailValidator();
        Assert.DoesNotThrow(
            () => sut.ValidateEmail("foo@bar.com"));
    }

It’s pretty readable right? This is my favorite-assertion-you-never-heard-of! Like Assert.Throws, it localises the thrown exception to one line – now future readers of the test know the intent and have the idea that this method may throw.

Supposing an exception is raised outside of this statement? Well the NUnit will give different diagnostic feedback about an unhandled exception, rather than about a failed assertion.

This finishes the story for NUnit… So far we’ve seen NUnit consistently provide more expressive options for exception testing than MSTest; and xUnit has seemed great too… but as I already hinted, they removed my favourite assertion.

Assert.DoesNotThrow has been deprecated by the xUnit team! It’s not making the cut for version 2.0!

Surely they can’t be advocating the Assert.Pass version?

I was googling for DoesNotThrow to research opinions and usage when I discovered the offending feature request on github.

Remove Assert.DoesNotThrow #188. BradWilson: So. Incredibly. Not. Needed.
Source

BradWilson: Every single line of code is an implicit

I hope I’ve explained why I disagree with that. Localisation of the exception, readability, and having an assertion in each test are more than reason enough.

Mark Seemann was the first to put the case for the defence. I’m glad someone voiced my views.

Ploeh: I, for one, liked the explicitness of being able to state that this particular line of code shouldn't throw, and that's the test.

If that was it, I would have decided it was a clear cut case – they were just wrong.

But there’s an idealogical issue at stake here. See, the test has a slightly twisted structure. We expect

  • Arrange
  • Act
  • Assert

And what we have instead is

  • Arrange
  • Assert(Act)

Note that Assert.Throws has the same problem. This seems to me to be a minor issue… if the test remains readable. But it is interesting that the xUnit team have taken such a strong opinionated stance.

Adam Ralph made a stronger argument for an alternative.

Personally, I believe neither Assert.Throws<T> nor Assert.DoesNotThrow<T> should exist.  (...you can use Record.Exception instead and preserve the structure of Arrange, Act, Assert)

I would never have thought of writing a helper like “Record.Exception” because it violates a different guideline I hold close to heart – “don’t return null” (because you force the caller to check for it). But the means justify the end in this case – which is to preserve the integrity of Arrange Act Assert.

The discussion on the xUnit project board is inspirational – because people passionate about testing are discussing and discovering best practices and building a framework that is designed around those ideas.

In the end your choice will be a matter of taste. But please remember you have more concise and expressive options than try-catching the exceptions yourself.

4 thoughts on “The xUnit team removed my favourite feature… and made me a convert.

  1. Sean

    my favorite way of expressing this code these days is to use the FluentAssertions library. With that, my code becomes:


    var sut = new MyObject();

    Action act = () => sut.DoSomething(null);

    act.ShouldThrow().And.ParamName.Should().Be("myParam");

    When this test fails, I also get really nice failure messages. It helps me keep things clear and concise and I’ve never had tests feel so readable. I wish the “should” phrasing was more assertive but it’s a trade-off I happily accept.

    Reply
    1. taharrison Post author

      I’ve not used FluentAssertions, but will take a look – thanks for the heads up.

      The issue I’ve always had with various other Fluent libraries is that they may read a bit like English, but they don’t obey the principle of least surprise – in order to chain words together to form sentence structures you have to work with methods and properties that return builders. It is hard to see exactly what work will be done and where.

      And if the library doesn’t quite support the exact kind of assertion you want to make, figuring out how to extend the library, or having a frankenstein test with a mix of fluent and non fluent logic is quite a burden.

      I feel like it is easier to communicate intent to my fellow coders by using natural familiar expressive code and agressively refactoring repeated code into test helpers named descriptively.

      I’ll follow up your recommendation and try look at what FA has to offer with an open mind. If you have any preferred resources / blogs, please link me. 🙂

      Reply
  2. Asier

    Hi Tom,
    Good blogpost.
    I agree with you that checking for exceptions in the attribute is not readable and also doesn’t constrain the assertion on a test just to a single line.

    On the other hand, IMHO using Assert.DoesNotThrow is a bad practice. If your test just checks for no exceptions being thrown or doesn’t do any assertion at all it could be interpreted as a code smell.
    In your case for example.. It would be better if the EmailValidator has a a method called “IsValid(email)” which returns true and false rather than throwing an exception. The client of that class will just check for the output of that method instead of having to catch an exception. Exceptions are side effects and should be used just in exceptional cases. I don’t know what is your application really trying to do, but I would say that email validation failure is not an exceptional case.
    Worth checking this article from Martin Folwer too: http://martinfowler.com/articles/replaceThrowWithNotification.html#WhenToUseThisRefactoring

    Reply
    1. taharrison Post author

      Absolutely agree 100% that exception throwing is a poor pattern for validation.

      I just wanted to create a really simple SUT to discuss testing styles around – in this case the simplest example “my validator throws on invalid input” is not the best practical design. I’ve also read comments in the xUnit thread about DoesNotThrow and why they removed it. One comment read (paraphrasing): “Whenever we encounter the assertion ‘DoesNotThrow’ we usually discover that something else in the design is wrong – if your code has no other side effects than the (non)presence of an exception, how do you know it’s done any work at all?”

      That Fowler article is excellent, thanks for the link.
      BTW I like the look of your blog.

      Reply

Leave a Reply

Your email address will not be published. Required fields are marked *