Oops, I Did It Again...

By 'it', I mean Assert. But lets step back.

Following a dry run for Ryan Weppler's upcoming webcast Testing Out the MVC Routing, a few guys got into a discussion about multiple Assert statements in a unit test. Steve 'asserted' (oh yeah, I just went there) that you should only ever have 1 Assert per unit test (edit: from Steve per the comments: "the one Assert per test thing is just best practice, not a written-in-stone sort of thing. As long as you have a good reason for doing it, then fill your boots. Just ask yourself why you're doing it."). I was attending remotely and didn't have a mic, so I could not respond, which turned out to be a good thing. You see, I wanted to say something like "I have a bunch of tests from my demo code that used multiple Asserts, and they all were necessary/made sense". Because I couldn't speak, I decided to send an email to that effect.

But the good thing about email is that it generally encourages you to back up your random statements with crazy things like facts and examples, so I popped open the Dice Hockey code and looked at my tests. Sure enough a bunch have more than one Assert. Yet as I gazed upon those 'perfect' tests with the intent to throw them in Steve's metrosexually attractive visage, I realised that I couldn't do it. In each case, Steve was right. Damn him and the Mac he rode in on!

Anyways, et's take a look for ourselves at a function from one of my tests:

private void EnsureAllDesiredPlayersWereRetrieved(

                IEnumerable<Guid> playerIds)

{

    Assert.AreEqual(playerIdsToRetrieve.Length, 

        playerIds.Count(), "Not all players retrieved!");

    IEnumerable<Guid> playersNotRetrieved = 

        (from ptr in playerIdsToRetrieve

         select ptr).Except(playerIds);

    Assert.IsTrue(playersNotRetrieved.Count() == 0, 

       "Not all players retrieved!");

}

In this first example, the first Assert is meaningless. It only has value because I know that during my data initialization at the start of the test I put more players into the database than I did in my list of ID's to retrieve. But really, the second Assert is the one that makes sure everyone got retrieved that should have been. If I want to test that no other players were retrieved (which is basically what that first Assert is doing), I should have another test.

And hey, speaking of that, I do have another test for that! Well, sort of. I have another function, as seen here:

private void EnsureNoPlayersWereRetrievedThatShouldNotHaveBeen(

            IEnumerable<Guid> playerIds)

{

    IEnumerable<Guid> playersThatShouldntHaveBeenRetrieved = 

       (from p in playerIds

        join ptnr in playerIdsToNotRetrieve on p equals ptnr

        select p);

        Assert.IsTrue(
       playersThatShouldntHaveBeenRetrieved.Count() == 0, 

          "Players were retrieved that should not have been!");

}

That looks nice, right? But check out where these functions get used:

[TestMethod]

public void Should_retrieve_only_specified_players_from_Players_table()

{

    var retrievedPlayers = 

        testGateway.GetPlayers(playerIdsToRetrieve);

    // Check that all desired players were retrieved

    EnsureAllDesiredPlayersWereRetrieved(

        (from rp in retrievedPlayers

         select rp.PlayerId));

    // Check that no players were retrieved that 

    // should not have been

    EnsureNoPlayersWereRetrievedThatShouldNotHaveBeen(

        (from rp in retrievedPlayers

         select rp.PlayerId));

}

That's a pretty clear-cut case of bad test writing; those should be two seperate tests, one for each function. Combine that with the clean-up of the two Asserts in that first function and we'd have a much nicer solution.

UPDATE: Code is all visable now, sorry for the formatting issues.