Tapestry Training -- From The Source

Let me help you get your team up to speed in Tapestry ... fast. Visit howardlewisship.com for details on training, mentoring and support!

Wednesday, February 20, 2008

Unit Testing: Crisis of Faith

I'm hitting a bit of a quandary with response to testing: I'm losing some faith in unit testing, or at least busywork unit testing relative to integration testing.

At issue is the way Tapestry is constructed; there's a lot of small moving parts that work together (integrate together might give you some foreshadowing). Sometimes testing the small pieces won't accomplish much. Case in point:

public class SecureWorker implements ComponentClassTransformWorker
{
    public void transform(ClassTransformation transformation, MutableComponentModel model)
    {
        Secure secure = transformation.getAnnotation(Secure.class);

        if (secure != null)
            model.setMeta(TapestryConstants.SECURE_PAGE, "true");
    }
}

This piece of code looks for the @Secure annotation on a component class, and records a bit of meta data that will be used elsewhere.

In the past I've written a chunk of tests even for something as simple as snippet; Following that pattern, I'd write two tests: 1) what if the annotation is missing? 2) what if the annotation is present? I'd use mock objects to ensure that the correct methods of the correct dependencies are invoked in each scenario. However, to write those tests, even leveraging existing base classes to manage the mock objects I'd need, would require several times as much test code as the production code I want to test.

But what would that prove? This bit of code will be useless without a large number of other pieces yet to be written: code that uses that meta data and forces the use of secure HTTPS links and so forth. Code that integrates this worker into the chain of command for processing component classes. Code that verifies that requests for pages that are marked as secure are, in fact, secure.

In other words, to verify the behavior I need to validate that all of those pieces are working together, that they are all integrated.

That doesn't mean I'm calling for the abandonment of unit tests. What I'm saying is that pointless unit tests don't accomplish much. I've found that unit tests are invaluable for testing edge cases and error conditions. Even the most rabid test driven pundits don't expect you to unit test your accessor methods ... I think tiny classes like this SecureWorker fall under the same general umbrella. I intend to focus my efforts on the integration tests that will encompass this code in concert with other code. And maybe write a few unit tests along the way, as long as they actually have value.

Update: It's not that I can't write the test, it's that doing so is not always worth the effort. Here's a similar test I wrote in the past:

public class SupportsInformalParametersWorkerTest extends InternalBaseTestCase
{

    @Test
    public void annotation_present()
    {
        ClassTransformation ct = mockClassTransformation();
        MutableComponentModel model = mockMutableComponentModel();
        SupportsInformalParameters annotation = newMock(SupportsInformalParameters.class);

        train_getAnnotation(ct, SupportsInformalParameters.class, annotation);
        model.enableSupportsInformalParameters();

        replay();

        new SupportsInformalParametersWorker().transform(ct, model);

        verify();
    }

    @Test
    public void annotation_missing()
    {
        ClassTransformation ct = mockClassTransformation();
        MutableComponentModel model = mockMutableComponentModel();

        train_getAnnotation(ct, SupportsInformalParameters.class, null);

        replay();

        new SupportsInformalParametersWorker().transform(ct, model);

        verify();

    }
}

Great: this detects whether @SupportsInformalParameters is on the class ... but I have other integration tests that actually stress the behavior when the annotation is present and when the annotation is absent. Ultimately, that tests both of these cases, and the code that ensures that this code is even executed, and the desired behavior of the component (with and without the annotation). Failures are unlikely to occur in this code, but likely in other code. What is the value of this test? Not even code coverage, and the integration tests hit both cases already.

16 comments:

Danno said...

I won't abandon unit-test cases even for inane code. I can honestly say that I can easily goof one line of code, or worse someone else can. ;)

Danno

Howard said...

My point is that every bit of code should have value.

In some cases, a unit test will pinpoint a broken bit of code. I know those have helped in the path, identifying the difference between = and !=. However, I often worked backwards from a failed integration test to find the improper unit test (where the unit test reflected the code as written, rather than the correct algorithm).

So I'm in favor of unit tests that will help me find and correct code errors, but I maintain that for a piece of code as in my example whose function is both a small portion of, and isolated from, the system behavior I'm targetting, a unit test has virtually no value.

William said...

This is a pretty typical reaction as a developer gets deeper into testing. You're absolutely right to push the envelope and see where you're truly getting value from testing. Mindless testing benefits nobody.

Four things things to watch for at this stage:

First, keep an eye out for "duh" bugs that creep back in. One of the benefits of seemingly dumb tests is that they can save you from truly dumb mistakes. More forehead slapping means the pendulum has swung too far.

Second, overly indirect testing can make it hard to find bugs. If you find yourself spending a lot of time to run down mysteriously broken integration tests, consider whether more unit tests would save time over the long haul.

Third, don't forget that testing has documentary value. If you're tempted to document a chunk of code, consider writing a readable test instead. Then the computer can help make sure your documentation stays up to date.

Fourth, test-driven development is a design technique, not just a testing technique. Dropping low-level tests can lead some people to write overcomplicated code. If you start bumping into code that is a little snarly, more direct testing may lead to cleaner designs.

Good luck, and let us know where you end up.

Howard said...

William -- all good points. I strongly feel that being test driven (of not test first), and leveraging a dependency injection container (HiveMind, then Tapestry 5 IoC) has impacted my code in a signficantly beneficial way. And, yes, hard to test is a design smell (I can think of a couple of classes in Tapestry 5 that do have that smell ... tested, but too hard to test, too hard to maintain the tests).

Andrew Binstock said...

You wrote "Even the most rabid test driven pundits don't expect you to unit test your accessor methods." Alas, this is not the case. They do in their quest to get 100% code coverage. See the comments to a blog post on this very point (http://binstock.blogspot.com/2007/11/fallacy-of-100-code-coverage.html)

For the kind of integration/unit testing you're describing, can you not create your own mocks? Not the traditional kind of mocks used in mock frameworks, but a mock that represents state at the specific point that you want to access? These special mock objects, which as you say are a lot of code to write, tend, however, to be reusable with other tests. This is how I solve the problem (as I understand your description) when working with other frameworks.

Glen said...

I guess one of the things that you have to consider is what is the unit you want to be testing at. It's tempting to use the method as the unit. That means you don't have to think about what you need to test too much. Just test every method.

A better idea is to verify higher level behaviours. What is this thing I'm trying to achieve with this bit of code? Test first should drive you in this direction but personally I find it hard to do test first all the time (as much as I would like to).

Stephen said...

I agree--unit testing at that level is usually a waste of time. Most of my tests end up being more acceptance level and it is great.

You still "don't write code that doesn't have a failing test"--but now your failing test is feature-level, which means the code you write to get it passing is actually needed.

Also, you can refactor all you want and, for the most part, not have to change your tests.

I just did this--rewrote a module from ~5k LOC down to ~3k LOC and had minimal test changes because none of the requirements changed--so the tests continued setting up the same inputs and asserting the same outputs. And I had 93% code coverage when I got done. Piece of cake.

I think if more people followed this approach, we wouldn't hear this nonsense about TDD "painting codebases into architectural corners" or whatever.

Hm. It seems that what I am advocating is BDD:

http://en.wikipedia.org/wiki/Behavior_driven_development

steve_l said...

I generally work at the functional test level these days, not unit tests. If the big functional test fails, then I drill down and add more detail, but otherwise, do the fewest tests needed to break the entire system.

Sony Mathew said...

Rate methods with a complexity rating from 1-5 with 5 being most complex. Then unit test everything with a complexity rating of 3 or higher. Anything below that is waste of time and will likely be covered in other unit tests.

Renat Zubairov said...

IMHO We can't replace unit tests with integration tests. That's a big mistake because on the system/integration for complex systems a number of different input parameters and number of output parameters increase in arithmetical progression. Coverage in this case is no measure at all.

When I hear people are saying that they have 80-90% coverage by integration tests it only means either system is too simplistic or output is not checked at all .

Basically we have no other choice as split system into simpler modules/units and test them separately. The main question here is how big the module is. In this example I agree with Howard that it makes no sense to test such class, but only because it's too small as one module/unit.

What is still important is a definition of what is module test and what is not. Mainly like this:

A test is not a unit test if:

* It talks to the database
* It communicates across the network
* It touches the file system
* It can't run at the same time as any of your other unit tests
* You have to do special things to your environment (such as editing config files) to run it.

Howard said...

It's fun to see the degree of discussion here; I think the order of the day is to be pragmatic. Tapestry without its integration tests hits on the order of 70% code coverage; add in the integration tests (which are executed with the unit tests ... fortunately, TestNG is not dogmatic about this) and the code coverage hits 93%.

As Matt Raible says, one size does not fit all. Today I was writing some more involved code than the example and I wrote a full mocked-up unit test suite for it; perhaps I put extra effort into it because I've been unable to make Selenium switch over to HTTPS as part of my integration tests.

In any case, I also strayed across William's fourth point, and identified some code that needed refactoring based on how difficult it was to test.

The moral is pretty standard: find what works, don't do things simply by rote or because it's expected, but really think about why your are doing anything and everything.

Craig said...

I agree unit tests can be pointless at times, but not always, for example large/complicated applications where you are not in charge of the database, and where the database has a direct impact of how your application works. We have applications that have versioned reference data that is used for dynamic portions of our application, which can be updated when business requests (for example for an older date you might get A, B, and C, but for a new date you might get A, B, D, and E). More often than anyone would want, our data team will create a new version and forget to load some of the older data, which then breaks our application at only specific times. Automated unit tests have been invaluable when it comes to things our data team can break.

Also, (less important), they can sometimes help find null pointers in updated code before our Q/A team can (which is always nice -- less written defects = happier project manager)

Anthony Oakhill said...

As long as you have a test that will fail if you change any aspect of the code in that small class, what does it matter if it is a unit test or an integration test?

The unit test will probably identify the failure more precisely and more quickly, but if the integration test finds it, good enough.

Test coverage IS important, and 93% coverage is quite good.

Stephane said...

Howard, I've recently reached the same conclusion.

I was trying to test Facade and Adapter objects. But the resulting unit tests had too many dependencies on the contracts of the collaborating objects. A clear test smell.

The problem basically is that the job of this kind of objects is to integrate other objects in the system. So he seems to me that it is very hard to try to test an integration behavior in isolation (The code of the test tends to closely mimick the code of the tested object)

So, I try to stick to unit tests, but I sometimes use integrated tests for this kind of objects.

Peter Lawrey said...

I think is worth wonder if adding a unit test really adds value. I am in favour of automated testing, but creating loads of trivial tests doesn't always help as much as some decent real world functional tests. For the code base I am working on I have 1230 tests for about 100K lines of code with over 70% coverage. I had 80% at one point but as the code size has increased I haven't thought it as important to keep such a high level of coverage.

mattraibert said...

My problem with what you have here is that the class/method you're testing is too small and therefore a bit of a straw man. Unit tests aren't going to seem very useful for a class whose whole purpose is to hide an if clause. Inline the method, use the unit testing facilities(mocks) you have created for the code's new home and only then consider refactoring.

Consider the TDD cycle: write a test to break; code to make it work; refactor your code to make it maintainable; start again. This should lead you to a better cost/value balance for your unit tests.