7
\$\begingroup\$

I've decided to challenge myself by writing a test standalone DLL with the objective of making all of its publicly-exposed objects able to be as intuitive and as simple-to-use as possible.

I've decided to try and create some objects that might be used in a game, like a deck of cards and a die.

Here's my Die for example:

public class Die
{
    private readonly Random _random;
    private readonly int _numberOfSides;

    public Die(int numberOfSides)
    {
        if(numberOfSides < 2)
            throw new ArgumentException(string.Format(GameComponentsResources.InvalidNumberOfSides, numberOfSides));

        _numberOfSides = numberOfSides;
        _random = new Random();
    }

    public int Roll()
    {
        return _random.Next(1, _numberOfSides);
    }
}

So obviously the random part of this is untestable unless I write a non-deterministic test, which I don't want to do.

If this was standard enterprise code I was writing, I'd mock the Random and inject it. However, let's say someone shelled out some cash for my GameComponents library. Do I think they would consider it a good product if they have to instantiate and pass in a random just to get a die for their game? Nah. I'd rather keep it concise and intuitive, like this: var die = new Die(6); or at the very most var die = dieCollection.GetDie(6);

So standard constructor injection is out of the picture for now. Property injection is better, but I still don't want to start depending on a container and writing a bootstrapper for something that is basically encapsulating a random.

I was thinking about getting hold of a seed value and passing it around (but without forcing consumers to pass in a seed value, unless they really wanted to), so I can get some deterministic values for my test, but the only way I can think to do this while maintaining good encapsulation is making a method like this:

protected virtual long GetSeed() //could be a property too 
{
    return Environment.TickCount; //or whatever
}

...and then inheriting from it and overriding it in my tests. I could also do this for the Random itself. But that's generally a legacy code refactoring technique. Do I really want my shiny new code to rely on this?

I'd like to see some solutions where I can get near-100% test coverage (i.e. not creating more layers but still ultimately resulting in an area of code deemed "untestable") and maintain a simple, intuitive quasi-API as possible.

\$\endgroup\$
5
\$\begingroup\$

Add one more constructor that takes an extra Random param:

public Die(int numberOfSides, Random random)
{
    if(numberOfSides < 2)
        throw new ArgumentException(string.Format(GameComponentsResources.InvalidNumberOfSides, numberOfSides));

    _numberOfSides = numberOfSides;
    _random = random;
}

public Die(int numberOfSides)
{
    this(numberOfSides, new Random());
}

Most users will use the single-param version. Some users, who want more control, for example in tests, can use the two-param version. You can write your tests in a straightforward way using the two-param version.

If you really don't want to expose the two-param constructor in the public API, then make it protected. But I think it's quite fine to include in the public API, it doesn't make the library confusing, and I don't see any harm.

If, for some reason you are against passing a Random instance (like @JasonLind, see his comment), then you can make the constructor taking a Random above private, and add another constructor that takes a seed parameter instead:

public Die(int numberOfSides, int seed)
{
    this(numberOfSides, new Random(seed));
}
\$\endgroup\$
  • \$\begingroup\$ Random is a pretty heavy class to derive from and Mock, you really should create an interface and wrap random. \$\endgroup\$ – JasonLind Mar 2 '15 at 9:53
  • 1
    \$\begingroup\$ I think there might be a misunderstanding. Classes that use a Random inside are hard to test because of the randomness. By passing a Random, the class becomes predictable, by making it use an appropriate seed. I don't know about C#, in Java, to test this Die class, I would pass it a new Random(1) for example, and then I will get predictable die rolls. I thought this was the purpose of the question: how to make this testable. There's no need for mocking anything or deriving from Random, as far as I can tell. \$\endgroup\$ – janos Mar 2 '15 at 9:58
  • 1
    \$\begingroup\$ @JasonLind I didn't suggest to derive from or mock a Random, so I don't understand your objection. I also strongly disagree with "creating an interface to wrap random". The seed parameter of Random is designed specifically to control the generated sequences for testability. It's a widely used technique to slip in a specific seed to make otherwise random behavior testable, and it's good that way. That said, if you're really against allowing users to pass in a Random, I added another alternative taking a simple int seed instead. \$\endgroup\$ – janos Mar 2 '15 at 11:02
  • 2
    \$\begingroup\$ If you don't want to expose your 2 parameter constructor to the public API, you can make it internal, then set [assembly: InternalsVisibleTo("[Test Project Namespace]")]. \$\endgroup\$ – krillgar Mar 2 '15 at 17:45
  • 1
    \$\begingroup\$ @janos Fortunately, the important methods on Random are all virtual, so it's actually about the same amount of effort to create a controlled Random using a seed or simply creating a Mock<Random> and having Next() return a static value or sequence of values. I haven't actually mocked it but the class looks mockable. As for your comment about the purpose of the question, you're right, the focus is testability. How I set up dependencies is less important than figuring out what should be a dependency and how to pass them without creating untestable areas or jacked-up access modifiers. \$\endgroup\$ – moarboilerplate Mar 3 '15 at 16:46
4
\$\begingroup\$

Your Die will never roll _numberOfSides.

return _random.Next(1, _numberOfSides);

The second parameter of Next is the exclusive upper bound of the random number returned.

\$\endgroup\$
  • \$\begingroup\$ Thanks. Any thoughts on testability? \$\endgroup\$ – moarboilerplate Mar 1 '15 at 5:01
4
\$\begingroup\$

I will go against the grain and advocate exactly the alternative you've been so quick to dismiss: standard constructor injection.

  1. provide a constructor that takes a Random
  2. mark it as internal.
  3. decorate your GameComponents assembly with an

    [assembly: InternalsVisibleTo("GameComponents.Tests")]
    

and bish bash bosh, you're golden.

No extra bits of public interface (uuugly), no extra interfaces to implement.

Your fictional customer won't see the constructor, and you'll be able to inject fake randomness to your heart's content.

A while ago I wrote a short post about InternalsVisibleTo, should you want to read it.

\$\endgroup\$
  • \$\begingroup\$ I was leaning towards this, but that still means my public constructor still needs to do : this(new Random()) though, right? \$\endgroup\$ – moarboilerplate Mar 2 '15 at 23:13
  • \$\begingroup\$ well, really : this(sides, new Random()) but yeah \$\endgroup\$ – moarboilerplate Mar 2 '15 at 23:22
  • \$\begingroup\$ @moarboilerplate yes, of course, but that should not be a problem. Client code won't know that your public constructor is delegating to the internal one. \$\endgroup\$ – s.m. Mar 3 '15 at 7:37
  • \$\begingroup\$ I think the problem I have with the testing constructor approach is that I end up not having test coverage on the constructor(s) that are going to be invoked by clients. Provided that constructor body stays empty that's not really an issue, granted, but I'd like to not have an area of untestable code. \$\endgroup\$ – moarboilerplate Mar 3 '15 at 15:46
  • \$\begingroup\$ @moarboilerplate Then you can introduce an internal property instead of an internal constructor. The property will hold the Random and, under normal circumstances, will be set by your public constructor. In your tests you will set it to your own fake Random. That way you can also add a test case to check that the public constructor actually sets the internal random. That way I don't think you would end up with untestable code, unless I'm missing something. I wouldn't like an approach that exposes the Random publicly, that's an implementation detail. What if you want to change it afterwards? \$\endgroup\$ – s.m. Mar 3 '15 at 18:59
3
\$\begingroup\$

I agree creating a IRandom interface that you inject into your dice and implementing a RandomMock in your tests would be overkill. In my tests I would just

 int sides = 6;
 var dice = new Dice(6);
 var roll = dice.Roll();
 Assert.IsTrue(roll >= 1 && roll <= 6);

As for the seed I think injecting that is overkill as well but you can make it an optional constructor argument and dependency inject as necessary.

\$\endgroup\$
3
\$\begingroup\$

Because the parameterless Random constructor uses a time-dependent seed, you can get effectively identical Die objects when they are created soon after each other. This is likely to cause some confusion if the user of the API tries to create e.g. six dice for the user to roll.

I wrote the following code to get a rough idea of how likely this is to happen

const int iterations = 10000;
var identicalPairs = 0;
var seedArray = typeof(Random)
    .GetField("SeedArray", BindingFlags.NonPublic | BindingFlags.Instance);
for (var i = 0; i < iterations; i++)
{
    var seedArrayA = (int[]) seedArray.GetValue(new Random());
    var seedArrayB = (int[]) seedArray.GetValue(new Random());
    if (seedArrayA.SequenceEqual(seedArrayB))
    {
        identicalPairs++;
    }
}

Console.WriteLine("{0} of {1} pairs were identical", identicalPairs, iterations);

Usually the output was 9999 or 10000.

\$\endgroup\$
  • \$\begingroup\$ Good catch. I haven't revisited the code yet but I'm definitely leaning towards requiring/doing more with a seed, especially because it will contribute to a deterministic test. \$\endgroup\$ – moarboilerplate Mar 1 '15 at 16:04
  • \$\begingroup\$ Whoaah... That was a seriously bad random generator!!! \$\endgroup\$ – holroy Nov 3 '15 at 22:56
0
\$\begingroup\$

In retrospect, the best way to test a component that uses random like this is to take a property-based testing approach and check that within a series of rolls the output never exceeds the number of sides the die was instantiated with.

\$\endgroup\$

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.