Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I've decided that I want to take a stab at test first programming. So, before I tackled writing an isPrime function, I wrote this unit test. It's my first and I'm not sure I'm doing this right.

I was thinking that I might want to extract the loops to just two methods that I would pass an array to. One for Assert.IsTrue and one for Assert.IsFalse, but I wasn't sure if that was a good idea in a unit test.

  • Am I covering my bases here?
  • What other cases am I missing?
  • What would you do differently?
using System;
using Microsoft.VisualStudio.TestTools.UnitTesting;
using Challenges;
using System.Numerics;

namespace ChallengesTest
{
    [TestClass]
    public class PrimeTest
    {
        [TestMethod]
        public void SmallPrimes()
        {
            int[] numbers = { 2, 3, 5, 7, 11, 13, 17, 19, 23, 29 };

            for (var i = 0; i < numbers.Length; i++)
            {
                Assert.IsTrue(Numbers.isPrime(numbers[i]));
            }

        }

        [TestMethod]
        public void Negatives()
        {
            int[] numbers = { -2, -3, -5, -7, -11, -13, -17, -19, -23, -29 };

            for (var i = 0; i < numbers.Length; i++)
            {
                Assert.IsFalse(Numbers.isPrime(numbers[i]));
            }
        }

        [TestMethod]
        public void PositiveNotPrime()
        {
            int[] numbers = { 4, 6, 8, 9, 10, 12, 14, 15, 16, 18, 20, 21, 22, 24, 25 };

            for (var i = 0; i < numbers.Length; i++)
            {
                Assert.IsFalse(Numbers.isPrime(numbers[i]));
            }
        }

        [TestMethod]
        public void ZeroAndOne()
        {
            Assert.IsFalse(Numbers.isPrime(0));
            Assert.IsFalse(Numbers.isPrime(1));
        }

        [TestMethod]
        public void BigPrimes()
        {
            int[] numbers = {104677,104681, 104683, 104693, 104701, 104707, 104711, 104717, 104723, 104729 };

            for (var i = 0; i < numbers.Length; i++)
            {
                Assert.IsTrue(Numbers.isPrime(numbers[i]));
            }
        }
    }
}
share|improve this question
1  
You are missing BigNotPrime :) - By the way, the term for "not prime" is "composite" (excluding special cases like 1 which is called a "unit" and 0/negative which are really up to programmer convention) and is well-understood, so you could use it if you want –  Thomas Jul 28 at 1:06
    
@Thomas add that as an answer and I'll up vote. Naming is a valid part of a review. =) –  RubberDuck Jul 28 at 2:17

4 Answers 4

up vote 4 down vote accepted

This is quite an extensive set of tests, as each of the numbers in the list is a test of sorts.

Here are some suggestions:

  1. How about testing negatives that are non-primes?
  2. Add a set of tests for big non-primes (pseudoprimes ideally)
  3. Add tests for values that are close to the limit of ints

Lastly, this is optional, but you can add some that are randomly generated so you can't cheat and hardcode the results.

share|improve this answer
    
What do you mean by "add some that are randomly generated"? –  RubberDuck Jul 27 at 23:40
1  
As in, make some of tests generate some of their own test cases. Aka, the smallPrimes test would generate a few small primes to add to the test cases. –  mleyfman Jul 27 at 23:41
    
If you're dealing with random numbers, how can you guarantee that those unit tests will always pass? –  krillgar Jul 31 at 18:46
    
You should generate random prime numbers to test isPrime = true and generate random non-primes to test isPrime = false, or you can simply test the random number with a reference function and test to make sure the result from your custom function is identical. –  mleyfman Jul 31 at 18:48

Methods in C# are written in PascalCase, so let's change isPrime to IsPrime.

You can use LINQ to make the code a bit clearer. Instead of

for (var i = 0; i < numbers.Length; i++)
{
    Assert.IsTrue(Numbers.isPrime(numbers[i]));
}

You can write

Assert.IsTrue(numbers.All(Numbers.IsPrime));

Instead of

for (var i = 0; i < numbers.Length; i++)
{
    Assert.IsFalse(Numbers.isPrime(numbers[i]));
}

You can write

Assert.IsFalse(numbers.Any(Numbers.IsPrime));

or

Assert.IsTrue(numbers.All(n => !IsPrime(n)));

whichever you find easier to read (I prefer the former).

In fact, I would be a bit lazy and copy a big list of prime numbers from somewhere and then write

var primes = Enumerable.Range(0, 1000000).Where(Numbers.IsPrime);
Assert.IsTrue(primes.SequenceEqual(PrimesLessThanOneMillion));

If you take @mleyfman's suggestion to test with randomly generated primes, be sure to seed the random number generator. Unit tests must be reproducible.

That is, never do this

var random = new Random();

but do this

var random = new Random(42);

One of the benefits of unit tests is that if they fail, you know it is due to a change in the code, and can then git bisect (or similar) to find the breaking commit. If a unit test fails due to something that is not a change to the code (e.g. a different RNG seed) you have lost this important benefit.

Edit: as @svick pointed out, using the same seed may not generate the same sequence between different versions or implementations of .NET. See these answers on StackOverflow. If all dev machines (including the build server) are running the same version of .NET, you should be fine.

share|improve this answer
3  
I'm not sure using the same seed is enough to guarantee the same sequence, when running on different versions of .Net (or .Net and Mono) –  svick Jul 28 at 0:33
    
@svick good point, I'll add that to the answer. –  mjolka Jul 28 at 0:39
2  
How would you generate the expected results for randomly generated test cases? –  Ben Aaronson Jul 28 at 8:41
    
@BenAaronson by generating random numbers and checking with IsPrime ;) and then once you use a fixed seed, you can just hardcode the primes in your test... Yes, I don't think randomly generating them is a good idea, I just wanted to make a note about using a fixed seed. –  mjolka Jul 28 at 8:44
    
Or by pulling random indexes from files containing primes and non-primes @BenAaronson. –  RubberDuck Jul 28 at 11:29

Naming

To understand why a test fails we need to be able to understand what the test is supposed to test by just glancing at the method name.

So looking at your testmethod

public void SmallPrimes()

wouldn't tell you, in the case it fails, what this test method should be testing.

Unit Test methods should be named following MethodName_StateUnderTest_ExpectedBehavior.

So a better name would be:

public void IsPrime_RangeOfPrimesFromTwoToTwentyNine_Evaluated() 

And

public void Negatives()

would become

public void IsPrime_RangeOfNegativeNumbers_Evaluated()

For

public void ZeroAndOne()

I see another problem.The method's name implies that this method has two responsibilities. If you don't mind , you should at least provide a message to the Assert to indicate which part has failed.

So changing this method to

[TestMethod]
public void IsPrime_NumbersZeroAndOne_Evaluated()
{
    Assert.IsFalse(Numbers.isPrime(0),"Failed for {0}",0);
    Assert.IsFalse(Numbers.isPrime(1),"Failed for {0}",1);
}

will show you, if this test will fail, with which number your code has problems with.

or better

[TestMethod]
public void IsPrime_NumberZero_Evaluated()
{
    Assert.IsFalse(Numbers.isPrime(0), "Failed for {0}",0);
}

[TestMethod]
public void IsPrime_NumberOne_Evaluated()
{
    Assert.IsFalse(Numbers.isPrime(1), "Failed for {0}",1);
}

the former SmallPrimes() testmethod will become

 [TestMethod]
 public void IsPrime_RangeOfPrimesFromTwoToTwentyNine_Evaluated()()
 {
     int[] numbers = { 2, 3, 5, 7, 11, 13, 17, 19, 23, 29 };

     for (var i = 0; i < numbers.Length; i++)
     {
         Assert.IsTrue(Numbers.isPrime(numbers[i]), "Failed for {0}",numbers[i]);
     }

 }

Some might complain that the method names are too long but in the end it is all about readability.

share|improve this answer
1  
I'm not sure about those names, but great call on returning a message. I was thinking of how great that would have been when I was testing the function. –  RubberDuck Jul 28 at 11:27
    
My first posting on here, I was chided for having multiple Asserts in my Unit Test. I completely understand what they meant now with everything that well explained above. It may seem overkill, but it's good to know exactly what the test is doing and why it failed. That is part of the reason why so many Unit Test method names are so ridiculously verbose. –  krillgar Jul 31 at 18:49

What would you do differently?

I would use NUnit rather than the Microsoft test tools. NUnit has a better assertions library (for example Assert.Throws<T>() is quite useful). It also has easier data-driven testing that would let you remove all of the looping logic from your code. For example:

[Test]
[TestCase(2)]
[TestCase(3)]
[TestCase(5)]
[TestCase(7)]
[TestCase(11)]
[TestCase(13)]
[TestCase(17)]
[TestCase(19)]
[TestCase(23)]
[TestCase(29)]
public void SmallPrimes(int num)
{
    Assert.IsTrue(Numbers.isPrime(num));
}

Admittedly it's a lot of lines, but they're very easy to understand. Also, the test method itself becomes dead simple.

Since I'm on the topic of tooling, you might want to look into the TestDriven.NET Visual Studio plugin, which provides very nice "right-click -> run test" integration.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.