Should tests break the Law of Demeter?


I was having a chat with a colleague recently about how to test a method like this:

initialize: function (options) {
  Backbone.Courier.add(this);
  this._resetColumnOrientation();
}

One could easily write a test like this:

describe('When initialize is called - ', function () {
  it('Then the view should be added to Backbone Courier so that it can spawn events', function () {
    const backboneCourierAddSpy =    sinonSandbox.spy(Backbone.Courier, 'add');
    SomeView.initialize();
    expect(backboneCourierAddSpy.args[0]).toEqual([view]);
  });

  it('Then the column orientation should be reset so that the correct column data is shown', function () {
    const resetColumnOrientationSpy = sinonSandbox.spy(SomeView, ‘_resetColumnOrientation');
    SomeView.initialize();
    expect(resetColumnOrientationSpy.callCount).toEqual(1);
  });
});

But should we? This is basically an interaction based testing approach. We don’t examine the side effects of Backbone.Courier.add because hopefully the vendor library Backbone Courier has its own test suite. Similarly, resetColumnOrientation would have its own set of tests. Here we’re just checking that the method gets called because it is important that the column orientation is reset when the view is initialized. If this doesn’t happen, then the view won’t behave as expected.

I’ve discussed this with a few people and have gathered some arguments pro and contra. Let the debate begin!

Arguments against this approach

  1. _resetColumnOrientation should really be a private method (no other modules should have knowledge of that method) hence we have no business testing it.
  2. This style of testing is too low level — it will make the tests brittle if we want to refactor it in the future. What if we decide that the method should be renamed or that we no longer want to use Backbone Courier (no complaints from me if that happened!)?
  3. This is just motivated by coverage hunting. It’s a meaningless test. There are no branches to explore here. These methods are always going to get invoked.
  4. We should just test for side effects, perhaps with a UI test that makes sure that the view has the correct columns after it’s opened.

Arguments for this approach

  1. This style of test serves as living documentation. The tests tell us what is meant to happen when the view is initialized.
  2. This test prevents regressions from new developers deleting code without fully understanding its significance.
  3. It’s a really quick test to write and will execute much faster than a UI test.
  4. If we later refactor the code and get failing tests, so what? It takes no time at all to modify the test and helps us to refactor safely by reminding us of how the code used to behave.

My conclusion

Personally, the only opposing argument that I find convincing is #1 — _resetColumnOrientation should be a private method. This is a very good point. In the code, we’ve tried to signal that by prefixing it with an underscore but that doesn’t stop anyone accessing it from another module (or from a test). It is considered an anti-pattern to make methods public purely for making it easier to test.

Perhaps what we should do in this instance is create a view test (still a unit test but one that examines the rendered HTML) to make sure that columns are correctly orientated. I have done that for this module but don’t particularly like doing it that way because it takes a while to write those kinds of DOM oriented tests.

describe('should conditionally render the column flip button - ', function () {
    const testCases = [
        {
            'columnConfigText': 'no columns',
            'columnConfigData': [],
            'renderColumnFlip': false
        },
        {
            'columnConfigText': '50|50',
            'columnConfigData': [50, 50],
            'renderColumnFlip': false
        },
        {
            'columnConfigText': '40|60',
            'columnConfigData': [40, 60],
            'renderColumnFlip': true
        },
        {
            'columnConfigText': '60|40',
            'columnConfigData': [60, 40],
            'renderColumnFlip': true
        },
        {
            'columnConfigText': '30|70',
            'columnConfigData': [33, 67],
            'renderColumnFlip': true
        },
        {
            'columnConfigText': '70|30',
            'columnConfigData': [67, 33],
            'renderColumnFlip': true
        }
    ];
beforeEach(function () {
        setUp();
    });
afterEach(function () {
        tearDown();
    });
testCases.forEach(function (testCase) {
        const shouldRenderText = testCase.renderColumnFlip ? 'should ' : 'should not ';
        it(`for ${testCase.columnConfigText} the column flip toggle  ${shouldRenderText} render`, function (done) {
            itemLayoutModal.setData({
                columns: testCase.columnConfigData,
                tabs: []
            });
itemLayoutModal.render();
const columnFlipButton = document.querySelector('[data-lrn-author-change-orientation]');

            let expectedDisplay = '';
            if (!testCase.renderColumnFlip) {
                expectedDisplay = 'none';
            }
     expect(columnFlipButton.style.display).toEqual(expectedDisplay);
            done();
        });
    });
});

In terms of short term productivity, it feels more efficient to break the Law of Demeter and write a quick test that checks whether or not a method has been called.

But long term, that approach causes issues. Information hiding is generally a good principle.

Much as I dislike it, the best thing here is probably to refactor the view to not have _updateColumnOrientation on the prototype and instead test the side effects of that method when the initialize method is called. The test can be abstracted so that when _updateColumnOrientation is called in other places, we don’t have to copy/paste the test but instead just run the other test to check that the UI changed in the expected manner.


Jeremy Nagel

This article was originally published on my personal blog.

 

 

Related Content


This post was posted in , , , by on