danielwertheim

danielwertheim


notes from a passionate developer

Developer that lives by the mantra "code is meant to be shared".

Share


Tags


Disclaimer

This is a personal blog. The opinions expressed here represent my own and not those of my employer, nor current or previous. All content is published "as is", without warranty of any kind and I don't take any responsibility and can't be liable for any claims, damages or other liabilities that might be caused by the content.

Why the ExpectedExceptionAttribute sucks!

Daniel WertheimDaniel Wertheim

Today I was reading a book which showed an example of an unit test. And even if it was just there to show unit testing as a concept when doing ASP.Net MVC development, I really think you never, ever, ever, ever, ever.....(n)....ever should use that ugly attribute. Why, well follow along.

BTW, the example tests below is not as they were in the book and they are just created for showing you the fragility of ExpectedExceptionAttribute

Test 1: Place a bid When no bids exists Then the bid gets added

You should be able to place a bid when no other bid exists, then there should be one bid registered and that is the one you placed.

The Test

[Test]
public void WhenNoBidsExists_BidIsAdded()  
{
    const int artefactId = 1;
    const decimal bid = 100m;

    var auction = new Auction();
    auction.PlaceBid(artefactId, bid);

    var bids = auction.GetBids(artefactId);
    Assert.AreEqual(bid, bids.Single());
}

The Implementation

public class Auction  
{
    private readonly Dictionary<int, List<decimal>> _bids
         = new Dictionary<int, List<decimal>>();

    public void PlaceBid(int artefactId, decimal bid)
    {
        _bids.Add(artefactId, new List<decimal> { bid });
    }

    public IEnumerable<decimal> GetBids(int artefactId)
    {
        if (!_bids.ContainsKey(artefactId))
            return Enumerable.Empty<decimal>();

        return _bids[artefactId];
    }
}

The Result

------ Test started: Assembly: ClassLibrary1.dll ------

1 passed, 0 failed, 0 skipped, took 0,14 seconds (NUnit 2.5.5).  

OK, nothing complicated here, lets move on to the next test.

Test 2: Place a bid When a higher bid exists Then it throws an InvalidOperationException

[Test]
[ExpectedException(typeof(InvalidOperationException))]
public void WhenHigherBidExists_ThrowsInvalidOperationException()  
{
    const int artefactId = 1;
    var auction = new Auction();

    auction.PlaceBid(artefactId, 10m);
    auction.PlaceBid(artefactId, 5m);
}

The Implementation

public void PlaceBid(int artefactId, decimal bid)  
{
    var hasExistingExceedingBid = GetBids(artefactId)
        .Any(existingBid => existingBid >= bid);

    if (hasExistingExceedingBid)
        throw new InvalidOperationException(
            "The bid is to low. Existing bids with same or higher value exists.");

    _bids.Add(artefactId, new List<decimal> { bid });
}

The Result

------ Test started: Assembly: ClassLibrary1.dll ------

2 passed, 0 failed, 0 skipped, took 0,14 seconds (NUnit 2.5.5).  

OK, everything works and the ExpectedException is thrown and the second test passes. Lets continue and add yet another test to meet a new requirement.

Test 3: Place a bid When the bid is lower than five Then it throws an InvalidOperationException

Just by reading the test spec there should be an alarm bell going of here. I will give you a small hint: ... Then it throws an InvalidOperationException. Two tests for different requirements throws the same exception. What could go wrong? Lets see. Lets continue our journey.

The Test

[Test]
[ExpectedException(typeof(InvalidOperationException))]
public void WhenBidIsLowerThan5_ThrowsInvalidOperationException()  
{
    const int artefactId = 1;
    var auction = new Auction();

    auction.PlaceBid(artefactId, 4m);
}

The Implementation

public void PlaceBid(int artefactId, decimal bid)  
{
    if (bid < 5)
        throw new InvalidOperationException(
            "The bid is to low. A bid must be greater than 5.");

    var hasExistingExceedingBid = GetBids(artefactId)
        .Any(existingBid => existingBid >= bid);

    if (hasExistingExceedingBid)
        throw new InvalidOperationException(
            "The bid is to low. Existing bids with same or higher value exists.");

    _bids.Add(artefactId, new List<decimal> { bid });
}

The Result

------ Test started: Assembly: ClassLibrary1.dll ------

3 passed, 0 failed, 0 skipped, took 0,14 seconds (NUnit 2.5.5).  

OK, still everything works out fine, or does it?

A change of the requirements - instead of check against bid lower than five, ten should be used

Piece of cake. Just update the test WhenBidIsLowerThan5_ThrowsInvalidOperationException and make it fail and correct the implementation so that we get a green test for the new requirement.

The Test

[Test]
[ExpectedException(typeof(InvalidOperationException))]
public void WhenBidIsLowerThan10_ThrowsInvalidOperationException()  
{
    const int artefactId = 1;
    var auction = new Auction();

    auction.PlaceBid(artefactId, 9m); //UPDATED
}

The Implementation

public void PlaceBid(int artefactId, decimal bid)  
{
    if (bid < 10) //UPDATED
        throw new InvalidOperationException(
            "The bid is to low. A bid must be greater than 10."); //UPDATED

    var hasExistingExceedingBid = GetBids(artefactId)
        .Any(existingBid => existingBid >= bid);

    if (hasExistingExceedingBid)
        throw new InvalidOperationException(
            "The bid is to low. Existing bids with same or higher value exists.");

    _bids.Add(artefactId, new List<decimal> { bid });
}

The Result

------ Test started: Assembly: ClassLibrary1.dll ------

3 passed, 0 failed, 0 skipped, took 0,14 seconds (NUnit 2.5.5).  

Three tests passes. How about that! Nothing wrong there is it? Of course. Lets tweak the test that checks if there's any existing bids that are higher.

Updated test #1

[Test]
[ExpectedException(
    typeof(InvalidOperationException),
    ExpectedMessage = "The bid is to low. Existing bids with same or higher value exists.")]
public void WhenHigherBidExists_ThrowsInvalidOperationException_Better1()  
{
    const int artefactId = 1;
    var auction = new Auction();

    auction.PlaceBid(artefactId, 10m);
    auction.PlaceBid(artefactId, 5m);
}

I have now put an expectation on the message, now lets see what happens.

------ Test started: Assembly: ClassLibrary1.dll ------

Test 'ClassLibrary1.AuctionTests.WhenHigherBidExists_ThrowsInvalidOperationException_Better1' failed: The exception message text was incorrect  
Expected: The bid is to low. Existing bids with same or higher value exists.  
 but was: The bid is to low. A bid must be greater than 10.
    BidTests.cs(80,0): at ClassLibrary1.Auction.PlaceBid(Int32 artefactId, Decimal bid)
    BidTests.cs(56,0): at ClassLibrary1.AuctionTests.WhenHigherBidExists_ThrowsInvalidOperationException_Better1()

3 passed, 1 failed, 0 skipped, took 0,20 seconds (NUnit 2.5.5).  

The exception message text was incorrect Expected: The bid is to low. Existing bids with same or higher value exists.
but was: The bid is to low. A bid must be greater than 10.

How about that! The exception was thrown at the wrong place. Of course this situation could have been avoided with some specific business exception or even an ArgumentOutOfRangeException; but again. This is a simplified, fictive scenario which still isn't making me happy, since the ugly ExpectedExceptionAttribute still is present. Lets fix that.

Assert.Throws

Moste testing frameworks (NOT MSTest) has some sort of ability to pass a delegate to a method and the lets you specify that you expect that delegate to throw an exception of a certain type. If the exception isn't thrown, the test fails.

For you MSTest users, you can of course write this test util on your own.

Updated test #2

[Test]
public void WhenHigherBidExists_ThrowsInvalidOperationException_Better2()  
{
    const int artefactId = 1;
    var auction = new Auction();

    auction.PlaceBid(artefactId, 10m);

    var ex = Assert.Throws<InvalidOperationException>(
        () => auction.PlaceBid(artefactId, 5m));

    Assert.AreEqual(
        "The bid is to low. Existing bids with same or higher value exists.",
        ex.Message);
}

I hope you get the point, and that you stop using the not so nifty ExpectedExceptionAttribute

//Daniel

Developer that lives by the mantra "code is meant to be shared".

Comments