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.
