Why poor tests are worse than no tests

It's been awhile since I wrote my last blog post so I thought I would resume with a subject close to my heart...unit testing.

Unit testing is tricky and lots of people don't focus enough on the key pillars that make up a good test:

• Trustworthy - Can the test be run multiple times and work in the same way each time? A test which works sometimes and fails randomly when a build happens during continuous integration(CI) is just going to be ignored and has no benefit to the team. Worse still, a bad test can give you a false impression that everything is fine when really it's not. 

• Readable - Can you look at the name of the test and work out what it's meant to do? For example, a test named "It_works_and_doesnt_fail" isn't as good as "It_will_add_a_valid_order_without_errors".

• Maintainable - Is your test simple to setup and refactor when code changes are made in the rest of the system? The term code smells applies to tests just as it does to production code. A test with a lot of setup code, mocks or conditional logic makes the test harder to maintain over time and will slow down how quickly you can refactor code. 

Now, the inspiration for this post came when I happened to see a test written by a colleague on another project. The test had some warning signs which I wanted to explore in this post. I'm not going to use the real test here but instead offer up a simple example which includes the same problems. 

Our test is about saving a school student. If the student is attending a Swiss school then the length they should be in compulsory education is 11 years and as such they are saved via a specific Swiss stored procedure. 

In our "Bad Test" we are have some good things like the use of AAA (arrange, act and assert) for breaking the test up so its more readable but the name of the test is lacking meaning. What is it this test is really trying to prove?

public void When_saving_a_student_who_attends_a_swiss_school()
{
// Arrange
var school = new School{IsSwiss = true};
var student = new Student(school);
var sut = new SaveStudentCommandHandler();

// Act
sut.Handle(student, CancellationToken.None);

// Assert
_dbConnectionMock.Verify(mock => mock.ExecuteAsync(
It.IsAny<object>(),
It.Is<string>(x => x == "sp_APISaveNonSwissStudentsToSchool"),
It.IsAny<int>(),
It.IsAny<commandtype>(),
It.IsAny<itransaction>()),
Times.Never);
}

The main problem with our test though is its use of internal implementation details from the "SaveStudentCommandHandler" class. Tests should be written so they test boundaries within an application. The internal details of a class should never be tested as those details are private for very good reasons. 

For a moment lets imagine I'm a developer on this project and I refactor the stored procedure  "sp_APISaveNonSwissStudentsToSchool" so it has a nice new name of "sp_APISaveAllStudentsToSchool" and I update our command handler so Swiss students and non Swiss students use this one single stored procedure. I run the tests and everything is green so my changes must be OK?  

Well no, unfortunately not. The test will still pass, it will make you think a Swiss student isn't being saved via the non Swiss path when it is. What this test should be doing is asserting on the differences between a Swiss student and a non Swiss student. 

Forget mocking the database connection and call the actual database to save a student. If you call the real code you have the ability to retrieve the new student and check it's been saved in the right way. For example with a little refactoring we could have this test: 

public void When_saving_a_student_who_attends_a_swiss_school_it_will_set_the_compulsory_schooling_length_to_11_years()
{
// Arrange
var school = new School{IsSwiss = true};
var student = new Student(school);
var sut = new SaveStudentCommandHandler();
var getStudentQueryHandler = new GetStudentQueryHandler();

// Act
await sut.Handle(student, CancellationToken.None);
var actualStudent = getStudentQueryHandler.Handler(new GetStudentQuery{Id = student.Id});

// Assert
Assert.That(actualStudent.CompulsorySchoolingLength, Is.EqualTo(11));
}

Now the test is refactored it is:

- More readable as its name tells us what it should be doing.

- More maintainable as we aren't mocking the internal implementation details.

- But most importantly it more trustworthy.