2

We're in a project that has quite a lot of code and while in general we have a good test coverage and things work out pretty well, I noticed that our tests grew more and more complex over time up to a point where the tests are more complex than the code they test.

As a bit of background we're using Spock for testing and make ample use of it's mocking and interaction testing features - which may be part of the problem. In general we opted for white-box testing, so our test code mostly knows about the internal workings of the classes under test (which may be another contributing factor to the problem).

Let me give you a simplified example of a less than stellar test on this method:

class SomeServiceImpl {
    SomeDao someDao;

    public void store(SomeInputForm form) {
        // some checks of the form here.
        SomeEntity entity = new SomeEntity();
        // copy data from form to entity here.
        someDao.store(someEntity);
    }
}

And the test:

SomeServiceImpl underTest
SomeDao someDao

def setup() {
    underTest = new SomeServiceImpl()
    someDao = Mock(SomeDao)
    underTest.someDao = someDao
}

def "storing something yields that stuff ends up in the database"() {
    setup:
    def form = new SomeInputForm(... with some data ...)
    SomeEntity result = null        

    when:
    underTest.store( form )

    then:
    1 * someDao.store( { result = it } _ as SomeEntity )
    // additional checks to verify that the entity has the same data as the form
    result.someProperty == form
}

The intention of this test is to verify that some data that comes in via a form is stored inside the database. We mock out the DAO so we don't need a real DB here. While the test certainly works it has several problems:

  • It knows very well how the service works internally. As soon as you change the implementation of the service you have to change the implementation of the test, even if the end result would be the same.
  • It's hard to detect the side effect of the store method (see the complicated code for extracting the generated entity).
  • If you have multiple collaborators you will have to do a lot of mocking and stacking them all together.
  • The test basically repeats the code of the service in the then section.

I'm currently experimenting with a more functional way of things where you have just functions getting input and producing output, e.g. like this:

class SomeServiceImpl {

      void verify(SomeInputForm form) {
          // verify and throw exception if a problem is there
      }

      SomeEntity createEntityFromForm(SomeInputForm form) {
         // do the conversion here
      }


      void store(SomeEntity entity) {
         dao.store(entity);
      }
}

While this solves part of the testing problem, because now I can test the verify and createEntityFromForm methods without any collaborators and without knowing about the actual implementation, there still has to be a place where these three methods are called in the correct order:

   service.verify(form);
   Entity en = service.createEntityFromForm(form);
   service.store(form);

Which effectively just moves the problem somewhere else (and introduces a lot of margin for error as you need to call methods in the correct order now).

So I wonder whether there is a better way of organising the code differently to make the tests less brittle and less whitebox while still detecting the side effects (or even a way where you wouldn't need to detect the side effect but still know stuff is properly saved to the database).

  • Which effectively just moves the problem somewhere else -- Sure, but that "somewhere else" is very easy to reason about. – Robert Harvey Jul 28 '16 at 21:51
  • As to the side effects, the way functional programmers deal with them is by sequestering them in a monad. While you don't have to be that formal about it, you can still limit the number of functions you write that cause side effects. Those are usually your database functions, and you already have a well-understood way of testing those. – Robert Harvey Jul 28 '16 at 21:54
1

To your first bullet point and last bullet point are pretty much the same thing and I'll address those last.

To your second bullet point, it is not this tests job to detect side effects of the SomeDao's store method. This test is testing SomeServiceImpl, not SomeDao, it doesn't matter what SomeDao does. Another test fixture should be testing that. The only thing that this test should be checking was whether store was called. Otherwise this is an integration test.

Third bullet point - yes, this is an unfortunate drawback of unit testing but the more complicated a system gets the more considerations we must make. But this isn't entirely a bad thing. It forces us to think about how one piece of code interacts with other pieces. It gives us a sharp glimpse of the point of interaction between them and let's us evaluate how we expect things to work.

Now the real meat. While I don't know that I'd consider expectations "repeating" code, but this is probably the biggest legitimate criticism to unit tests and mocking frameworks; the knowledge the tests often have to have about the specific method they're testing.

But that's not the end of the world. Yes if you completely change how you do data access the unit test will have to drastically change too, but that's unlikely. If your DAO has a proper interface on it and respects the open/closed principle this is an unlikely scenario. So you probably will only catch regressions or have to write more tests for new functionality rather than have to refactor for changed functionality.

So I'd say just make sure your service dependencies are well abstracted, try your best to make them closed for modification (+open for extension) and you will be in about as good a shape as you can be.

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.