3

I have the following simple implementation:

var VALIDATION_TYPES = {
    ERROR: 1,
    WARNING: 2
};
var validationRules = {};

this.addErrorRule = function (event, rule) {
    addValidationRule(event, VALIDATION_TYPES.ERROR, rule);
};

var addValidationRule = function (event, type, rule) {
    if (!validationRules[event]) {
        validationRules[event] = {};
        validationRules[event][VALIDATION_TYPES.ERROR] = [];
        validationRules[event][VALIDATION_TYPES.WARNING] = [];
    }

    validationRules[event][type].push(rule);
};

I'm wondering whether addErrorRule should be unit-tested and if so how? Should I expose validationRules object to unit-tests?

EDIT:

I could modify my addValidationRule to return the number of rules added and check for that number in my test:

var addValidationRule = function (event, type, rule) {
    var countAddedRules = 0;

    if (!validationRules[event]) {
        validationRules[event] = {};
        validationRules[event][VALIDATION_TYPES.ERROR] = [];
        validationRules[event][VALIDATION_TYPES.WARNING] = [];
    }

    var originalCount = validationRules[event][type].length;
    validationRules[event][type].push(rule);
    countAddedRules = originalAddedRulesCount - validationRules[event][type].length;
    return countAddedRules;
};
3

I think that both of the testing method proposed so far are bad.

omouse suggest exposing validationRules an internal implementation detail. In doing so, he is suggesting that you test the implementation when you should be testing the interface. See this blog for discussion.

The op then proposed returning the number of added rules. But as far as I can tell it will always return 1. Even if the method did return varying values, you'd only be testing the values it returned, not the actual functionality you are interested in.

So how do you test this method? Firstly, don't think of it in terms of how to test that method. Instead, think about what behavior you want to test. Don't think about the internal state, or how it does it. Think about how the method will be used in the rest of your program.

Presumably, you want to be able to do something like this:

validator.addErrorRule('foobar', function(payload) {
   if (payload.cost > 50) {
        return "No. Price Too High";
   }
});
validation_results = validator.validate(payload)

All we need in order to turn that into a test is to say:

assert.deepEqual(validation_results, [
    {validator: 'foobar', error: 'No. Price Too High'}
])

This actually tests the functionality we care about and avoids introducing implementation details.

Many coders have gotten the idea that they need to test each method independently. That's why people think they need to test addErrorRule separately from validate. Don't fall into this trap. Methods within the same object are usually tightly interconnected and attempting to test them independently is a fool's errand. Furthermore, you miss out on testing the interaction between the methods which is usually the entire point of the functionality you wanted.

  • Thanks for your answer! There's a huge amount of discussions whether private methods\properties should be tested so it seems there's no definite answer. Regarding my case I decided to take out storage functionality from validator class and make it its own class. Then I can mock it inside validator class and if needed will test storage class on its own. What do you think? – Maxim Koretskyi Jan 19 '15 at 20:33
  • 1
    @Maximus, I'm afraid I think that's a very bad idea. Introducing a mock in this testing will make the tests more complex, include more implementation details, be harder to follow, be less helpful as documentation, and provide a shallower test of your functionality. The only possible advantage to introducing a mock here is to prevent tests on validator from failing because of bugs in storage. But I think that's an almost worthless benefit. – Winston Ewert Jan 19 '15 at 21:05
  • Got you, thanks! Probably it's really worthless benefit because of the current simple implementation of the storage (being simple hash map). Best! – Maxim Koretskyi Jan 20 '15 at 7:51
2

You definitely should be unit-testing addErrorRule. You can do this by testing for the side-effects of the addValidationRule call. Make sure there is a validation rule added to the right list (errors, not warnings).

Yes, you must expose the validationRules variable to be able to unit-test addErrorRules.

Here's example code for how the unit test might look like:

assertEquals(validationRules.length, 0);

addErrorRule('an error event', 'this is the rule');
assertEquals(validationRules['an error event'][VALIDATION_TYPES.WARNING].length, 0);
assertEquals(validationRules['an error event'][VALIDATION_TYPES.ERROR].length, 1);
assertEquals(validationRules['an error event'][VALIDATION_TYPES.ERROR][0], 'this is the rule');

You don't have to go further than that because then you're just testing the general side-effects of the addValidationRule function (which you say you're already doing).

Don't use Mocks or Mock Objects for This!

It's possible to use mocks for this but then you have to question what you're really testing here: does addErrorRule produce the correct side-effects, or is it enough that it calls the addValidationRule method and we can safely assume the side-effects will be correct?

If it's the latter, if you're testing with a mock method, you cannot check for the correct result in validationRules because it will be filled with whatever return value you supply in the mock which defeats the purpose of testing addErrorRule entirely.

The only time you would use a mock for this case would be if you were using a library or API which are difficult to test or time-consuming to test (for example, you would want to mock out database or filesystem calls or remote REST API calls and assume they give the correct side-effects).

  • the problem here is that if I change internal implementation of how rules are stored, I'll need to rewrite my test which might not be a good thing – Maxim Koretskyi Jan 19 '15 at 16:57
  • I've come up with another idea of how this method could be tested, please see my update. – Maxim Koretskyi Jan 19 '15 at 17:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.