Broken Old/Legacy Unit Tests

Question

I work for a big company and I'm responsible for a large java application with thousands of junit tests. Since I moved to this role, there have been 200-300 broken tests (likely broken for years). The tests are old and fragile and they're a mess of spaghetti dependencies that typically end with live sandbox data.

My goal is 100% passing tests so we can break the build on unit test failures, but I can't do it until I address the broken tests. I have very little budget because the maintenance budget is primarily for support, but my team has identified and fixed the low-hanging fruit tests (mostly config/local resource issues) and we're down to 30-40 really ugly tests.

What are some opinions on best practice? I don't think the tests are valuable, but I also don't know what they're testing or why they don't work without digging in, which takes time and money we probably don't have.

I'm thinking we should document the statuses of the broken tests with anything we know, then either delete or ignore the broken tests completely and enter a lower-priority bug/work item to investigate and fix them. We'll then be at 100% and start to get real value out of the other tests and if we have a maintenance/refactoring windfall we'll be able to pick them up again.

What would be the best approach?

Edit: I think this is a different question than this question because I have a clear direction for the tests that we should be writing going forward, but I inherited legacy failing tests to address before the large current set of tests becomes meaningful.

Answer 1

What I would do is first disable the tests which are failing-and-have-always-failed.

Make it so a test failing matters.

As you investigate you might be able to ask people who have been with your company longer about them, there might be a lot of tribal knowledge about them which you can document/capture. Maybe from your VCS logs. "Oh, that test has always failed since we upgraded to X" or other information can be useful.

Once you know what the functionality being tested was you can determine:

• Do we care about this being tested
• How important is this to be tested

And then make a priority list.

Likely nothing on this list is important enough to get more time later since it's been ignored for years already. So I wouldn't spend too much time/resources documenting and analyzing all these broken tests.

Answer 2

I would do the following:

1. Attempt to determine exactly what the failing tests are trying to validate.

2. Triage - if some tests are trying to test unimportant things like (an old) state of the world, delete them. If you realize that some of them are trying to validate something important, try to determine if those tests are doing so correctly. If they're testing incorrectly, make them test correctly.

3. Fix whatever is wrong with your production code now that you have good tests.

Remember accounting, every line of code is a liability, but may be incorrectly valued as an asset. The delete key can create a lot of value for your firm.

Answer 3

200-300 broken tests (likely broken for years).

Ouch! I faced a similar situation once but with like 7 tests failing where the team started ignoring the fact that they failed for months due to "always-crunch" mentality.

My goal is 100% passing tests so we can break the build on unit test failures, but I can't do it until I address the broken tests.

I was obsessed with a similar goal even though I was just a junior developer in the team because I was noticing a pile-up where more tests were failing over the months. I wanted us to turn those from "warnings" into build errors (perhaps somewhat obnoxiously to the rest of the team).

I'm thinking we should document the statuses of the broken tests with anything we know, then either delete or ignore the broken tests completely and enter a lower-priority bug/work item to investigate and fix them. We'll then be at 100% and start to get real value out of the other tests and if we have a maintenance/refactoring windfall we'll be able to pick them up again.

These are my thoughts as well. You can temporarily disable all these faulty tests and slowly visit them and fix them over time. It's important to schedule those fixes if you consider them really important though, even if they're low priority, since it's easy for such items to simply go unfixed. The priority to me is to make sure that no new tests are introduced that fail.

Like any kind of warnings, if they don't break the build, they tend to pile up quickly. This is assuming that kind of team dynamic where the habit of ignoring warnings (failed tests in this case) can quickly lead to more warnings being introduced, and reduce the temptation to keep those warnings to zero.

A very conscientious team might not suffer these problems and avoid introducing new warnings (new failures in tests), but it's definitely safer to go a bit more heavy-handed and exercise a prevention strategy by turning these into errors that must be fixed prior to a merge process.

So my suggestion is the same as yours (albeit only a strong opinion -- maybe somewhat can back this up with metrics and a more scientific answer). Disable those old tests, and put it on the schedule to eventually fix them. The first priority is to make sure that this problem doesn't pile up and start getting worse by making sure the tests that currently succeed won't ultimately end up being ignored if they start failing.

Answer 4

In a way you're lucky. It's better to have tests that fail and shouldn't (they give you a warning at least that something's wrong) than to have tests that pass and shouldn't (which give you a false sense of security).
Of course if you have the former it's quite likely you have the latter as well (so tests that pass but should be failing).

As stated already, for now disable those failing tests but have them print a message in your test log as a constant reminder about them.
But you should definitely find the resources to go over your entire test suite in order to find and weed out the tests that pass and shouldn't, because each of them means there's a bug in your code that you're currently not detecting in your test cycles.

Using that dark cloud hanging over the code base you might well be able to get some budget for a full review of your tests, if you play it right and don't just tell them you think some tests should be looked at because they seem to fail when they shouldn't, but that you don't trust that your tests are detecting errors in your code properly, that the test set can't be trusted to do its job.
When I did that at a previous company I was working for such a review found that hundreds of tests were written with incorrect assumptions about what the code SHOULD do, leading to the code (which was written using the same incorrect assumptions) passed the tests when really it should not have. Fixing this solved many a nasty corner case bug that (while most weren't critical) could have brought down some important systems.

Answer 5

Any failing unit test should cause the build to be broken. Good on you for realizing it and setting that goal. The human mind can hardly ignore anything more completely than the source of a persistent false alarm.

Throw these tests away and don't look back. If they have been failing for years and haven't been addressed by now then they are not a priority.

As for tribal knowledge, if the people with the tribal knowledge are still around they should have fixed the failing tests by now. If not, then again, these are not a priority.

If there is no tribal knowledge, you and your team have to take ownership of the logic. The failing tests may be more misleading than helpful - the world may have moved on.

Create new tests that are relevant and get on with writing great code.