views:

1306

answers:

21

In the last two companies I've been at, I have found an overriding mentality among developers that it's okay to write unit tests in a throw-away style. Code that they would never write in the actual product suddenly becomes OK in the unit tests. I'm talking

  • Rampant copying and pasting between tests
  • Code styling rules not followed
  • Hard-coded magic strings across tests
  • No object-oriented thought or design for integration tests, mocks or helper objects (250 line single-function tests!)

.. and so on. In my current position, I'm highly dissatisfied with the quality of the test code. Generally we do not do code reviews on our test assemblies, and we also do not enforce style or code analysis of them on our build server.

Is those options the only way to overcome this inertia about test quality? I'm looking for ideas to take to our developers, without having to go to higher management saying that we need to use resources for enforcement of test quality (although I will if I have to). Any thoughts or similar experiences?


EDIT: While I value and respect the dissenting viewpoints, I don't think I'll be convinced that test code should be held to a unique standard. I don't think developers should be ignoring the fundamentals of object-oriented programming at any time. Tests are important parts of the product, and are required in the application lifecycle for telling the story of what the code should be doing. While the application of the test code might be fundamentally different, I'm unconvinced that that should allow for what I perceive as sloppier coding.

+8  A: 

Can you pair program for an hour or two with one developer who you think might be sympathetic to your cause?

During the session, make sure you refactor some code, and of course, make sure you do the same with some of the test code while you are there. You'll have an ally, and you can then spread the word, one developer at a time.

If you go to management, it's probably going to become an us and them situation, but if you try winning around people one at a time, it will happen naturally.

Good luck!

Paddyslacker
+7  A: 

Sounds like the tests are written as an afterthought. Make sure the developers are writing their tests before coding a new module. They need to use the tests during development as a tool for debugging.

Byron Whitlock
TDD FTW, IMO TDD really leads to better tests. BTW, Sorry I tried to cram a metric crap ton of TLA's in this comment :)
Allen
@allen: +1 for successfully stringing together 4 acronyms ;)
RCIX
OMG WTF BBQ drat it's harder than it looks. :-D
roufamatic
OMG, ROFL. IMHO, FWIW: TDD/BDD FTW. KISS.
Epaga
+9  A: 

Have a different developers write the tests for the others code. This way they both have "real" code they will have to show as their own.

Glennular
This could leverage the competitiveness that most developers have. It's a good idea at least in theory. I'd love to hear some real-life case studies of this being put into practice.
Adam Crossland
Now this - is an interesting concept. We have dedicated QA people, at least a couple of which are coders. I'm going to ponder this - it isn't out of reality that we could get someone assigned to a project during development to be dedicated to writing test code alongside the feature developers.
womp
+1  A: 

What can you do about it? Unless you are the party responsible to handle coding style, you can't really do much about it. You need the support of developers with influance.

I'm in the camp of "at you you do have unit tests." I've been trying to work to get some form of automated testing setup and while everyone agrees its a good idea, no one will work towards it.

The best thing you can do is talk it up and try to illustrate why it's good by doing it yourself. Set the example and hopefully others will find the importance of it on their own.

Frank V
+2  A: 

The only way I've seen unit test code look good is where the point of view is that every bit of code in the repository is production code, even if it is unit tests, and it all follows the same standards. Coding standards and code reviews apply to unit tests as well as product code.

tloach
+16  A: 

You semi answered your own question.

Generally we do not do code reviews on our test assemblies

Perform a code review. Kick back if the tests are not up to scratch. Eventually developers will be forced to treat test code as a first class citizen.

Finglas
Agreed. Automatic (build server) and "manual" (code review) enforcement ramps up the quality on our production code for sure, and over time, all the developers code at a much higher quality level naturally. But why do they get the attitude that it's still ok to write crap for tests? This may be the only way... but I'm looking for other ideas too.
womp
I have no idea. Once you get the 'testing bug' via TDD for example, your test code is equally as important. There's no two ways about it. If they are unaware of the benefit of writing tests, or unaware of the benefit of writing **good** tests, you should demonstrate why. Some people need first hand evidence of a best practice, rather than simply having someone tell them to do it.
Finglas
Good luck with this, its damned hard enough as it is trying to get people to spend the time to write unit tests in the first place
Allen
+5  A: 

I'm in the camp that unit test code should be held to a similar standard as your production code. We use PMD (others prefer FindBugs, I know) rulesets and run them against both source and test code at build time with Maven. We also run Cobertura, and developers are supposed to come to code reviews with close to 100% test-coverage in Cobertura and as few PMD errors as possible.

We use Hudson for our CI too, and I really like their Cobertura graphs & source views to see how well code is covered. We also regularly take a look at the PMD reports generated from Maven to see if there are potential problem classes. Granted, PMD errors don't bother me quite as much in test code, but we're trying to have everyone committed to standards across all the code we write.

I'd also add that I think it helps to have a strong developer or two set up some basic architecture for the tests early on. Then, going forward, if those 'testing SMEs' recognize that most unit tests needs some common utility methods, they could be responsible for helping to place those common methods in an appropriate place and refactoring existing code to use them.

elduff
+1  A: 

If you want a half-way solution, then consider non-enforced code analysis on your CI server for reporting purposes only. Make these stats glaringly obvious and public. When your tests have very few violations and theirs has hundreds, things will change by themselves over time. And if not, slowly add importance to those numbers until they actually are enforced.

If you want an immediate solution, then go with Finglas's suggestion and review the tests and fail them if they're not on the up and up.

Jaxidian
+24  A: 

To a certain extent, I disagree with your premise. I gather from the other responses that this viewpoint is not going to be popular, but I would argue that test code is NOT real code, at least not in the sense that product code is real code. Let me explain what I mean.

1) Your product code has to work every time, needs to be maintainable and extensible, and should not result in end-users seeing problems or discrepancies. Your test code is the opposite in every way: no end users will ever see it, and individual test cases should NOT be extended. They should be duplicated and run as new tests. The developers who run the unit tests should know immediately if there are problems or test cases that need to be updated based on changes, and should fix those as part of their development cycle.

2) Your product code should be designed to have as little code repetition as possible. Your test cases by definition have to have a lot of repetition. Each test case has to be run once with a known valid value, a known invalid value, edge cases, etc. You can't eliminate the repetition, because the repetition is part of what you're doing in the tests.

So it seems to me that because test code has A) a different audience, B) a different purpose, C) different usage scenarios, that it's appropriate for it to be written using different standards.

To back up the idea that there are different standards for different types of code, I could refer to Rapid Development, by Steve McConnell, where his effort estimates for in-house code and for shrink-wrap code are very different. I would argue that by its nature, test code is always "in-house" code.

jwismar
I +1'd you because I sort of agree with you. However, if their horrible unit test code is causing people to loose significant amount of time trying to figure out what went wrong, then that does impact the bottom line
Allen
An interesting viewpoint... but just because tests are repetitive doesn't mean you should copy and paste the code for each repetition. What if you have say, 10 tests around something that uses a session key - if you hardcode the session key, now I have to change 10 tests when the key changes in production, instead of just changing the key in the resource file of the test project.
womp
That example is not hypothetical, btw.. :(
womp
But alas you forget about Open Source project in which having decent test code is not completely "in-house" as it may be shared with some end users as well. If the test cases are not written well, duplicates may easily be created and while that is not really a huge deal, I always hate reinventing the wheel when I don't have to.
galford13x
+1 I totally agree with you. The question is similar to - "how to convince people to write prototypes in respect to coding standards". Different requirements, different standards. BTW @womp - it can happen in large projects that a test code of core modules can become a critical and extensively used (eg. read) code, as it can happen with a sample application. In this case additional standards should be enforced. This is not a general rule though.
agsamek
If test code does not work every time, developers will start to ignore test failures on the build server. "Oh, that's probably a data error." If test code is difficult to maintain, developers won't update it and will comment it out if it breaks. I've seen this happen.
TrueWill
As Kent Beck put it, unit-tests are documentation. While I do not completely agree with that, I do think that by looking at the tests for a class, one should be able to understand how the class is intended to be used.
Space_C0wb0y
@TrueWill - I said devs need to fix the unit tests when they make changes - otherwise, how do they know when they're done? IOW, I agree with you.@Womp - I guess I'm saying that the level of acceptable cut-and-paste is different. When it starts to get painful, then pull it into a shared resource, or at least make it easier to change. But my point is that the threshold is different between test code and product code.@galford13x - @Space_C0wb0y mentions this too: as long as it's meeting its documentation and testing tasks, I'm willing to allow it different standards than the product code.
jwismar
Bad code is bad code. It doesn't matter where you put it, or who you "release" it to, it simply won't do its job properly. Code always needs to be well written, well documented, maintainable and fit for purpose, or it will fail (not validate the code correctly) or be inefficient (cost unnecessary developer time). A parallel example: the shoddy batch scripts people hack together to automate builds etc - they are usually a nightmare to maintain because people don't take the time to write them to be robust or maintainable. Just do it right, once - it's less work in the long run.
Jason Williams
Totally disagree with this mentality. It will get you through a 6 month project but not a 4 year project. Eventually the cost of maintaining tests this way will exceed the benefit of writing them at all.
roufamatic
Product code and test code have different requirements. But that doesn't mean that as code they should have different standards. Duplication in test code is not desirable. If there is common setup then factor that into a helper method. The tests themselves shoudl be unique. If you are copying/pasting that's a sign that the tests need some more thought.
Skeets
I both agree, and disagree - it depends on the type of tests you're writing. Typical "exercise the code" tests? I consider them a bit more disposable, as they tend to be fragile anyway. But good "unit" tests are not - they are more akin to executable specifications, and as such should be considered one half of the actual code written, along with the product code.
kyoryu
The more difficult something is to change, the more difficult it is to change what depends upon it. This is as true for production code "depending" on successfull execution of brittle suits of unit tests as for normal code. Maintain your professional attitude towards the test code. Avoid duplication.
Tormod
I agree with this to a point. Tests have a high tendency to be procedural. It is only natural that test code will look much more procedural than object-oriented. If you've been working with object-oriented code for a long time, procedural code may look bad at first glance, but there is nothing inherently wrong with the style. However regardless of the style there's no excuse for bad code or ignoring object-oriented techniques where they make sense.
JamesH
+2  A: 

Are you running your tests against your codebase every time your continuous integration engine (Hudson, CruiseControl, etc) builds your code?

It made a big difference for us, 1 dev would break another dev's test and immediately go to it and realize that it was impossible to figure out what they were trying to test. They'd have to go talk to that developer and he would essentially be shamed into doing it better the next time.

Allen
+1  A: 

Its been my experience that a lot of the bad habits regarding sloppy code in unit tests flow from the fact that many of the "unit tests" being written are what are described here as hybrids, basically tests that are neither pure unit nor integration tests and are too tightly coupled to a particular implementation. Since these tests are frequently abandoned anyway no-one invests much time in coding them up to normal standards. If this can be addressed, then the coding quality seems to follow naturally.

kekekela
+2  A: 

Test code is repetitive because tests are repetitive.

You're testing something like a repository. What tests do you need to run?

  • Add a valid item, then ensure it was added.
  • Add an invalid item, ensure you get a validation error.
  • Add a valid item, remove it, ensure the item was added and removed.
  • Add a valid item, then add a duplicate item, ensure you get an error.
  • Add a valid item, then modify it, ensure the modification succeeded.
  • Add a valid item, then modify it to an invalid state, ensure you get an error.
  • Add a valid item, then modify zzzzxcvbasdf... sorry, kinda dozed off for a sec.

It doesn't get more repetitive than this. That's why we write automated tests in the first place! And yes, there's going to be magic values. Why shouldn't there be? We're testing one valid modification and one invalid modification, each of those values is only ever going to be used once. Sure, stash it in a const or static readonly field somewhere if you want, but that doesn't change what it is, and IMO doesn't necessarily lead to much more readable/maintainable test code.

Obviously if your individual test cases are stretching into the tens or hundreds of lines and it's mostly copypasta then you have a problem, but I don't think I've seen this. And I think that a series of tests that are all 5-10 lines of very similar-looking code with the occasional magic value thrown in is not just okay, it's inevitable. Even if you refactor like crazy and end up with a series of coarse "test actions" from which every test can be built, you'll still end up writing a lot of repetitive code, because you're constantly repeating the same actions but in slightly different orders or with slightly different values.

Tests by nature deal with the validation of specific inputs and outputs, whereas the program it tests is designed to solve a general problem. When you're writing code to solve a general problem, it's a lot easier to write clean abstractions.

Maybe I just haven't seen some of the tests that you've seen, maybe they're a lot worse than I'm imagining, but as it is, I'm really not sure that I see the problem. Even when I look at unit tests for well-known libraries, I find that they tend to be pretty repetitive, they could easily be the result of a lot of copying and pasting.

Perhaps an example of good test code vs. bad test code would help? Perhaps what you need to do is make it more explicit and clear where you draw the line, what you consider to be good test code and bad test code. I tend to be pretty hardcore about clean, readable code, and if I am having trouble seeing exactly what you mean here, a junior dev with 3 months of C# under his belt doesn't stand a chance.

Aaronaught
Again, I would have to disagree with this. The problem I am facing is that in your example above, say it takes 5 lines of code to construct a valid item. Rather than do that in the [SetUp] method, I'm faced with the same five lines of code copied to all the tests. You will never convince me that this is OK.
womp
@womp: I agree, if you're rewriting the same 5 lines of code to construct the *exact same item* repeatedly then it should be in its own method or refactored in some other way, but that's not at all clear from your question. So again I point to the fact that if people don't know *exactly* what it is you have a problem with, they're going to have a hard time fixing it. Your question was "how do I get developers to do *X*"; the first step is letting them know what *X* **is**.
Aaronaught
My question was "How do I get developers to stop writing bad test code". X = bad test code. I'm pretty sure that's clear in the question. I also gave examples of specific things that developers were resorting to that produces said code. Do you *really* need specific examples of what constitutes bad code? It would detract from the aim of the question, since people would just be analyzing those specific examples.
womp
@womp: Yes, I am *really* suggesting specific examples because *test code is not the same as production code*. If you're happy with the other answers then feel free to ignore mine. As far as I see, *all* manner of code has *some* standards, but if you want people to follow them then you need to *communicate them clearly*. What is your standard for good test code? "Copy and paste" and/or "magic numbers" can mean many, many different things in practice.
Aaronaught
Just because a test is repetitive, doesn't mean the code has to be duplicated. And once you make a constant out of a magic value, it isn't a magic value anymore.
Chris Knight
+1  A: 

I agree that test code should be written well. It doesn't have to be "elegant" but it should be good. One reason for this is production support. Unit tests are very useful for production support people to be more confident with what they do. This is especially true if the people in production support are not the same developers who developed the original code.

So maybe you should approach it by explaining the importance of quality test code. Most developers are rational and logical, and many of us have been on production support at one time or another and know how hellish it can be. If you explain how good test code helps everyone, not just the current developer, they may see the wisdom in doing it right.

I don't think code reads and stuff are the right approach because developer time is so valuable that it is hard to justify spending time code reading test code (I realize this kind of goes against what I said about test code being important).

Anyway, good luck, I hope you get the problem fixed.

Marshall Alsup
+12  A: 

First, understand the root cause:

Refactoring test code is hard.

It's really a hard problem. You have a lot of duplication because a number of tests frequently vary by one or two statements. Sometimes the differences lead to obvious refactorings, but other times they really don't.

When it's not obvious, it can be hard to justify spending the time to rearchitect them. After all, the tests aren't the product, and they provide adequate support to the actual product, so why bother? Plus there are few examples in the wild of really well-designed test code, so it's hard to be sure you're doing the right thing.

However, coders can be influenced. Show them how it will make THEIR lives better and they'll make the change.

I highly recommend Gerard Meszaros's xUnit Test Patterns as a starting point for discussion. It's a giant book, but it's well-organized into two sections: a short and straightforward exposition on the why's and how's of test refactoring, followed by a large catalog of test patterns and smells in encyclopedia format.

This book provides the ammunition you seek. First, it gives you the vocabulary you need to talk about the problems. Second, and more importantly, it gives you a way to provide examples and boilerplate code to your developers so they can see what they're missing. Once they see the benefits and the way forward, your coders are going to want to improve their code.

roufamatic
+1 for your insightful comments about examples in the wild and vocabulary needed. And from the excerpts, this book looks like it could be a jackpot. I've seen this book before but have never picked it up. Thanks!
womp
Just curious, how did this turn out?
roufamatic
+2  A: 

I actually like to keep test code pretty low-level and simple (sometimes repetitive). If you start to build out complex methods or frameworks to setup tests, and manipulate test objects outside your "production" code, you may end up with bugs in your test code.

I think if you're seeing a lot of repetition and copying and pasting in the test code, it might mean that you could improve your production code to better handle different test cases. It could also mean that you have repetitive tests that essentially test the same functionality over and over.

Andy White
I think your point about keeping tests simple is super-important. Tests should be simpler than what they're testing. Otherwise when a test fails the defect is more likely to be in the test than in the thing tested, and developers stop paying attention to the tests.
Nathan Hughes
+2  A: 

For trivial code duplication, you could look into using row tests, eg:

[Test]
[Row(1,1,2)]
[Row(2,2,4)]
public void TestAddOperator(int a, int b, int expected)
{
    Assert.AreEqual(expected, a + b);
}

A side effect is that your magic constants move out of the test body, and get names by default.

Above is using a framework such as MbUnit. I'm sure there are equivalents for other languages/testing frameworks.

Douglas
+3  A: 

The main key is to set things up so that good practices will arise organically.

First, enforce that unit tests be run frequently and automatically, and that they are fixed when they fail.

As people experience the pain of having to maintain them, if you have a smart team that thinks about such things, the right set of practices will emerge. For example, the copy-pasting will become a problem if people have to go fix many tests every time something in the code changes, and things will be refactored accordingly.

Magic numbers will be slowly rooted out when people need to use the same numbers from other tests, or when people find the test code unreadable and unmaintainable.

Code review is also a big help for this - some of this organic improvement will happen via code review. Most fundamentally, enforce that the tests are written when the main code is checked in. This gives code reviewers an opportunity to get their hooks into the test code as well.

If you have a code review tool, tweak it so that it surfaces the code review of the test code more prominently, perhaps by having a field in the form that mentions "test code review" separately - or if the test code is checked in in a separate diff, then set up some commit barrier that requires code review. If your tools aren't this fancy, then just start asking to review peoples' test changes and see if the practice spreads. Or write a tool (but that tool had better directly improve peoples' productivity as well, or people won't want to use it)

Basically, you've got to set up the tools/process so that people fall naturally into doing the right things. Don't let the old code bother you until you need to change it. The best sign of a good team is not necessarily that all the code is great, it's that the more recent the code is, the better it is.

Drew Hoskins
A: 

Code quality is important because it makes it cheaper and easier to maintain code.

If your test code quality is lacking, a potential reason is that the developers do not have to maintain that code.

Solution: Hold them accountable for tests, keeping them current, and false failures.

kyoryu
+5  A: 

You mentioned that tests aren't subject to code review. That is a mistake.

Since tests are not subject to code review, I'm guessing that they are also not subject to having bugs filed against them, QA, engineering planning, architectural review, etc.

Each line of test code should be subjected to the same quality standards as the actual product code. Certainly you'll let a few lines of questionable product code slide through if the rationale and alternatives support allowing them. The same goes for testing code, occasionally a few sub standard tests will be permitted, but under no circumstance should tests be sub standard by default.

Edwin Buck
+1  A: 

You need tests that demonstrate how test code should be written in the first place. You can set an example by writing refactored, easily readable and maintainable tests dispite the fact that most of the test code is a giant 'broken window'.

When you lead with good style, good developers will follow. If someone has to modify your test he or she will follow your style. I very rarely find someone 'pooping' in my test code. What I find is that they start caring about the quality of the test code themselves.

I personally would not go to management. Maybe its only my company, but they do not understand why we write tests in the first place. You might find yourself in a political battle where you will likely to lose.

c_maker
A: 

Comes down to discipline, correcting bad habits, having a good way of TDD to begin with. Every team member has to have buy in with unit testing, test driven. Also for code style probably someone already mentioned stylecop... and for correcting bad habits this catches some of them $1 in the broken build jar (style cop fails, a test fails, build fails).

I'm pretty hardcore about focusing on test driven, so in fact the standard run with debugger key in visual studio instead launches one of two rake commands: test:feature[highlightedCucumberTag] or test:class[selectedSpecification].

Our specifications are quite small because everything is a well defined unit. We do our best to avoid hard dependencies. So there is a c# file for each behaviour / scenario, and we use an add-in for visual studio to set the "depends on" of the Whens to the base specifcation such as:

Designer.Tests.Model.Observers.... Contained in folder Designer\Models\Observers

ObserverSpecification.cs --->When_diffuser_observer_is_created.cs --->[It] public void Should_return_the_expected_diffuser() ---->When_diffuser_observer_is_modified.cs --->[It] public void Should_raise_property_notification_for_diffuser()

Nice way of organizing everything.

Here's a sample specificaiton:

using MavenThought.Commons.Testing;
using SharpTestsEx;

namespace Designer.Tests.Model.Observers
{
    /// <summary>
    /// Specification when diffuser observer is created
    /// </summary>
    [ConstructorSpecification]
    public class When_diffuser_observer_is_created
        : DiffuserObserverSpecification
    {
        /// <summary>
        /// Checks the diffuser injection
        /// </summary>
        [It]
        public void Should_return_the_injected_diffuser()
        {
            Sut.Diffuser.Should().Be.SameInstanceAs(this.ConcreteDiffuser);
        }
    }
}

and in this one we had to use a concrete diffuser because Diffuser is a core / domain object and hence we do not put property notification on it.

using System.ComponentModel;
using MavenThought.Commons.Testing;
using SharpTestsEx;

namespace Designer.Tests.Model.Observers
{
    /// <summary>
    /// Specification when diffuser observer has the injected diffuser change
    /// </summary>
    [Specification]
    public class When_diffuser_observer_has_diffuser_property_change
        : DiffuserObserverSpecification
    {
        /// <summary>
        /// Listen to the property change of the SUT
        /// </summary>
        private PropertyChangedEventHandler _mockPropertyChangeHandler;

        /// <summary>
        /// Checks the diffuser injection
        /// </summary>
        [It]
        public void Should_have_diffuser_changed()
        {
            this._mockPropertyChangeHandler.AssertPropertyChangedWasCalled(this.Sut, o => o.Diffuser.SupplyAirVolume);
        }

        /// <summary>
        /// Mock up the property change handler
        /// </summary>
        protected override void GivenThat()
        {
            base.GivenThat();

            _mockPropertyChangeHandler = MockIt(this._mockPropertyChangeHandler);
        }

        /// <summary>
        /// Listen to property change of SUT
        /// </summary>
        protected override void AndGivenThatAfterCreated()
        {
            base.AndGivenThatAfterCreated();

            Sut.PropertyChanged += _mockPropertyChangeHandler;
        }

        /// <summary>
        /// Initialize the SUT with diffuser
        /// </summary>
        protected override void WhenIRun()
        {
            ConcreteDiffuser.SupplyAirVolume = 5;
        }
    }
}

Takes some discipline for practice, however, we have nothing even near 250 line methods for any one of our specifications.

Sean B