How to prove a bug in this piece of code via unit test

we have a habit in our company that when a bug is reported we do following steps:

1. Write a unit test that fails clearly showing the bug exists
2. Fix the bug
3. Re-run the test to prove the bug has been fixed
4. Commit the fix and the test to avoid regressions in the future

Now I came across a piece of legacy code with very easy bug. Situation looks like follows:

public final class SomeClass {
    ...
    public void someMethod(Parameter param) {
        try {
            开发者_JS百科if (param.getFieldValue("fieldName").equals("true")) { // Causes NullPointerException
                ...
            }
        } catch (Exception ex) {
            log.warn("Troubles ...", ex);
        }
    }
}

The problem here is that fieldName is not mandatory, so if not present, you get NPE. The obvious fix is:

if ("true".equals(param.getFieldValue("fieldName"))) {
    ...
}

My question is how to write a unit test to make the method fail. If I pass in a message which doesn't contain the fieldName it just logs the NPE but it won't fail ...

You may think what the method does? I can test the effect the method has. Unfortunatelly it communicates with some remote system so this will require a huge integration test which seems to be overkill with such a small and straiht-forward bug.

Note that it will be really hard if not impossible to make any changes in the code that are not directly causing the bug. So changing the code just to make it easier to test will probably not be an option. It's quite scary legacy code and everybody is really afraid to touch it.

You could do several things:

1. Stub the logger and check whether the error was logged or not as an indicator whether or not the bug occurred. You can use TypeMock or Moles if the logger can't be easily replaced.
2. Refactor the part inside the try block into its own method and call only that method inside the try block and make your unit test also call that method. Now the exception will not be silently logged and you can check whether or not it was thrown.

I think the best practice would be to mock out the logger, and assert that the logger has not been called for a pass. If it's a large change, I'm assuming the logger is used in a lot of places, which will help you with other tests in the future. For a quick-fix, you could raise an event in the exception catcher, but I don't think that's a very 'clean' way of doing it.

IMHO any catch all is wrong. You want to catch specific Exceptions, that you are looking for. Unit-testing is also about making the code better, so you can change it to

catch (ExpectedException ex)

The log could be an injected service, meaning that SomeClass looks like this:

public final class SomeClass 
{
    private final Logger log;

    public SomeClass(Logger log)
    {
        this.log = log;
    }
}

So in your unit test you could pass a fake Logger (possibly constructed with a mocking framework) which allows you to detect whether or not a warning was logged during the test.

If log is a global variable, you could make that global variable writable so that you can replace the logger in your tests in a similar way.

Yet another option is to add a Handler to log in your unit test for the purpose of detecting warn calls (assuming that you are using java.util.logging.Logger, but that doesn't seem to be the case because the method is warning, not warn).

Well, your companies method does mean you sometimes have to write a lot of extra tests, and you could argue if this is the way to go for this "obvious easy bug". But you're doing it for a reason.

I'd look at it like this:

• Your method should have a certain effect.
• It does not have this effect at the moment.
• If you want to fix it and test if the method works, you need to check the effect.

Therefore, you probably should write the whole code, even with the external systems, to proof the function works. Or in this case, doesn't.

Add an additional logger appender at warn level for this class/logger at the beginning of the test (and remove it at the end).

Then run the someMethod and check if the new appender is still empty (then there was no exception) or has content (then there was a exception).

You might consider using Boolean.toBoolean(String) or "true".equalsIgnoreCase(text)

If you don't want this behaviour you might want to add a test which shows that "True" and "TRUE" are treated as false.