looping in unit test bad?

I have a unit test that relies on a random dice roll. I roll a 20 sided die and if the value is 20 it counts as a critical hit.

What I'm doing right now is rolling the 20 sided die up to 300 times. If any one of those rolls is a 20 then I know I had a critical hit.

Here's what the code looks like:

public class DiceRoll
{
    public int Value { get; set; }
    public bool IsCritical { get; set; }

    // code here that sets IsCritical to true if "Value" gets set to 20
}

[Test]
public void DiceCanRollCriticalStrikes()
{
    bool IsSuccessful = false;
    DiceRoll diceRoll = new DiceRoll();

    for(int i=0; i<300; i++)
    {
        diceRoll.Value = Dice.Roll(1, 20); // roll 20 sided die once
        if(diceRoll.Value == 20 && diceRoll.IsCritical)
        {
            IsSuccessful = true;
            break;
        }
    }

    if(IsSuccessful)
        // test passed
    else
        // test failed 
}

Although the test does exactly w开发者_StackOverflowhat I want it to I can't help but feel like I'm doing something wrong.

On a related note, the DiceRoll class has other information in it as well but my question is specifically about looping in a unit test, so I left it out to make it more clear

The problem with this approach is that your are relying on a random behaviour. There is a possiblity that within the 300 rolls, the wanted state never appears, and the unit test fails, without the tested code being wrong.

I would look into extracting the dice roll logic out from the Dice class through an interface ("IDiceRoller" for instance). Then you can implement the random dice roller in your application, and another dice roller in your unit test project. This one will could always return a predefined value. This way you can write tests for specific dice values without having to resort to looping and hoping for the value to show up.

Example:

(code in your application)

public interface IDiceRoller
{
    int GetValue(int lowerBound, int upperBound);
}

public class DefaultRoller : IDiceRoller
{
    public int GetValue(int lowerBound, int upperBound)
    {
        // return random value between lowerBound and upperBound
    }
}

public class Dice
{
    private static IDiceRoller _diceRoller = new DefaultRoller();

    public static void SetDiceRoller(IDiceRoller diceRoller)
    {
        _diceRoller = diceRoller;
    }

    public static void Roll(int lowerBound, int upperBound)
    {
        int newValue = _diceRoller.GetValue(lowerBound, upperBound);
        // use newValue
    }
}

...and in your unit test project:

internal class MockedDiceRoller : IDiceRoller
{
    public int Value { get; set; }

    public int GetValue(int lowerBound, int upperBound)
    {
        return this.Value;
    }
}

Now, in your unit test you can create a MockedDiceRoller, set the value you want the dice to get, set the mocked dice roller in the Dice class, roll and verify that behaviour:

MockedDiceRoller diceRoller = new MockedDiceRoller();
diceRoller.Value = 20;
Dice.SetDiceRoller(diceRoller);

Dice.Roll(1, 20);
Assert.IsTrue(Dice.IsCritical);

Although the test does exactly what I want it to I can't help but feel like I'm doing something wrong.

Your instincts are correct. There is no way to mathematically ensure that you will roll a 20, regardless of how many rolls you do. While probabilistically this will most likely happen, it doesn't make for a good unit test.

Instead, make a unit test which verifies that a critical strike is registered IFF (if-and-only-if) a 20 is rolled.

You'll probably want to also verify that your random number generator gives you a good distribution, but that's another unit test.

And now, an opposing view:

The odds of not getting a 20 in 300 rolls is about 1 in 5 million. Someday, your unit test might not pass because of that, but it will pass the next time you test it even if you don't touch any of the code.

My point is that your test will probably never fail because of a streak of bad luck, and if it does, so what? The effort that you would expend beefing up this test case is probably better spent on some other part of your project. If you want to be more paranoid without making your test more complicated, change it to 400 rolls (odds of failure: 1 in 814 million).