Skip to main content

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.

   


Labels:

Comments

Post a Comment

Popular posts from this blog

Shouldn’t validating a checkbox be easier than this?

  Well it's been a while since I have done a post but with changing jobs recently and life in general getting in the way I just couldn’t seem to find the time. However, now I have a spare five minutes I thought I would fire off a post :) So what's the post about? Validating a checkbox web control. Now I know your reading this thinking that's easy all you need is a custom validator and a event handler in your code behind and job done? Well in most cases yes I would agree with you. However, my latest project (a legacy application) uses code generation to create the vast majority of the ASP.NET pages and user controls. Therefore it’s difficult for me to include anything other than the standard set of ASP.NET validators or something which inherits off the “ BaseValidator ” and get it working correctly in the XSLT templates generating the code. Because of this I decided to create my own validator for the checkbox web control. So how do we get started? Well first off you ne...
Post a Comment
Read more

X509 certificate encryption fun :-)

Recently I needed to encrypt data on a server and allow a limited number of service accounts the ability to decrypt that data so it was as safe as possible. The approach I took to achieve this was by using a X509 certificate and it's ability to allow you to encrypt information via it's public key and decrypt that information through the private key. The key parts of this approach are: - Create a certificate - Ensure the KeySpec of the certificate is set up correctly to allow for encryption e.g. "KeyExchange" or "None" if you are doing this via PowerShell - Set the security on the private keys so only specific user accounts can access it and decrypt information encrypted via the public key. Step 1 - Create a certificate to use The easiest way to get a quick example going is via PowerShell to create a dummy root certificate and the one we will use for encrypting and decrypting. $rootCert = New-SelfSignedCertificate -CertStoreLocation Cert:\LocalMachine\My -Dns...
Post a Comment
Read more

SharePoint - Search Service Application (SSA) - Attempted to perform an unauthorized operation

On my current project I needed to adjust and add some search mapping via SharePoint's Central Administration web site. This should have been very straight forward as you have the ability to add managed properties and mappings easily via the UI. However, when I went to save my changes I got the error "The settings could not be saved because of an internal error: Attempted to perform an unauthorized operation". Now this was very confusing as I am administrator on the server and added into all of the correct SharePoint groups. I also tried the same action via PowerShell and got the same error.   After a few hours of research and head scratching I managed to get to the bottom of the problem which is the way my user account had been added as an administrator to the server. In the company I work for to make it easier to manage the administrators on a server a group is created in active directory called " servername_admins ". This group is added to the local administra...
Post a Comment
Read more