4

I believe the existence of this unit test is justified. However, to me, this unit test seems very coupled to the method implementation, though I'm not sure it can be improved upon.

class ItemManager {
  void save(Item item) {
    if (item.id == null)  
       create(item)
    else 
       update(item)
  }
}

   @Test
   public void save() throws Exception {
      ItemManager manager = spy(new ItemManager());
      doNothing().when(manager).create(any(Item.class));
      doNothing().when(manager).update(any(Item.class));

      Item item = new Item();
      manager.save(item);
      verify(manager, (times(1))).create(item);

      item.setId("id");
      manager.save(item);
      verify(manager, (times(1))).update(item);
}

Edit:

Seems there are two problems with this unit test, the coupling to the implementation and that the test covers two behaviors. I agree that the two behaviors should be split into separate tests, and understand that the coupling is undesirable, but I'm still not sure what would be the appropriate way to test this.

It seems that the correct way to test the save behavior is to verify it results in the expected invocations of my persistence layer. However, this seems very redundant with unit tests of the create and update methods.

Is the redundancy in the below test appropriate? Any suggestions for improvement?

 Dao mockDao;

    @Test
    public void create() {
      manager.create(new Item()); 

      verify(mockDao).insert("expected params");
    }

    @Test
    public void update() {
      Item item = new Item();
      item.setId("id");
      manager.update(item); 

      verify(mockDao).update("expected params");
    }

    @Test
    public void saveWithoutId() {
      manager.save(new Item()); 

      verify(mockDao).insert("expected params");
    }

    @Test
    public void saveWithId() {
      Item item = new Item();
      item.setId("id");
      manager.save(item); 

      verify(mockDao).update("expected params");
    }
5
  • 3
    please prove the full class or at least a list of all its methods and what its supposed to do
    – Ewan
    May 24 '17 at 17:07
  • 1
    I don't think It's tightly coupled, I rather think It's useless. You are just testing a single if. It would take you ages to test components in this way
    – Laiv
    May 24 '17 at 19:34
  • 2
    @Laiv save has two behaviours depending on the input, it should absolutely be unit tested. May 24 '17 at 19:59
  • 2
    It might also be worth saying that your test is testing two separate concerns. You should have two different tests, not one that does both. May 24 '17 at 19:59
  • It's the way the test is implented. I didn't mean that the test is wortheless. I agreed. There should be differents methods testing all the branching caused by the state of the imputs and the if. OK cases and KO cases.
    – Laiv
    May 24 '17 at 20:56
2

I'd (also) answer: yes.

UnitTest verify the public observable behavior of a unit. public observable behavior menas what return values it produces and how it communicates with its dependencies.

Your test verifies which of its internal methods it calls.

Most likely your ItemManager class communicates with a persistence layer. A good test would verify the communication between your code under test (cut) and this persistence layer:


Another thing about UnitTest is: each method in a UnitTest verified a singe expectation.

Your test verifies two expectations at once for two different inputs.

combining both aspects your test should look like this:

   @Mock
   private Connection databaseConnection;
   @Mock
   private PreparedStatement preparedStatement;

   private  Item item = new Item();
   @Before
   public vod setup(){
      doReturn(preparedStatement).when(databaseConnection).createPreparedStatement(anyString());
   }
   @Test
   public void saveItenWithoutIdInserts() throws Exception {
      ItemManager manager = new ItemManager(databaseConnection);

      manager.save(item);
      verify(databaseConnection).createPreparedStatement(ItemManager.INSERT_SQL);
   }
   @Test
   public void saveItenWithIdUpdates() throws Exception {
      ItemManager manager = new ItemManager(databaseConnection);
      item.setId("id");
      item.setName("some name");      
      manager.save(item);
      verify(databaseConnection).createPreparedStatement(ItemManager.UPDATE_SQL);
      verify(preparedStatement).setString(ItemManager.INDEX_OF_ID_IN_UPDATE_SQL,"id");
   }

   @Test
   public void saveCopiesValuesFromItemToPreparedStatement() throws Exception {
      ItemManager manager = new ItemManager(databaseConnection);
      item.setName("some name");      
      manager.save(item);

      verify(preparedStatement).setString(ItemManager.INDEX_OF_NAME_IN_SQL,"some name");
   }

I'm concerned that unit tests of my manager.create and manager.update methods would be redundant with saveItenWithoutIdInserts and saveItenWithIdUpdates. I've edited my question to reflect this, any thoughts? – user39846

Your updated question indeed has redundant tests. But the reason is that you have redundant behavior.

You should make a decision which way you want you clients to use this code: do you want them to explicitly call update or insert or just call save?

If the latter update and insert are not part of the public interface and should not be tested explicitly. They should also not have public visibility.

But if you want the clients calling update and insert on their own decision you need this "redundant" tests at least as a documentation how the clients may use your code. Also you should add two more tests for what happens when they call update with a missing ID or insert having an ID unexpectedly.

4
  • 1
    This makes sense, but I'm concerned that unit tests of my manager.create and manager.update methods would be redundant with saveItenWithoutIdInserts and saveItenWithIdUpdates. I've edited my question to reflect this, any thoughts?
    – user39846
    May 24 '17 at 20:36
  • This is fine, but my LEAN nature is with OP and @Laiv here: a couple dozen lines of code to test the syntactic sugar of a single if statement? Since create() and update() are public there should already be tests for the "meat". Are we reaching overkill?
    – user949300
    May 25 '17 at 4:15
  • 1
    The point is: you are not testing the if, you're testing that your unit behaves differently for the different inputs. The if is just one way to achief that. May 25 '17 at 6:57
  • @user39846 "I've edited my question to reflect this, any thoughts?" - updated the answer. May 26 '17 at 19:54
1

Why do you write unit tests in the first place? I create a unit. The unit hopefully has a spec. I write unit tests that confirm that my unit meets its spec. If my unit tests are good, then you can rely on my unit working according to spec as long as the tests pass.

If I change my unit and break it, or I change my unit because the spec changed, then (a) my unit tests will break, and (b) your code which relies on my unit working according to the spec that you know will break. The nice thing about unit tests is that you immediately know the culprit: It's my code not working to the spec you expect. So you don't waste time hunting for a bug in your code that doesn't exist. That's nice for you. (We also have reliable information which units work according to their expected spec, which makes everyone feel a lot better).

Now quite obviously unit tests don't help me if I introduced a bug in my unit, only to the degree that they tell me there is a bug, but not where the bug is. Just like you got help by being told "bug is due to gnasher's broken unit", I would also like some help here. And I can get that help by writing tests for my internal code.

Consider that instead of writing "create" and "update" myself, I could have relied on someone else writing a unit implementing them. In that case, there would be unit tests. Now it shouldn't matter that I didn't write a separate unit, I still want these tests.

You can say quite rightfully that these are not unit tests because they don't test the observable behaviour but something internal. Still, they are useful tests. And I would want to run them at the same time as my unit tests, that is after any change in the unit. So they should go with the unit tests.

It's probably wise to mark in the source code for your unit tests which tests are testing the observable behaviour (because other developers will look at these tests if they are not sure that the observable behaviour you tried to implement is the observable behaviour that everyone agreed on), and which tests are internal (of no concern to users of your unit).

One argument could be that internal tests could fail, but as long as the observable behaviour unit tests run, everything is fine, and these internal test failures waste time. I would disagree. If my internal tests fail, and my unit tests pass, then either the code in my unit is badly understood, which is always asking for trouble, or my unit tests are no good.

-1

IMO, yes. Unit tests like this always bug me, Isn't the point to test that the item was actually saved successfully, not that a particular method was called?

Better to test the much more useful and generally applicable requirement that create() throws an Exception on a non-null id and likewise that update() throws an Exception on a null id.

Then, this test of save() is unnecessary. Or, if you insist, can be simplified to something like

Item item = new Item();
manager.save(item);
item.setId("id");
manager.save(item);
// no exceptions were thrown, we pass
1
  • Unit tests find broken code. If the save method is broken, and neither saves nor throws an exception, you will never know.
    – gnasher729
    May 27 '17 at 19:56

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.