Should failing unit tests that we're not going to fix be removed? [duplicate]

Question

My coworker likes to see all of our unit tests pass (as would anyone) and he argues that some of the failing tests of a system he wrote are unnecessary "noise" since they all involve the validity of the data and a separate system in our stack is supposed to be responsible for validation.

Personally, I think keeping the tests allows you to at least understand/acknowledge some of the failing points even if you consciously choose not to fix them.

Is he right in wanting them removed since they are "noise" compared to the other tested functionality and will probably never be fixed?

Just for context. Here are some concrete examples. They all involve code paths that are used, but passing non-validated data into the routines causes unexpected behaviors.:

• passing in NaN results in a false positive.
• passing in a value that overflows a double's significant figures results in false when true should have been returned. Rounding should not occur.
  • "139.9999999999999" -> Double.TryParse() -> 139.9999999999999
  • "139.99999999999999" -> Double.TryParse() -> 140

Answer 1

Let me start off with a few assertions:

• Tests are used to show you where your code fails.
• Tests should cover all scenarios for each unit that you want to test.
• The testresults should give a clear overview of what went wrong.

Now on to your scenario: you acknowledge that these tests are written in a location where they don't belong. I assume from your story that you have a separate project that tests all the validation, so why don't you just move these tests to that project?

The downsides from keeping these tests where they don't belong:

• Tests are fractured throughout projects even though they have a designated place to keep them together.
• The overview over what failing tests are relevant and what ones are consciously ignored becomes harder and you might miss a few new breaking tests.

That being said: removing tests just because you want to see them all pass is not the answer. The preferred solution is obviously to fix them but when this is not possible then you might want to look into classifying them inside a separate project.

It should also be noted that duplicate tests aren't good either. If you have two tests in two different projects that test the exact same thing then you have to do upkeep for two while only getting benefit from one. If you test your validation in the ValidationTest project and you do the very same in your other project, then you should remove those in your other project and keep things contained to where they are supposed to be.

Lastly, you might also want to make use of the testing framework's possibilities. For example MSTest will allow you to add an [Ignore] attribute to tests that should be ignored. This will result in a nice green bar with an additional category of ignored tests, thus keeping them prevalent.

Answer 2

It depends...

If the failing tests belong to functionality that is not being scrapped then they should not be removed.

As you probably are aware the entire point of unit tests are to be able to quickly and promptly test all the little bits and pieces of your code on a micro level to determine every tiny component is doing what it's supposed to. If a component is still in use, it should still have a test...

The next question is why are they failing? Should the tests be moved to a more appropriate location for testing? was there a change in the system design that broke them? Tests weren't designed properly and are failing because of it?

Regardless of the issue. If the functionality still exists they stay AND should be fixed to properly test again, even if that has to wait until later. (soon enough this noise will piss someone off enough they'll do something about it, hopefully that something is to start fixing them.)

Now if that functionality is temporarily moth balled, you might disable the tests until such a time you revisit it.

If the functionality is being removed (and not expected to ever come back) only then would I remove the tests.

Answer 3

Not going to be fixed - why?

Is it something that is planned to be fixed later? Keep tests.

Is the whole functionality no longer relevant (tests are checking something that is no longer true)? See if you can fix the tests to check the correct functionality or just remove the tests if new tests for that new functionality already exist.

Answer 4

It should be obvious that you don't fix your code because a unit test fails, but because your code does the wrong things - the unit test is only an indicator, and unit tests are not by definition always correct. If you say "our code is wrong but we can't be bothered to fix it", leave the unit tests and suffer the shame. If you say "our code is correct, you can't expect it to be called that way", remove the unit test.

If you write a function, define the legal inputs, and define the correct outputs for each input, then a unit test that doesn't give legal inputs and fails should be removed. If your code is limited by the precision of floating-point arithmetic, then a test that fails because it expects higher precision should be removed. That's what seems is happening here.

When I said "legal inputs", that could of course be misunderstood. I can write a function that expects a valid email address as an input; that function may fail if you pass something that isn't a valid email address. I can write another function that expects a non-NULL zero terminated C string that could contain any bytes but is supposed to contain a valid email address in UTF-8 encoding. A unit test that calls it with a null pointer or a non-terminated string is wrong and should be removed. But a unit test that passes a valid string with rubbish data should lead to a correct detection of the rubbish data, because with the function definition any rubbish data is "legal input" that must be handled correctly (most likely identified and an error returned or an exception thrown). And test cases that go into absolute corner cases of the spec and expect them to be handled correctly are correct as well.

Answer 5

The biggest problem with nuisance messaging from maintenance systems, whether they be automated tests, log files, or some other information source is that the receivers of those messages become attenuated to the noise and tune it out.

Right now, you may look at three failing unit tests and think,"No big deal," but it's a slippery slope. You probably don't have a test runner sophisticated enough to "ignore all but these three failures" to show you a green bar, and you probably only vaguely look at "n tests run, m passed, x failed" when you run the tests.

The purpose of unit tests is to pay attention so you don't have to. This allows you the freedom to refactor with the knowledge that the tests will tell you if you've overlooked something.

It sounds as if you think the unit test should stay. In that case, you have to fix the code so the test passes, even if you have to inject a mock for some other component. (It's a unit test, not an integration test.)

Answer 6

I would say: no! If such a failing unit test is not necessery and shows only small bugs you should not remove it, because such units test are essential for future plannig and for bug tracking of your application.

In the current version the bug may not influence your application but can you guarantee that for future versions where requirements are changing?

greetings Nico

Answer 7

The obvious answer is that if the behaviors are not going to be fixed, then in fact their actual behavior is not "unexpected", but rather "expected": it is understood to be in a certain way, and it is understood why it is that way, and it is understood that it will continue that way (won't be "fixed").

Such a behavior should be codified as the expected behavior which the unit test validates.

If passing NaN to a function produces a false positive, then write the regression test so that it validates this. Then one day if passing NaN to that function produces a negative, something changed (possibly in a broken way).

Answer 8

There is a continuum that situations like this fall into. If you need the functionality the unit tests test, you should obviously keep them. If you will never use it ever, ever, ever again, and you know this and are absolutely, positively, 100% sure of it, you should throw those unit tests out. For any other certainty level, the answer is not so cut-and-dry.

If you are going to use the functionality, but not right now, you may want to keep the unit tests, unless they are interfering with other unit tests or code validation. One testing suite I have been using, BusterJS for Node, has a method to put unit tests "on hold" by putting 2 slashes in front of the name, inside its string. In effect, you are "commenting the test out", though Buster still knows it's there and shows it to you. Your testing suite may have something like this: if it dooes, USE IT!

If there is the tiniest chance that you will ever use the functionality, decide how important it is that it be tested were you to use it, how intrusive the tests are, and how much bloat they cause, then decide from there. Also keep in mind that all this code has a cost in terms of readability and space, though usually there are ways to definitively separate it from the rest of the code and it probably won't take up enough extra space to worry about. Performance usually isn't a problem, as the app is usually smart enough to not run the tests while it's running production code.

Finally, remember that you may be able to have the best of both worlds: the unit tests exist and are kept up-to-date, but are either optional in your test suite or not actually performed. How you do this is up to you, but most programming languages have a concept called "commenting". This is probably how you are writing your documentation, so why not use it for stuff you need or want but aren't using right now? Then, if you ever need to use the test, just make it not a comment anymore. Remember to keep the tests as up-to-date as necessary or desired by periodically testing the tests themselves.

Answer 9

In general you should avoid dead code of all kinds. Regardless if its production or test code. Even if you find convincing arguments for keeping a particular code around, the main thing other developers see is that dead code is acceptable and they will do it as well. And because they might not be aware of your justifications they will do it for much less profound reasons. And it's true that will add to the overall noise. See broken window theory.

If you want to keep this dead test code around, just to be reminded about the assumptions your tested methods make about the input parameters, then go ahead document these assumptions for the method itself! Or to make it more explicit you can write explicit assertions in your method, which can serve as a documentation as well as a debug tool when enabled during debugging. There are even precondition frameworks available if you want to go that route, but that is not really necessary.

Oftentimes dead code can be removed without replacement. At other times it still can be deleted but should be replaced with some documentation or some real code (like assertions). If you have to keep dead code around make sure you document the reasons in doing so right above the offending code block.