Wednesday, December 20, 2023

Never delete tests

Ok, not never, but only delete them when they really serve no value and the codebase is improved by their removal.

As an example, I was recently told this test (well, one very like it) should be deleted.


[TestMethod]
public void CanCreateInstance_WithoutError()
{
        var sut = new MyCoolClass();
   Assert.IsNotNull(sut);
}


I was told this provides no value, and the code path will be covered by one of the [many] other tests [that exist and will remain] so I should delete it.


Firstly, I originally created this test before `MyCoolClass` existed, and the test only threw a NotImplementedException. I created test cases for what I knew I would want to implement. This allowed me to have Test Drive the Development and tell me when I was complete--because all the tests passed.


Ok, but that was then. The class has now been implemented, and many passing tests exist. Is it still worth keeping? 

Secondly, I think there is long-term value in having such a test:

  • It documents that it needs to support a parameterless constructor. If an additional constructor was added, this test shows that there's logic dependent on supporting a parameterless constructor.
    Yes, the compiler will complain if other code depended on such a constructor and one no longer exists, but part of the purpose of the test is to document the design intent behind the code. Here; the test documents that a parameterless constructor was intended and an instance can be created without any dependencies. If we start adding dependencies [or injecting them?] into the constructor, then this test tells us (hopefully) that we may need to revisit the expectations and wider implications of such a change. We may be able to get the code to compile after such changes, but are we breaking some subtle design intentions and decisions that were part of the original design and which may have implications that aren't immediately obvious?
    If there is logic in the constructor, this is especially important.--Even if there isn't any logic there now, it's good to have the test ready in case some is added.
  • It sets an example for other developers on the team that you want to encourage to write more tests. Having an example that shows all of the TDD process is better than only showing the part at the end. 
  • (Probably most importantly) This is useful when/if something fundamental breaks in the class. While technically, this functionality is exercised as part of all(?) the other tests, if everything else starts failing, but this doesn't, then we know the issue is not in the constructor. If this test fails, we know there's a problem in the constructor. We don't have to work it out from the other tests.
    Tests don't only tell us when something is wrong. They help us identify what is wrong when something breaks.
    I want to be able to look at the names of tests and see what they test. I don't want to have to work it out by looking at the data.  Looking at the names of the tests that are passing and/or failing should give me a clue where the problem is.


Why might I delete a test like this?

  • If we needed to support a constructor that does take parameters. (Although I may just modify this one.)
  • If this test was slow and the total execution time of all tests was an issue. Because this code is technically "tested" by other tests, I may accept that overall execution time is more important. In this case, it's not. This test runs in under 1 millisecond and is part of a larger suite that runs in a few seconds.


Finally, does debating whether to keep or delete this test constitute a good use of everyone's time?

There are many factors to consider:

  • How long does the test take to run? (Multiplied by every time it is run)
  • How much time is spent debating whether to remove it?
  • How long does the CI on the PR to remove the test take to run?
  • How long does it take someone reviewing test results to skip past (assuming they pass successfully) the results for this test?
  • How much time is spent navigating and reviewing the test while maintaining the code base? 

No, there often aren't easy answers to the above questions.

My default position on removing tests is to only do it when the test is wrong or is so slow that it has negative consequences on the development process/workflow (and so harms the business.)

I've never been in a position where the best use of anyone's time was to discuss and remove tests. (I even wrote this post in my own time.)

I can also only think of a handful of occasions where the execution time of a test (over the lifetime of a project) was more than the execution time of the CI on a PR to remove it. Even in most of those situations, the original test was kept, but moved to be less frequently run.


5 comments:

  1. There is no need for the Assert call in the above test. If new returns null then .NET is malfunctioning, not your class.

    ReplyDelete
    Replies
    1. Yes, technically, but I prefer that the test makes clear what is being tested. Maybe if the test was called something along the lines of "Creating an instance of the class does not throw an exceptions" - but then I can imagine lots of people potentially pointing out that tests "should" always have an assert.

      Delete
    2. The point still stands. You're test is testing something impossible. What you could do if you insist on having an assertion is actually assert what you're testing... that the constructor does not throw!

      The only point in this whole blog post I can agree with is documenting that you should have a parameterless constructor on the type. So this is an opinionated POV, as calling the ctor from other tests could be viewed as documenting that. I don't see a strong case being made for the test to exist, much less for the "don't delete tests" stance.

      Delete
  2. Nice post. I have a few projects recently with tests like these (because I also started with just the test and no class), and was thinking if maybe I should delete them. But, it's good to know that I'm not alone in thinking that I should actually keep them!

    ReplyDelete
  3. Kudos for using TDD. I'm right with you there, and I could see myself writing a test like this as part of my TDD process.

    That being said, I would have to agree that after the class has been created this test could probably be subsumed into another test that delivers additional value.

    For example, you say that there might be logic dependent on there being a parameterless constructor. Why not just use that parameterless constructor in the test for that logic, or create a copy of the logic test that uses the parameterless constructor?

    Keep in mind, too, that it's very likely this is not the only empty ctor test in the solution. A project with 50 classes, each of which has a test like this, means 50 of these unit tests hanging around.

    As someone who uses TDD by choice rather than as a team mandate, I already encounter healthy skepticism from other developers about how valuable TDD really is. Keeping a test like this around could fuel their arguments against the value of frequent unit testing or TDD. It's not a hill I would choose to die on either way, but I offer it as food for thought.

    ReplyDelete

I get a lot of comment spam :( - moderation may take a while.