Add @MockitoSettings(strictness = ...) based on how Mockito is used, not based on version alone

[Jenson3210]

As Mockito only changed default strictness as of 4, repositories using 3 should also be patched.

• fixes The default strictness did not change until v4 #622

[github-actions]

Some suggestions could not be made:

• src/main/java/org/openrewrite/java/testing/jmockit/JMockitBlockRewriter.java
  • lines 112-113

[Jenson3210]

It seems there are some confusing docs online, because there are many ways to use mockito. After our slack discussion I did some investigations.

JUnit5

In a project with dependencies:

dependencies {
    testImplementation(platform("org.junit:junit-bom:5.10.0"))
    testImplementation("org.junit.jupiter:junit-jupiter")
    testImplementation("org.mockito:mockito-core:<mockito_version>")
    testImplementation("org.mockito:mockito-junit-jupiter:<mockito_version>")
}

Given a java class

@ExtendWith(MockitoExtension.class)
class TestingTest {

    @Mock
    private List<String> mockList;

    @Test
    void testing() {
        // This would raise an UnnecessaryStubbingException as this mocked method is never called
        when(mockList.add("one")).thenReturn(true); // this won't get called
        System.out.println("Hello world!");
    }
}

This is throwing exception in following Mockito versions:

• 4.11.0 (latest 4.x)
• 4.0.0
• 3.12.4 (latest 3.x)
• 3.0.0
• 2.28.2 (latest 2.x)
• 2.17.0 (earliest mockito-junit-jupiter)

JUnit4

For JUnit4 it seems there is a difference between the way they are ran

In a project with dependencies

dependencies {
    testImplementation("junit:junit:4.12")
    testImplementation("org.mockito:mockito-core:<mockito_version>")
}

@RunWith(MockitoJUnitRunner.class)

@RunWith(MockitoJUnitRunner.class)
public class TestingTest {

    @Mock
    private List<String> mockList;

    @Test
    public void testing() {
        when(mockList.add("one")).thenReturn(true); // this won't get called
        System.out.println("Hello world!");
    }
}

This is succesfull in Mockito versions:

• 1.10.19 (latest 1.x)

This is throwing exception in following Mockito versions:

• 3.12.4 (latest 3.x)
• 3.0.0
• 2.28.2 (latest 2.x)
• 2.1.0 (earliest 2.x)

This exception can be silenced with @RunWith(MockitoJUnitRunner.Silent.class) instead.

@\Rule MockitoRule

public class TestingTest {

    @Rule
    public MockitoRule rule = MockitoJUnit.rule();

    @Mock
    private List<String> mockList;

    @Test
    public void testing() {
        when(mockList.add("one")).thenReturn(true); // this won't get called
        System.out.println("Hello world!");
    }
}

This is succesfull in Mockito versions:

• 1.10.19 (latest 1.x)
• 2.1.0 -> A Hint is displayed, which can be removed by using MockitoJUnit.rule().silent() instead
• 2.3.0 -> Introduced Strictness which can be set to silent, warn(default) or strict(fail if...)
• 2.28.2 -> see 2.3.0
• 3.0.0 -> same as 2.3.0
• 4.0.0 -> same as 2.3.0
• 4.11.0 -> same as 2.3.0

Mockito.mock

public class TestingTest {

    private List<String> mockList = Mockito.mock(List.class);

    @Test
    public void testing() {
        when(mockList.add("one")).thenReturn(true); // this won't get called
        System.out.println("Hello world!");
    }
}

This is succesfull in Mockito versions:

• 1.10.19
• 2.1.0
• 2.28.2
• 3.0.0
• 4.0.0
• 4.11.0

MockitoAnnotations.initMocks

public class TestingTest {

    @Mock
    private List<String> mockList;

    @Before
    public void setUp() {
        MockitoAnnotations.initMocks(this);
    }

    @Test
    public void testing() {
        when(mockList.add("one")).thenReturn(true); // this won't get called
        System.out.println("Hello world!");
    }
}

This is succesfull in Mockito versions:

• 1.10.19
• 2.1.0
• 2.28.2
• 3.0.0
• 4.0.0
• 4.11.0

[timtebeek]

Oh wow, no wonder the docs are confusing. 😅 Thanks a lot for the detailed analysis and report!

Wondering what to do here, as we do have recipes to migrate most of those onto JUnit 5, which would then start to fail on 2.17+ if we do not introduce @MockitoSettings(strictness = Strictness.WARN). But that would also mean you could potentially migrate a 4.x Mockito.mock or MockitoAnnotations.initMocks class to the MockitoExtension and have to introduce that @MockitoSettings there to keep the old behaviour.

Sounds like potentially our ScanningRecipe needs to keep track of what each test class was using (whenever we come from a pre-5 version), and add the annotation to the matching tests, if we want to be minimal in when we introduce that annotation. Would you agree @Jenson3210 ?

[Jenson3210]

I was at the same time writing a summary here. 😄
I think the actual bug is inside the MockitoJUnitToMockitoExtension recipe.
This one is responsible for migrating MockitoRule to Extension.

For Mockito.mock and MockitoAnnotations.initMocks there is no problem as their is no exception.

For the MockitoRule, the responsability is in the MockitoJUnitToMockitoExtension recipe. This one does not set MockitoSettings if that would be necessary --> Is this not the real bug then?

For the RunWith(MockitoJUnitRunner.class), we have RunnerToExtension, for the silenced equivalent we have MockitoJUnitRunnerSilentToExtension

I feel this means RetainStrictnessWarn does a check it maybe did not really needed to begin with. But as it is there, and this recipe is used in a declarative recipe also already related to specific versions, maybe it is better to no touch that one but to fix the rule -> settings migration so that it adds the annotations if required.

What do you think @timtebeek

[timtebeek]

I agree based on the above that perhaps indeed adding the settings should be done at the same time as migrating from rules to extension. That seems to be the point where the behavior and defaults are changed, in a way that can not be reliably determined after those recipes have run based on the version number alone. I'd be open to even remove the RetainStrictnessWarn recipe; better to have one recipe reliably make those changes.

[Jenson3210]

I'll get started on some unit tests.
Basically after a round of testing there,
If both silent and strictness are mentioned, strictness takes precedence.
If any of the two is set, annotation should be added.

• silent/Lenient-> settings = lenient
• warn -> settings = warn
• strict -> settings = strict

i think that mapping is the best

[Jenson3210]

@timtebeek, The code is ready and test are added.

However, I am getting this issue with my 4 added tests. My class is all the time being prefixed with a space and I have no clue. Would you happen to know?

[github-actions]

Some suggestions could not be made:

• src/test/java/org/openrewrite/java/testing/mockito/MockitoJUnitToMockitoExtensionTest.java
  • lines 19-20

[Jenson3210]

Looks like this bug is also present in current recipe already
strange the tests run...

[timtebeek]

Thanks for the detailed work here! Great to see this handled better in a way that wont break the tests at runtime. I'll follow up separately to remove the old RetainStrictnessWarn.