2
\$\begingroup\$

I'm relatively new to unit testing and having gone through quite a bit of pain I was starting to feel pretty good about my tests. The problem is, now I have a nice set of green ticks I'm also suffering from the nagging doubt that it's just giving me a false sense of security and is in fact just a waste of time.

I have included an example of a method under test and all its accompanying unit tests. My approach was to try and use spies to ensure only the method itself was being tested (the intention being that other methods will be covered in their own tests). So for example my validation methods throw an exception when a value fails validation but these are caught and ignored here, and only the fact the validation is called with the correct parameters is tested.

Writing these tests felt pretty good, but now I can't really see the point of them. The only thing which can break these tests is a change to this specific method and surely if anybody is changing this method they are doing so with good reason, and all I've done is given them an extra job of changing the tests.

I can definitely say that writing these tests caused me to make several improvements to the way I wrote the method under test so there is benefit from that point of view but the unit tests which are left behind feel a bit redundant.

So am I wrong in my approach? Or in my analysis of the value my approach? There are so many evangelists for unit testing - many of whom are far cleverer than me - so I'm pretty sure it's me that's wrong somewhere.

Method under test:

/**
 * Calculate the physical centre x co-ordinate for the passed unscaled value
 * @param {string} unscaledValue - The logical cx value to be converted to a physical value.  It could be a string for category axes or a number for measure axes.
 * @param {number} [innerBarCount] - The number of small bars within a bar group.  This is only required for multiple category axes.
 * @param {number} [offset] - The zero based index of an inner bar within a bar group.  This is only required for multiple category axes.
 */
this._getCx = function (unscaledValue, innerBarCount, offset) {

    var returnCx = 0;

    // Validate the required parameters and properties
    dimple.validation._isDefined("x axis", this.x);
    dimple.validation._isDefined("unscaledValue", unscaledValue);

    // Act based on axis types
    if (this.x._hasMeasure() || this.x._hasTimeField()) {
        // Measures can return a straight scale
        returnCx = this.x._scaleValue(unscaledValue);
    }
    else if (this.x._hasMultipleCategories()) {
        // For multiple categories the second two parameters are required
        dimple.validation._isPositiveNumber("innerBarCount", innerBarCount);
        dimple.validation._isPositiveNumber("offset", offset);
        // Scale to get the left position and then calculate the inner position of the bar based on offset
        // plus a half accounting for bar gaps
        returnCx = this.x._scaleValue(unscaledValue) + this._xBarGap() + (offset + 0.5) * (this._xBarSize() / innerBarCount);
    } else if (this.x._hasCategories()) {
        // Scale to get the left position of the bar and add half the bar size to get the centre
        returnCx = this.x._scaleValue(unscaledValue) + (this.x._pointSize() / 2);
    } else {
        throw dimple.exception.unsupportedAxisState("x");
    }

    return returnCx;

};

And the tests themselves:

describe("dimple.series._getCx", function() {

    var seriesUnderTest = null,
        // Mock return values as ascending primes to avoid coincidental passes
        unscaledValue = 2,
        scaleReturn = 3,
        innerBarCount = 7,
        offset = 11,
        barGap = 13,
        pointSize = 17,
        barSize = 19;

    beforeEach(function () {
        // The axis to return mock values while testing
        var mockAxis = jasmine.createSpyObj("axis spy", [
            "_hasMeasure",
            "_hasCategories",
            "_hasMultipleCategories",
            "_hasTimeField",
            "_scaleValue",
            "_pointSize"
        ]);
        // These will be individually overridden in tests to mock different axis types
        mockAxis._hasMeasure.andReturn(false);
        mockAxis._hasCategories.andReturn(false);
        mockAxis._hasMultipleCategories.andReturn(false);
        mockAxis._hasTimeField.andReturn(false);
        // Set the return type dimensions
        mockAxis._scaleValue.andReturn(scaleReturn);
        mockAxis._pointSize.andReturn(pointSize);

        // Instantiate the series to test
        seriesUnderTest = new dimple.series();
        seriesUnderTest.x = mockAxis;

        // Set up series mocks
        spyOn(seriesUnderTest, "_xBarGap").andReturn(barGap);
        spyOn(seriesUnderTest, "_xBarSize").andReturn(barSize);

        // Set up validation spies
        spyOn(dimple.validation, "_isDefined").andReturn(true);
        spyOn(dimple.validation, "_isNumber").andReturn(true);
        spyOn(dimple.validation, "_isPositiveNumber").andReturn(true);

    });

    it("Validates required members", function () {
        try { seriesUnderTest._getCx(unscaledValue); }
        catch (ignore) { /* validation is not under test */ }
        expect(dimple.validation._isDefined).toHaveBeenCalledWith("x axis", seriesUnderTest.x);
    });

    it("Validates required parameters", function () {
        try { seriesUnderTest._getCx(unscaledValue); }
        catch (ignore) { /* validation is not under test */ }
        expect(dimple.validation._isDefined).toHaveBeenCalledWith("unscaledValue", unscaledValue);
    });

    it("Does not validate optional parameters for axes other than multiple category", function() {
        try { seriesUnderTest._getCx(unscaledValue, innerBarCount, offset); }
        catch (ignore) { /* validation is not under test */ }
        expect(dimple.validation._isPositiveNumber).not.toHaveBeenCalled();
        expect(dimple.validation._isPositiveNumber).not.toHaveBeenCalled();
    });

    it("Validates optional parameters for multiple category axes", function() {
        seriesUnderTest.x._hasMultipleCategories.andReturn(true);
        try { seriesUnderTest._getCx(unscaledValue, innerBarCount, offset); }
        catch (ignore) { /* validation is not under test */ }
        expect(dimple.validation._isPositiveNumber).toHaveBeenCalledWith("innerBarCount", innerBarCount);
        expect(dimple.validation._isPositiveNumber).toHaveBeenCalledWith("offset", offset);
    });

    it("Throws an exception if axis returns false for all types", function() {
        expect(function () { seriesUnderTest._getCx(unscaledValue); })
            .toThrow(dimple.exception.unsupportedAxisState("x"));
        expect(seriesUnderTest.x._hasMeasure).toHaveBeenCalled();
        expect(seriesUnderTest.x._hasCategories).toHaveBeenCalled();
        expect(seriesUnderTest.x._hasMultipleCategories).toHaveBeenCalled();
        expect(seriesUnderTest.x._hasTimeField).toHaveBeenCalled();
    });

    it("Uses the x axis scaling for measure axes", function() {
        seriesUnderTest.x._hasMeasure.andReturn(true);
        expect(seriesUnderTest._getCx(unscaledValue)).toEqual(scaleReturn);
        expect(seriesUnderTest.x._hasMeasure).toHaveBeenCalled();
        expect(seriesUnderTest.x._scaleValue).toHaveBeenCalledWith(unscaledValue);
    });

    it("Uses the x axis scaling for time axes", function() {
        seriesUnderTest.x._hasTimeField.andReturn(true);
        expect(seriesUnderTest._getCx(unscaledValue)).toEqual(scaleReturn);
        expect(seriesUnderTest.x._hasTimeField).toHaveBeenCalled();
        expect(seriesUnderTest.x._scaleValue).toHaveBeenCalledWith(unscaledValue);
    });

    it("Calculates middle bar position for multiple categories", function() {
        seriesUnderTest.x._hasMultipleCategories.andReturn(true);
        expect(seriesUnderTest._getCx(unscaledValue, innerBarCount, offset))
            .toEqual(scaleReturn + barGap + (offset + 0.5) * (barSize / innerBarCount));
        expect(seriesUnderTest.x._hasMultipleCategories).toHaveBeenCalled();
        expect(seriesUnderTest.x._scaleValue).toHaveBeenCalledWith(unscaledValue);
        expect(seriesUnderTest._xBarGap).toHaveBeenCalled();
        expect(seriesUnderTest._xBarSize).toHaveBeenCalled();
    });

    it("Calculates middle bar position for single categories", function() {
        seriesUnderTest.x._hasCategories.andReturn(true);
        expect(seriesUnderTest._getCx(unscaledValue))
            .toEqual(scaleReturn + pointSize / 2);
        expect(seriesUnderTest.x._hasCategories).toHaveBeenCalled();
        expect(seriesUnderTest.x._scaleValue).toHaveBeenCalledWith(unscaledValue);
        expect(seriesUnderTest.x._pointSize).toHaveBeenCalled();
    });

});
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

I agree with what you said, i.e.:

  • The only thing which can break these tests is a change to this specific method and surely if anybody is changing this method they are doing so with good reason, and all I've done is given them an extra job of changing the tests.

  • I can definitely say that writing these tests caused me to make several improvements to the way I wrote the method under test so there is benefit from that point of view but the unit tests which are left behind feel a bit redundant.

However "The only thing which can break these tests is a change to this specific method" isn't quite true: the test could also discover a breaking change to any of the libraries or subroutines called from (used by) the unit-under-test.


I do like automated tested (aka "regression testing") but I prefer to high-level to low-level tests.

Testing low-level "units" is especially useful, in my opinion, if and only if they need to be tested before they're "integrated": for example because they will be integrated with code written by other people, and because debugging during integration testing is expensive (because during an integration test it's relatively expensive to discover which of the components to blame).

For further discussion, see this question and the various answers to it: Should one test internal implementation, or only test public behaviour?

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for the response, I've been thinking on this a while and actually I've started to see the benefit of the tests. I think it comes down to the way that people will modify the code. If somebody needs to alter the functionality described in the test, there seems little value, however it's more likely that people will extend the logic, in which case it's important to test that they haven't disrupted the existing logic flows e.g adding a new condition to the if-else block for a currently unknown axis type. With that in mind I can certainly see benefit in these tests. \$\endgroup\$ Commented Jan 31, 2014 at 9:31

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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