[mockito] Trouble with delete test (resent)

[Russell Bateman]

(Resend: I apologize. Thunderbird duped me into something stupid to do with font size. Here, I think, is what I meant to send.)


I'm still a newbie to Mockito. I'm not certain how much info someone will need to steer me right, but I'll try to give enough. I've got this ReST servlet I'm trying to write tests for. The architecture for this particular piece is fairly standard:

StateService (JAX-RS) -> StateBo (business logic) -> StateDao -> State (POJO/bean) -> database

I'm trying to write a unit test for StateBo. So far, I'm pretty sure it's working although there are bits that don't seem to do much (but then, there's hardly any business logic in this piece, which is why I chose it as the simple case to get started on).

The bit that simply doesn't work in JUnit is testDelete(). I also question what testUpdate() and testFindByPropertyAndValue() are doing for me, but mostly, I'd like to fix testDelete(), please.

Here's the code for the test (and, if useful, the business logic class I'm trying to test comes after).

Thanks to anyone willing to take the time to help induct me into the halls of Mockito understanding!

Also, any criticism of what/how I'm doing is very welcome.

Thanks and best regards,
Russ

package com.acme.user.bo;

import static org.junit.Assert.assertTrue;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.doNothing;
import static org.mockito.Matchers.any;

import java.util.LinkedList;
import java.util.List;

import org.junit.Before;
import org.junit.Test;

import com.acme.user.entity.State;
import com.acme.user.entity.StateDao;

public class StateBoTest
{
    private static final Integer ID_1    = new Integer( 1 );
    private static final Integer ID_2    = new Integer( 2 );
    private static final State   ALABAMA = new State( ID_1, "us", "al", "Alabama" );
    private static final State   ALASKA  = new State( ID_2, "us", "ak", "Fred" );
    private static final String  CODE    = "code";
    private static final String  AL_CODE = "al";

    private StateBo    bo;
    private StateDao   dao;

    @Before
    public void setup()
    {
        this.bo  = new StateBo();          // class under test
        this.dao = mock( StateDao.class ); // dependent class (mocked)
        this.bo.setDao( this.dao );        // every test needs this wiring
    }

    private final boolean checkStateResult( State control, State result )
    {
        assertEquals( control.getId(),      result.getId() );
        assertEquals( control.getCountry(), result.getCountry() );
        assertEquals( control.getCode(),    result.getCode() );
        assertEquals( control.getName(),    result.getName() );
        return true;
    }

    @Test
    public void testCreate()
    {
        // set up mocked result on create...
        when( this.dao.create( ALABAMA ) ).thenReturn( ALABAMA );

        // call the method we wish to test...
        State result = this.bo.create( ALABAMA );

        // verify the method was called...
        verify( this.dao ).create( result );

        // verify correct results returned...
        assertNotNull( result );
        assertTrue( checkStateResult( ALABAMA, result ) );
    }

    @Test
    public void testFindAll()
    {
        // squirrel-cage the database...
        List< State > all = new LinkedList< State >();
        all.add( ALABAMA );
        all.add( ALASKA );

        // return mocked result set on read...
        when( this.dao.read() ).thenReturn( all );

        // call the method we want to test...
        List< State > result = this.bo.findAll();

        // verify the method was called...
        verify( this.dao ).read();

        // verify correct number of results returned...
        assertEquals( 2, result.size() );

        // verify correct results returned...
        assertTrue( checkStateResult( ALABAMA, result.get( 0 ) ) );
        assertTrue( checkStateResult( ALASKA,  result.get( 1 ) ) );
    }

    @Test
    public void testFindById()
    {
        // return mocked result set on readById...
        when( this.dao.readById( 1 ) ).thenReturn( ALABAMA );

        // call the method we want to test...
        State result = this.bo.findById( 1 );

        // verify the method was called...
        verify( this.dao ).readById( 1 );

        // verify correct result returned...
        assertNotNull( result );
        assertTrue( checkStateResult( ALABAMA, result ) );
    }

    /**
     * I don't think this test accomplishes anything and it needs to be re-thought.
     * Or, maybe it's okay as it is? If so, what's it doing for me?
     */
    @Test
    public void testFindByPropertyAndValue()
    {
        // squirrel-cage the database...
        List< State > all = new LinkedList< State >();
        all.add( ALABAMA );
        all.add( ALASKA );

        // return mocked result set on read...
        when( this.dao.readByPropertyAndValue( CODE, AL_CODE ) ).thenReturn( all );

        // call the method we want to test...
        List< State > result = this.bo.findByPropertyAndValue( CODE, AL_CODE );

        // verify the method was called...
        verify( this.dao ).readByPropertyAndValue( CODE, AL_CODE );

        // verify correct number of results returned...
        assertEquals( 2, result.size() );

        // verify correct results returned...
        assertTrue( checkStateResult( ALABAMA, result.get( 0 ) ) );
        assertTrue( checkStateResult( ALASKA,  result.get( 1 ) ) );
    }

    @Test
    public void testUpdate()
    {
        // set up mocked result on update...
        doNothing().doThrow( new IllegalStateException() )
            .when( this.dao ).update( any( State.class ) );

        // call the method we wish to test...
        this.bo.update( ALABAMA );

        // verify the method was called...
        verify( this.dao ).update( ALABAMA );
    }

    /**
     * Hmmmm... back from JUnit, I get:
     *
     * Argument(s) are different!
     * Wanted:
     *    stateDao.delete( { id=1, country='us', code='al', name='Alabama' );
     *        at StateBoTest.testDelete( StateBoTest.java:175 )
     *
     * Actual invocation has different argument:
     *    stateDao.delete( null );
     *        at StateBo.delete( StateBoTest.java:50 )
     */
    @Test
    public void testDelete()       THIS GUY DOESN'T WORK: SEE COMMENT ABOVE
    {
        // set up mocked result on update...
        doNothing().doThrow( new IllegalStateException() )
            .when( this.dao ).delete( ALABAMA );

        // call the method we wish to test...
        this.bo.delete( ID_1 );

        // verify the method was called...
        verify( this.dao ).delete( ALABAMA );         (LINE 175)
    }
}

package com.acme.user.bo;

import java.util.List;

import org.apache.log4j.Logger;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional;

import com.acme.user.entity.State;
import com.acme.user.entity.StateDao;

@Transactional
@Service( "stateBo" )
public class StateBo
{
    public static Logger log = Logger.getLogger( StateBo.class );

    @Autowired( required = true )
    private StateDao stateDao;

    public State create( State state )
    {
        return stateDao.create( state );
    }

    public List< State > findAll()
    {
        return stateDao.read();
    }

    public State findById( Integer id )
    {
        return stateDao.readById( id );
    }

    public List< State > findByPropertyAndValue( String propertyName, String propertyValue )
    {
        return stateDao.readByPropertyAndValue( propertyName, propertyValue );
    }

    public void update( State state )
    {
        stateDao.update( state );
    }

    public void delete( Integer id )
    {
        State state = findById( id );
        stateDao.delete( state );                         (LINE 50)
    }

    /* Beyond here is just stuff for JUnit/Mockito tests... */
    public void setDao( StateDao dao )
    {
        stateDao = dao;  // sets a mocked DAO object since no autowiring
    }
}

[Russell Bateman]

Um, I've kept at this. I think I get it now. This is what I've found, in case someone wishes to confirm this.

    @Test
    public void testDelete()       THIS GUY NOW WORKS

    {
        // set up mocked result on update...
        doNothing().doThrow( new IllegalStateException() )
            .when( this.dao ).delete( ALABAMA );

        // set up modked result for readById() since bo.delete() calls bo.findById()
        // which calls dao.readById() and we haven't set up that as a mock result
        when( this.dao.readById( ID_1 ) ).thenReturn( ALABAMA );

        // call the method we wish to test...

        this.bo.delete( ID_1 );

        // verify the method was called...
        verify( this.dao ).delete( ALABAMA );
    }

Based on this, I have to add a new concept to my methodology: not only set up mocked results for the methods that are directly and obviously called, but analyze them to see if they have similar needs and, if so, go set up mocked results for them too.

Anyone like to say "duh!" to me?

Thanks for your time, guys.

Russ

[Brice Dutheil]

Hi Russell,

I think your tests are a bit "over-zealous", you can probably relax your test code

For example testCreate does seem pretty simple, here's how I would code it, and removing my comments makes it a 3 liner.

@Test

public void on_create_returns_same_instance() { // note the name of the test that identify a scenario

    when( dao.create( ALABAMA ) ).thenReturn( ALABAMA );

    State result = bo.create( ALABAMA );

    // no need for verify, otherwise the tests won't pass, the verification in this context is implicit.

    assertSame( ALABAMA, result ); // no need to check the properties your StateBo is a pass-through

    // however if StateBo does something, you might want to replace that by an "assertStateProperties"

    // but then you will give another name to your test, like "on_create_ensure_properties_do_have_<your expected values or meaning>"

}

And now with the deletion code :

@Test

public void should_invoke_Dao_on_delete_by_id() { // delete scenario

    // mockito mocks do nothing by default, no need to mock that

    when( this.dao.readById( ID_1 ) ).thenReturn( ALABAMA ); // ok this is needed

    this.bo.delete( ID_1 );

    verify( this.dao ).delete( ALABAMA ); // ok, you need to verify that the interaction happened

}

@Test

public void should_not_propate_on_second_delete_of_same_id() { // another delete scenario on successive delete of same ID

    doNothing().doThrow( new IllegalStateException() ).when( this.dao ).delete( ALABAMA ); // needed here as part of the scenario

    when( this.dao.readById( ID_1 ) ).thenReturn( ALABAMA ); // still needed

    this.bo.delete( ID_1 );

    this.bo.delete( ID_1 );

    // verify of the interaction not needed, it has been in the previous test,

    // here you are testing something else i.e. just that StateBo don't propagate the exception on the second delete call

}

Really if your code is a pass-through you don't really care about the properties of a State. If your code begins to do other things then you will document it in the test with identifiable scenarios. I invented them in the above examples, but you get the point.

How's that sound ?

Cheers,

-- Brice

--
You received this message because you are subscribed to the Google Groups "mockito" group.
To post to this group, send email to moc...@googlegroups.com.
To unsubscribe from this group, send email to mockito+u...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/mockito?hl=en.

[Russell Bateman]

Brice,

My over-zealousness is a function of my continued discomfort with the technology of course. That will change with growing familiarity and success.

This reply is very helpful and I've learned a great deal from it.

I greatly appreciate you taking the time to comment. Support like this is crucial to the success of a technology and, in my experience, Mockito is one of the best supported technologies around.

Thanks!

Russ

[Brice Dutheil]

Please don't take that word literally ;)

I used to be over-zealous too a long time ago, and seen others being "over-zealous", it ended in very verbose tests that were difficult to refactor and poorly maintainable. That's why I wanted to warn you.

In my opinion practicing TDD is a great help to improve your design. The test gives you feedback on the quality of the production code and the quality and readability of your test code.

If you are interested I strongly advises you to read the book Growing Object Oriented Software Guided by Tests, it's revealing.

Anyway I'm happy you found it useful, don't hesitate to ask questions here and there :)

Thank you for this appreciation :)

I regularly keep myself thinking on thanking a lot Szczepan about it ;)

-- Brice