Test Review Guidelines

Benefits:

• It's Easier to understand the programmer's intent. Becuase test code is intended to convey intent by the test author, the reader of the test has a much easier time understanding what the developer what trying to accomplish, instead of reading how they accomplished it. For example, in a code review you could be reading many lines of code before really understanding what the developer was trying to accomplish in terms of busines requirements. in a test, just reading the name of the test can sometimes provide good enough understanding of the developer's understanding of the requirement.
• It's easier to dive into the implementation code if needed. Becuase the test is really a working harness against the code, it's easy to just start debugging or going into the code under test from a test case, instead of launching the real application or looking for that code in the whole code base. 
• It's a good way to maintain coding standards in tests. Doing a test review helps you make sure that test names are expressive enough, or that all the devs on the team use the same conventions or teqhnicues for the same kinds of things. Conssitency in test code conventions is important if we want all the devs on the team to understand each other's code and this is a good way to make sure it can actually happen, or catch it early if it does not. 
• It's a good way to catch test smells and bad practices, and teach newcomers. For an experiences test developer it should be pretty easy to catch many small test smells (as outlined later) that might not be as easy to catch for newcomers. Each one of those smells can lead to less readable, maintainable or trustworthy tests. it's a good way to teach the techniques as people actually write real test code. 
• It's much faster than a code review. Tests are much smaller and contained in nature than production code, so they are relatively very easy to review and understand. Test review is not a full replacement for code review, but more of a prefix for it, or a filter to review code that has been written, by starting out with known use cases of the new code.

 

what to look for

  there are three main areas to watch out for: readability, maintinability and testability

Readability

We want to make sure the test is as readable as possible to whoever is looking at it, even if it is the first time they read this test. that means we want to surprise the reader as little as possible, as well as make sure that the intent of the test as well as what the test is actualy doing or any assumptions it may have, are clearly visible.

 

• Make sure the test name defines the three basic ideas: what are you testing? (name of method or component), under what scenario or state are you testing it? What is the expected behavior?

for example, the test name could be:

[Test] 
public void IsLoginOK_NonExistingUserName_ReturnsFalse()

 

• Make sure that in the test class, if a [Setup] method is used, it only initialized objects that are used by all the tests in that class. otherwise it gets harder to understand the dependencies that each test needs.  Short example:

 

Calculator c;
[Setup]
public void Setup()
{
c = new Calculator; //assumed to be used by all the tests in this class
}  

        

  If there are some objects that only some tests need, but you still want to make sure they are initialized in one place, create a special factory method that creates those dependencies and call it directly from only those tests that need that object.

for example:

 [Test]
public void IsLoginOK_WithInvalidUserObject_ReturnsFalse()
{
UserObject user = CreateUserWithBadLogin();
        
} 
        
[Test]
public void IsLoginOK_WithInvalidUserObjectAndNoDB_ThrowsException() 
{
UserObject user-CreateUserWithBadLogin(); 
} 
public UserObject CreateUserWithBadLogin()
{
return new user ('bad login'); 
} 

 

• Make sure that the arrange, act and assert parts of the test are divided by a space, so that they are easier to read and understand. also make sure they are not mixed together.
• Make sure that there are no magic numbers or strings in the test. Always use the simplest form of a value that will still prove the test works or fails. If you need to send in a number, and '1' is just as good as '1042', use '1' (just like you'd use 'i' and 'j' in for loops - it's expected and part of a convention. the same with strings. if and empty string is just as OK as sending in "some other string", use the empty string as the simplest form.  if you have to use some number or string, put it in a well named variable or const that explains it. for example:

• [Test]
  public void IsLoginOK_WithInvalidUserObjectAndNoDB_ThrowsException() 
  {
  string loginStringWithTooManyCharacters = "123456789";
  ... 
  } 

   Readability with mocks and stubs

 

•  If you are using a hand rolled stub, make sure it does not contain hard coded return values or behavior. All behavior should be set from within the test. Otherwise whoever is reading the test will not understand why the test should fail or pass. for example of less readable test:




[Test] 
public void IsLoginOK_WithInvalidUserObjectAndNoDB_ThrowsException() 
{
//if I only look at the test I will have no idea the logger will always return 'false' when "Write" is called. 
ILogger stubLogger = new MyLogger();
... 
} 
       
//the stub 
class MyLogger:ILogger
{
public bool Write(string text)
{
return false; 
} 
} 

 

here is a more readable version of this:

[Test] 
public void IsLoginOK_WithInvalidUserObjectAndNoDB_ThrowsException() 
{
ILogger stubLogger = new MyLogger();
stubbLogger.WriteReturnValue=false;  //test has full and explicit control of stub behavior
... 
} 
       
//the stub 
class MyLogger:ILogger
{
public bool WriteReturnValue; 
public bool Write(string text)
{
return WriteReturnValue; 
} 
} 

 

• Make sure that if a manual stub or mock is used by only one test class, it is located in the same code file. that makes it easier to browse the code.
• Make sure that stubs and mocks are not initialized in a setup method. usually this makes the test code less readable because the reader has to jump to the setup to see all the assumptions for the test. most of the time this is also a test smell becuase different tests in the same class will likely want mocks and stubs to behave differently for different scenarios. which means each test will have to initialize the stubs and mocks locally anyway. putting them in the setup method could prove to be useless most of the time. having specialized helper methods that create those stubs and mocks with different parameters and being called by each test could make things more readable.
• make sure that stubs and mocks variables in the test are named correctly by role. for example 'stubLogger' or 'mockLoginManager'. this makes understanding the test much easier. if this is too much work, just call everything 'fakeXX' as in 'fakeLogger' and 'fakeLoginManager' as to not put a specific role on them. the reader of the test can then look at which fake object is actually being verified upon and determine that is the mock in the test. if the end of the test is an assert.XX it means all fakes are really stubs.
• try not to use an auto-mocking container if it is not needed. it makes tests less readable for most people, and more complicated.

 

 

Maintainabilty traps

 maintinability problems mean that maintinaing your tests takes too long. sometimes longer than the time the tests save you on the project. 

 

• make sure a test does not invoke another test. that makes for hell on earth at some point. no isolation, no setup methods running between them, ordering of tests may become an issue in essay writing . you get the idea. This usually happens when someone wants to re-use some code in another test. instead, refactor the piece of code you want both tests to run into a shared mathod, and call it from both of the tests. 
•  make sure that you don't create production code objects within the test (if you find the word 'new" in your test it might be a tell). use factory methods to create objects. that way if the constructor of an object changes, you only need to change the factory method in the tests for the at object, instead of going through all the tests and changing them one by one to use the new constructor.
• make sure that the DataRow pattern is used whenever you have the same exact test and assert, with the only things changing being the input parameters and expected output values. if you use NUnit or MbUnit look into the [RowTest] attribute. in XUnit.NET this is called [Theory]. with MS Test there is no such attribute. you will have to refactor the test into a special method that does the assert and takes the input and expected output as parameters:

[Test]
public void Calculate_TwoNumbers_Subtracted_1()
{
	bool expected = 1;
	Calculate_Verify(2,1,expected);
} 
[Test]
public void Calculate_TwoNumbers_Subtracted_1()
{
	bool expected = 3;
	Calculate_Verify(2,5,expected);
} 

private void Calculate_Verify(int howmuch, int from,int expected)
{
	int result = calc.Calculate(howmuch,from);
	Assert.AreEqual(expected,result);
}

 




 Maintainability with mocks and stubs - over specification

there are several smells of over specification in mocks and stubs. usually specification means that you are assuming too much about the underlying implementation of the code under test, to a point where any little change to private code may break your test without there actually being a bug. 

 

•  Make sure there is one mock per test. the rest should be stubs. More than one mock per test means you are testing more than one thing, meaning you may be assuming too many things, or your test is far easier to break.  
• Make sure you only verify (or assert calls) on one object. this is a variation of the previous rule. if you use RhinoMocks - avoid using VerifyAll. instead use 'Verify(mock)' to make sure only one object is used a a mock. if you use older version of typemock, don't use MockManager.VerifyAll. instead use mock.Verify() on a specific object.
• Don't use stubs as mocks at the same time. you either use a stub to return some value to the application or throw an exception essay editing, or you use a mock to check that a call has been made. verifying calls on a stub just makes sure that the stub was called correctly. but the point is to only verify one call that should happen in the end as the result of the whole operation.
• prefer using Assert.XXX instead of using mock objects (state based testing over interaction testing)  as it makes the test more maintainable over time. for more info see here and here. The point is, if you can resort to only needing to use stubs in your tests, you are more likely to have the tests stand the test of time. the only time you have to have a mock is when the end result of an operation is a call to something you have no control of (sending an email, calling a service..)
• Avoid asserting that a method was not called since it is pure overspecification. instead assert that something is true. there is always  an end result to something.

 

smells of test bugs

It's easy to write tests that may have bugs or that may not be trusted by other developers. here's some pointers to preventing this problem.

 make sure unit tests and integration tests are in seperate projects. that way developers will always have the ability to run all the unit tests in the system with one click, and they should all pass. integration tests usually need configuration and may not  pass before that is done. you want your devs to be able to get a green light without doing anything, so at least have some of the tests always be green and in a seperate project.

make sure the test usues hard coded numbers and strings for asserting expected values. try to avoid dynamically created expected values, as that might create a bug in the test, or even repeat the bug from production code. for example: