Partial mock of an instance only in a single NonStrictExpectations block possible

[jOlliv]

Partial mocking of an instance is not possible in more than one NonStrictExcpectations blocks. This prevents creating reusable methods that do mock a specific method of a given instance to make the test code more readable.

In the following example the method mockBothMethodsAtOnce succeeds and the method mockBothMethodsSuccessively fails with the exception:

java.lang.IllegalArgumentException: Class is already mocked: class PartialMockAnInstanceTest$ClassToPartialMock

Example:

public class PartialMockAnInstanceTest
{
@Test public void mockBothMethodsAtOnce() throws Exception {
    final ClassToPartialMock classToPartialMock = new ClassToPartialMock();
    new NonStrictExpectations( classToPartialMock ) {{
        classToPartialMock.getA(); result = "mockedA";
        classToPartialMock.getB(); result = "mockedB";
    }};
}

@Test public void mockBothMethodsSuccessively() throws Exception {
    final ClassToPartialMock classToPartialMock = new ClassToPartialMock();
    new NonStrictExpectations( classToPartialMock ) {{
        classToPartialMock.getA(); result = "mockedA";
    }};
    new NonStrictExpectations( classToPartialMock ) {{
        classToPartialMock.getB(); result = "mockedB";
    }};
}

private class ClassToPartialMock {
    public String getA() { return "A"; }
    public String getB() { return "B"; }
}
}

[jOlliv]

It was working with version 1.8 and fails with the versions 1.9 and 1.10.

[rliesenfeld]

Yes, JMockit 1.9 added a validation against duplicate mockings, to prevent certain misuses of the API that I've seen happening.

So, ok, you want "reusable methods that mock a specific method of a given instance to make the test code more readable". That initially sounds reasonable, but it seems to me that such tests would just as easily be less readable, due to the use of multiple expectation blocks for the same mocked instance, which makes the test more complicated than it needs to be.

That new validation could be relaxed to allow this situation, but I would prefer to first see a realistic example test demonstrating that it brings more benefit than harm.

[jOlliv]

As suggested in the jmockit tutorial in the section "Reusing expectation and verification blocks" we reuse our NonStrictExpectation blocks to avoid code duplicates and keep our test methods short. For our tests that use partial mocking of an instance we made no difference. The following code example illustrate how we use it:

@Tested OurTestedClass ourTestedClass;

@Test public void testAMethodThatNeedsAMocked() {
    mockGetAWith( "returnValueOfA" );
    ourTestedClass.appendSomethingToValueOfA();
    // some assertions or verifications
}

@Test public void testAMethodThatNeedsBMocked() {
    mockGetBWith( "returnValueOfB" );
    ourTestedClass.insertSpacesInB();
    // some assertions or verifications
}

@Test public void testAMethodThatNeedsAAndBMocked() {
    mockGetAWith( "returnValueOfA" );
    mockGetBWith( "returnValueOfB" );
    ourTestedClass.concatValuesOfAAndB();
    // some assertions or verifications
}

private void mockGetAWith( final String returnValue ) {
    new NonStrictExpectations( ourTestedClass ) {{
        ourTestedClass.getA(); result = returnValue;
    }}
}

private void mockGetBWith( final String returnValue ) {
    new NonStrictExpectations( ourTestedClass ) {{
        ourTestedClass.getB(); result = returnValue;
    }}
}

[rliesenfeld]

Ok, but why not simplify the tests as follows:

@Tested OurTestedClass ourTestedClass;

@Test public void testAMethodThatNeedsValueOfA() {
    outTestedClass.setA( "valueOfA" );
    ourTestedClass.appendSomethingToValueOfA();
    // some assertions or verifications
}

@Test public void testAMethodThatNeedsValueOfB() {
    outTestedClass.setB( "valueOfB" );
    ourTestedClass.insertSpacesInB();
    // some assertions or verifications
}

@Test public void testAMethodThatNeedsAAndB() {
    outTestedClass.setA( "valueOfA" );
    outTestedClass.setB( "valueOfB" );
    ourTestedClass.concatValuesOfAAndB();
    // some assertions or verifications
}

Note that the original example tests display two well-known bad testing practices:

1. Partial mocking of the tested class. As with any other mocking API, the common advice is that partial mocking is to be avoided, only being used in exceptional circunstances.
2. Mocking of simple "getters". Mocking APIs are meant for behavior verification, not for merely providing simple data values. The advice here is to avoid mocking getters and setters, using real, plain objects instead. Again, such methods should be mocked only in exceptional circunstances.

[jOlliv]

Yes, I fully agree with you with your points to the example code.

To use getter for the example was not very helpful, but it was the fastest way to show how the test code is shorter and code duplicates were avoided. Just imagine it were no getters but methods that do some calculation and return the result that is used in the calling method to do some further calculation. In this case you will have some tests that ensure that the called methods works correctly. Then you need some tests that ensure that the calling method works correctly and so you want to mock the called methods with the required results. It is also necessary if you want to test the error handling in the calling method and you cannot produce the required exceptions for the called methods without mocking it.

We have an existing large code base with a few thousand tests. And it would be an appreciable effort to change our test code and modify the production code to become it working with the new jmockit version. So it would be very nice to have the previous behaviour that allows to reuse the expectation blocks also for partial mocking.

[jOlliv]

The last code example suggested that there is a way to test it with jmockit in an nice way. But it isn't. As mentioned in my comment after that the example suggest that the methods are getters and there is a setter for each. Here is an excerpt of the real code.

The example shows how partial mocking is used to test the methods isolated to keep the number of combinations and so the test setup low. I think the real question for the issue is: Shall JMockit support partial mocking or not? If JMockit support partial mocking you also want to be able to extract methods for repeating code and reuse expectation blocks like for normal mocks. So please consider if it is not possible to relax the validation for these cases!

public class MyComponentTest {

@Tested private MyComponent myComponent;
@Mocked private FacesContext mockFacesContext;

@Test public void testSetPageToForValidPage() {
    numberOfPagesIs( 3 );
    expectSetCurrentPageCallWith( 2 );
    myComponent.setPageTo( mockFacesContext, "2" );
}

@Test public void testSetPageToForNegativePageNumber() {
    expectNoSetCurrentPageCall();
    myComponent.setPageTo( mockFacesContext, "-1" );
}

@Test public void testSetPageToForPageGreaterThanNumberOfPages() {
    numberOfPagesIs( 3 );
    expectNoSetCurrentPageCall();
    myComponent.setPageTo( mockFacesContext, "4" );
}

@Test public void testSetCurrentPageIfRemainsOnPage() {
    currentPagesIs( 2 );
    expectNoResetSelection();
    expectNotWriteCurrentPageToViewState();
    myComponent.setCurrentPage( mockFacesContext, 2 );
}

@Test public void testSetCurrentPageIfNotSetYet() {
    currentPagesIs( null );
    expectNoResetSelection();
    expectWriteCurrentPageToViewState( 2 );
    myComponent.setCurrentPage( mockFacesContext, 2 );
}

@Test public void testSetCurrentPageIfPageChanges() {
    currentPagesIs( 2 );
    expectResetSelection();
    expectWriteCurrentPageToViewState( 3 );
    myComponent.setCurrentPage( mockFacesContext, 3 );
}

@Test public void testGetCurrentPageWillFallbackToFirstPage() {
    currentPagesIs( null );
    assertThat( myComponent.getCurrentPage( mockFacesContext ), is( 1 ));
}

@Test public void testGetCurrentPageWithPageSet() {
    currentPagesIs( 2 );
    assertThat( myComponent.getCurrentPage( mockFacesContext ), is( 2 ));
}

private void numberOfPagesIs( final Integer numberOfPages ) {
    new NonStrictExpectations(myComponent) {{
        myComponent.readNumberOfPagesFromViewState( mockFacesContext );
        result = numberOfPages;
    }};
}

private void currentPagesIs( final Integer currentPage ) {
    new NonStrictExpectations(myComponent) {{
        myComponent.readCurrentPageFromViewState( mockFacesContext );
        result = currentPage;
    }};
}

private void expectSetCurrentPageCallWith( final int currentPage ) {
    new NonStrictExpectations(myComponent) {{
        myComponent.setCurrentPage( mockFacesContext, currentPage );
        times = 1;
    }};
}

private void expectNoSetCurrentPageCall() {
    new NonStrictExpectations(myComponent) {{
        myComponent.setCurrentPage( mockFacesContext, anyInt );
        times = 0;
    }};
}

private void expectNoResetSelection() {
    expectResetSelection( false );
}

private void expectResetSelection() {
    expectResetSelection( true );
}

private void expectResetSelection( final boolean callExpected ) {
    new NonStrictExpectations(myComponent) {{
        myComponent.resetSelection( mockFacesContext );
        times = callExpected ? 1 : 0;
    }};
}

private void expectNotWriteCurrentPageToViewState() {
    new NonStrictExpectations(myComponent) {{
        myComponent.writeCurrentPageToViewState( mockFacesContext, anyInt );
        times = 0;
    }};
}

private void expectWriteCurrentPageToViewState( final int currentPage ) {
    new NonStrictExpectations(myComponent) {{
        myComponent.writeCurrentPageToViewState( mockFacesContext, currentPage );
        times = 1;
    }};
}
}

public class MyComponent {

public int getNumberOfPages( FacesContext context ) {
    Integer numberOfPages = readNumberOfPagesFromViewState( context );
    return numberOfPages != null ? numberOfPages : 1;
}

public void setPageTo( FacesContext context, String pageString ) {
    int page = Integer.parseInt( pageString );
    if (page > 0 && page <= getNumberOfPages( context )) {
        setCurrentPage( context, page );
    }
}

public int getCurrentPage( FacesContext context ) {
    final Integer currentPage = readCurrentPageFromViewState( context );
    return currentPage != null ? currentPage : 1;
}

public void setCurrentPage( FacesContext context, int newCurrentPage ) {
    final Integer currentPage = readCurrentPageFromViewState( context );
    if (currentPage != null && currentPage.intValue() == newCurrentPage) {
        return;
    }
    writeCurrentPageToViewState( context, newCurrentPage );
    if (currentPage != null) {
        resetSelection( context );
    }
}
// implementation of the methods not needed for the example
protected void resetSelection( FacesContext context ) { }
protected Integer readNumberOfPagesFromViewState( FacesContext context ) { return 10; }
protected Integer readCurrentPageFromViewState( FacesContext context ) { return 3; }
protected void writeCurrentPageToViewState( FacesContext context, int currentPage ) {}
}

[rliesenfeld]

Yes, this is more like it, thanks!

Using partial mocking on the tested class, as done here, is usually frowned upon, but I wouldn't say categorically that it's always a bad idea. In this example, I would need to better understand what those last four methods in "MyComponent" actually do to reach a conclusion; perhaps it would be easier to mock whichever other types they use, or even to not mock anything at all.

Fortunately, the test failures here can be avoided by making use of verification blocks, rather than verifying all expected method calls through recorded expectations. The rewritten test class, with all tests passing while still doing partial mocking, is as follows:

public class MyComponentTest {

    @Tested MyComponent myComponent;
    @Mocked FacesContext mockFacesContext;

    @Test public void setPageToForValidPage() {
        numberOfPagesIs(3);
        myComponent.setPageTo(mockFacesContext, "2");
        verifySetCurrentPageCallWith(2);
    }

    @Test public void setPageToForNegativePageNumber() {
        new NonStrictExpectations(myComponent) {};
        myComponent.setPageTo(mockFacesContext, "-1");
        verifyNoSetCurrentPageCall();
    }

    @Test public void setPageToForPageGreaterThanNumberOfPages() {
        numberOfPagesIs(3);
        myComponent.setPageTo(mockFacesContext, "4");
        verifyNoSetCurrentPageCall();
    }

    @Test public void setCurrentPageIfRemainsOnPage() {
        currentPagesIs(2);
        myComponent.setCurrentPage(mockFacesContext, 2);
        verifyNotWriteCurrentPageToViewState();
        verifyNoResetSelection();
    }

    @Test public void setCurrentPageIfNotSetYet() {
        currentPagesIs(null);
        myComponent.setCurrentPage(mockFacesContext, 2);
        verifyWriteCurrentPageToViewState(2);
        verifyNoResetSelection();
    }

    @Test public void setCurrentPageIfPageChanges() {
        currentPagesIs(2);
        myComponent.setCurrentPage(mockFacesContext, 3);
        verifyWriteCurrentPageToViewState(3);
        verifyResetSelection();
    }

    @Test public void getCurrentPageWillFallbackToFirstPage() {
        currentPagesIs(null);
        assertEquals(myComponent.getCurrentPage(mockFacesContext), 1);
    }

    @Test public void getCurrentPageWithPageSet() {
        currentPagesIs(2);
        assertEquals(myComponent.getCurrentPage(mockFacesContext), 2);
    }

    void numberOfPagesIs(final Integer numberOfPages) {
        new NonStrictExpectations(myComponent) {{
            myComponent.readNumberOfPagesFromViewState(mockFacesContext);
            result = numberOfPages;
        }};
    }

    void currentPagesIs(final Integer currentPage) {
        new NonStrictExpectations(myComponent) {{
            myComponent.readCurrentPageFromViewState(mockFacesContext);
            result = currentPage;
        }};
    }

    void verifySetCurrentPageCallWith(final int currentPage) {
        new Verifications() {{
            myComponent.setCurrentPage(mockFacesContext, currentPage);
        }};
    }

    void verifyNoSetCurrentPageCall() {
        new Verifications() {{
            myComponent.setCurrentPage(mockFacesContext, anyInt);
            times = 0;
        }};
    }

    void verifyNoResetSelection() {
        verifyResetSelection(false);
    }

    void verifyResetSelection() {
        verifyResetSelection(true);
    }

    void verifyResetSelection(final boolean callExpected) {
        new Verifications() {{
            myComponent.resetSelection(mockFacesContext);
            times = callExpected ? 1 : 0;
        }};
    }

    void verifyNotWriteCurrentPageToViewState() {
        new Verifications() {{
            myComponent.writeCurrentPageToViewState(mockFacesContext, anyInt);
            times = 0;
        }};
    }

    void verifyWriteCurrentPageToViewState(final int currentPage) {
        new Verifications() {{
            myComponent.writeCurrentPageToViewState(mockFacesContext, currentPage);
        }};
    }
}

[rliesenfeld]

For JMockit 1.12, the API will be enhanced to better support use cases like the one exemplified in the two previous comments. Specifically, it will be allowed to annotate an instance field in the test class with both @Tested and @Mocked, so that the tested class gets the semantics of dynamic partial mocking for all tests, without needing to use the "[NonStrict]Expectations(Object...)" constructor.

[Varun200864]

Please provide details. How to add 1.12 in pom.xml file.

[rliesenfeld]

To build the current codebase containing this fix, you'll need to clone/download it from the GitHub repository, then run the "install" goal from the main/pom.xml Maven module, using Maven 3.2 and JDK 1.8. That should be it.

[jOlliv]

Thank you for supporting this use case and for the fast solution!