13

While there are ways of keeping unit tests from being executed, what is the value of checking in failing unit tests?

I will use a simple example: Case Sensitivity. The current code is case sensitive. A valid input into the method is "Cat" and it would return an enum of Animal.Cat. However, the desired functionality of the method should not be case sensitive. So if the method described was passed "cat" it could possibly return something like Animal.Null instead of Animal.Cat and the unit test would fail. Though a simple code change would make this work, a more complex issue may take weeks to fix, but identifying the bug with a unit test could be a less complex task.

The application currently being analyzed has 4 years of code that "works". However, recent discussions regarding unit tests have found flaws in the code. Some just need explicit implementation documentation (ex. case sensitive or not), or code that does not execute the bug based on how it is currently called. But unit tests can be created executing specific scenarios that will cause the bug to be seen and are valid inputs.

What is the value of checking in unit tests that exercise the bug until someone can get around to fixing the code?

Should this unit test be flagged with ignore, priority, category etc, to determine whether a build was successful based on tests executed? Eventually the unit test should be created to execute the code once someone fixes it.

On one hand it shows that identified bugs have not been fixed. On the other, there could be hundreds of failed unit tests showing up in the logs and weeding through the ones that should fail vs. failures due to a code check-in would be difficult to find.

2
  • That's one way to increase those test coverage numbers.
    – JeffO
    Commented Jan 19, 2016 at 20:33
  • If you already put in the effort to write the unit test, why would you want to have to rewrite it when you do decide to fix the issue? Just because it is checked in, does not mean that it has to be run in the suite. (You could make a category for "Known Issues", and treat those tests as a backlog/TODO list.)
    – Caleb
    Commented Jan 20, 2016 at 2:20

8 Answers 8

Reset to default
18

I do not like broken unittests checked in because they produce unnecessary noise. After every unittest i would have to check all failed issues(red). Is it red because there is a new problem or because there is an old open to do/to fix. This is not okay if there are more than 20 unittests.

Instead I use

  • [Ignore("Reason")] attribute that makes the result yellow or
  • throw new NotImplementedException() that makes the result grey

Note: I am using NUnit for .net. I am not sure if the "gray" feature is there in other unittest frameworks.

So I like the following meaning of unit test-results.

  • green : all finished
  • gray : planned new features that need to be done but with low priority
  • yellow : bugs not fixed yet. Should be fixed soon
  • red : new bugs. Should be fixed immediately

Everything except "red" can be checked in.

To answer the question: there is more harm than value to check in "red-failed-testes" but checking in "yellow-ignored-tests" or "grey-NotImplementedYet-tests" can be usefull as a todo-list.

2
  • the problem I see with this approach is, that the ignored tests will probably never be fixed. You could also just remove the whole test code, what would be the difference (I'm being a bit cocky here)
    – Lovis
    Commented Jan 19, 2016 at 14:52
  • 4
    will probably never be fixed is a political decision if you want to spend efford in automated tests or not. With "ignored tests" you have the chance to fix them. Throwing away "ignored tests" means "abandon automated tests by and by until there are no more "
    – k3b
    Commented Jan 19, 2016 at 15:05
10

I won't pretend that this is industry standard, but I check in broken tests as a way of reminding me or my other project members that there is still a problem with either the code or the unit test itself.

I suppose one thing to consider is whether your development policies allow for failed tests without penalty. I have a friend who works at a shop that does test-driven development, so they always start out with failing tests...

6
  • 6
    But you should never check in a failing test, as your build server shouldn't build a project with a broken test.
    – CaffGeek
    Commented Mar 15, 2011 at 15:20
  • @Chad: Building and testing are two separate pieces of one automated steps. Building ensures that everything compiles. Test ensures that the result of the build is valid. My interpretation of the question was not, "should I check in code that doesn't compile?" Instead it was, "should I check in a test I know will fail?"
    – unholysampler
    Commented Mar 15, 2011 at 15:33
  • 2
    I was just adding a point to consider, some continuous integration build servers run the tests, and if they fail, they don't get deployed. Rightfully, as if the build fails, the code fails, and there is no point in deploying a product known to be broken.
    – CaffGeek
    Commented Mar 15, 2011 at 15:40
  • @Chad: Right, I totally forgot about CI servers. That would definitely be a point to consider. It is also worth clarifying what we mean by "broken" tests; are they just plain "bad" tests, or is the test failing because the API changed in some way?
    – Tieson T.
    Commented Mar 15, 2011 at 15:58
  • 1
    I know this is old but I gotta say, the point of checking in a failing test is to keep the build from building. It's something a peer reviewer can do to show you what is wrong. The person who wrote a failing test doesn't have to be the same as the one who satisfies it. But this code had better not be headed to production until the test either passes or is deleted.
    – candied_orange
    Commented Jun 15, 2023 at 18:20
7

The purpose of unit tests is to assert the expected behavior of a system, not to document defects. If we use unit tests to document defects, then the usefulness of them to assert expected behavior is lessened. The answer to the question "Why did this test fail?" is not a simple "Oh, something is broken that I didn't expect to be broken." It has become unknown whether the test failure is expected or unexpected.

Here is a paragraph from the beginning of chapter 13 of Working Effectively with Legacy Code:

Automated unit tests are a very important tool, but not for bug finding--not directly, at least. In general, automated tests should specify a goal that we'd like to fulfill or attempt to preserve behavior that is already there. In the natural flow of development, tests that specify become tests that preserve. You will find bugs, but usually not the first time that a test is run. You find bugs in later runs when you change behavior that you didn't expect to.

6

Failing unit tests give the development team visibility into what must be done to conform to the agreed upon specifications.

In short, failing unit tests give the team a "TODO" list.

For this reason, failing unit tests are far better than no unit tests at all.*
The absence of unit tests leaves the development team in the dark; specifications must be repeatedly confirmed manually.

[*Provided that the unit tests actually test something useful.]

1
  • 2
    There are better ways to maintain a todo list, e.g. a whiteboard, a to do list app, or an issue tracking system. It's much easier to use a test suite if you expect it to always pass fully, and any test failure that appears is then a new problem to fix immediately.
    – bdsl
    Commented Mar 27, 2018 at 21:21
3

But the broken ones that identify bugs in a new project, named as such. That way you can see that they SHOULD break... As they are being fixed, they would be turned green, and moved into the normal test suite.

NOTE: That project would have to be set to not be built on the build server, if your build server prevents checkins that break the build (assuming you define a broken build as one in which all tests don't pass)

2
  • +1 although there is no answer if to checkin or not there is an important argument: build server
    – k3b
    Commented Mar 16, 2011 at 6:21
  • I'd rather use an attribute to mark such a test instead of moving it to a separate project.
    – CodesInChaos
    Commented Jan 19, 2016 at 15:05
2

Unit tests should test for error cases in addition to success cases of a function. A function should explicitly reject bad input or should have documentation to explain what input is considered valid.

If you have a function that is not doing either of those things, it is a bug and you should have a way of recording that it exists. Creating a unit test that demonstrates this issue is one way to do that. Filing a bug ticket is another option.

The point of unit testing is not to have 100% success, the point is to find and fix bugs in the code. Not making tests is an easy way to have 100% success, but that is not very beneficial to the project.

6
  • Woah... "The point of unit testing is not to have 100% success", are you saying they don't all need to pass!?
    – CaffGeek
    Commented Mar 15, 2011 at 16:01
  • 2
    @Chad: The point is that it is better to have tests that you know will fail, but are demonstrating a real issue instead of not having the test just so you can have the green check mark at the end of the nightly build/test.
    – unholysampler
    Commented Mar 15, 2011 at 17:03
  • 8
    @unholysampler, never have broken tests, unless they are CLEARLY marked as "should" break (by being in a different project). Otherwise, they become noise, and you don't know when a test that should be passing, has been broken. It completely defeats the purpose of Continuous Integration and having UnitTests
    – CaffGeek
    Commented Mar 15, 2011 at 17:18
  • 2
    @Chad: I think this is getting into semantics of definitions. Based on the OP, it sounded like he was talking about creating a valid test that exercises a bug. However, the bug is low priority and not likely to to be fixed immediately. You are the one that brought up Continuous Integration, which puts much stricter restrictions on the automated process.
    – unholysampler
    Commented Mar 15, 2011 at 17:33
  • 4
    @unholysampler, CI, or no CI, the point is, when you run the tests, and are used to seeing some Red lights, you become accustomed to it. So, when something that was green, turns red...how do you know?!? It's a horrible practice, and one of the reasons testing fails to be accepted in so many organizations.
    – CaffGeek
    Commented Mar 15, 2011 at 17:36
1

File a bug for every failure, and note that in the test result. Then if you ever get your act together and fix the bug, your test passes and you delete that from the test result. Never just ignore problems.

-3

How I see TDD done with implementing tests for an unfinished code is, write tests first with [ExpectedException] attribute or similar. This should pass initially as the incomplete code would not have any logic in it and write a throw new Exception() code in it. Though throughing exception is wrong, this would atleast make the tests pass initially and fit for checkin. We may forget an ignored test but may definitely tidy up or fill the incomplete code. When we do so, automatically the corresponding test that was expecting an excpetion would now start failing and alarm you to fix it. This may involve a slight alteration of test to get rid of ExpectException and instead do real asserts. CI, Dev, testers and customers all happy and a win-win situation?

1
  • 1
    This does not answer question. Its not asking what is TDD and why test for expected exceptions.
    – Andy Wiesendanger
    Commented Jan 28, 2016 at 16:36