MockitoExtension gets added even when not necessary

[DidierLoiseau]

What version of OpenRewrite are you using?

I am using

• OpenRewrite v8.16.1
• Maven plugin v6.25.0
• rewrite-recipe-bom 3.20.0

How are you running OpenRewrite?

I am using the Maven plugin, and my project is a single module project.

What is the smallest, simplest way to reproduce the problem?

Consider these classes:

@ExtendWith(MockitoExtension.class)
abstract class BaseTest {
}

class ChildTest extends BaseTest {
	@Mock
	Object underTest;

	@Test
	void myTest() {
		assertNotNull(underTest);
	}
}

class ExplicitInitTest {
	@Mock
	Object underTest;

	@BeforeEach
	void setup() {
		MockitoAnnotations.openMocks(this);
	}

	@Test
	void myTest() {
		assertNotNull(underTest);
	}
}

(the next one only with Spring Boot < 4.0, I think)

@ExtendWith(SpringExtension.class)
class SpringExtensionTest {
	@Mock
	Object underTest;

	@Test
	void myTest() {
		assertNotNull(underTest);
	}
}

@Target({ ElementType.TYPE })
@Retention(RetentionPolicy.RUNTIME)
@ExtendWith(MockitoExtension.class)
@interface MyMockitoAnnotation {
}

@MyMockitoAnnotation
class CustomAnnotationTest {
	@Mock
	Object underTest;

	@Test
	void myTest() {
		assertNotNull(underTest);
	}
}

These tests pass without an explicit @ExtendWith(MockitoExtension.class) on the test class itself.

What did you expect to see?

OpenRewrite should not alter these classes when running AddMockitoExtensionIfAnnotationsUsed, nor Java or Spring Boot upgrades.

What did you see instead?

The test classes all see @ExtendWith(MockitoExtension.class) being added:

@ExtendWith(MockitoExtension.class)
public class ChildTest extends BaseTest {
	@Mock
	Object underTest;

	@Test
	void myTest() {
		assertNotNull(underTest);
	}
}

@ExtendWith(MockitoExtension.class)
public class ExplicitInitTest {
	@Mock
	Object underTest;

	@BeforeEach
	void setup() {
		MockitoAnnotations.openMocks(this);
	}

	@Test
	void myTest() {
		assertNotNull(underTest);
	}
}

@ExtendWith(SpringExtension.class)
@ExtendWith(MockitoExtension.class)
public class SpringExtensionTest {
	@Mock
	Object underTest;

	@Test
	void myTest() {
		assertNotNull(underTest);
	}
}

@ExtendWith(MockitoExtension.class)
@MyMockitoAnnotation
class CustomAnnotationTest {
	@Mock
	Object underTest;

	@Test
	void myTest() {
		assertNotNull(underTest);
	}
}

The extension shouldn’t be added when one of the following is true:

• a parent class already has it
• there is an explicit call to MockitoAnnotations.initMocks() or MockitoAnnotations.openMocks() (it could be in a parent class too!)
• using the SpringExtension in SB < 3.4, maybe? (in 3.4 this usage appears to have been tacitly deprecated, and removed in SB 4, hence Add @ExtendWith(MockitoExtension.class) Test using Mockito Annotations #848 )
• the test class (or one of its parents) is meta-annotated with one of these extensions
• maybe other conditions?

What is the full stack trace of any errors you encountered?

[WARNING] Changes have been made to src\test\java\…\ChildTest.java by:
[WARNING]         org.openrewrite.java.spring.boot3.UpgradeSpringBoot_3_5
[WARNING]             org.openrewrite.java.spring.boot3.UpgradeSpringBoot_3_4
[WARNING]                 org.openrewrite.java.spring.boot3.UpgradeSpringBoot_3_3
[WARNING]                     org.openrewrite.java.spring.boot3.UpgradeSpringBoot_3_2
[WARNING]                         org.openrewrite.java.spring.boot3.UpgradeSpringBoot_3_1
[WARNING]                             org.openrewrite.java.spring.boot3.UpgradeSpringBoot_3_0
[WARNING]                                 org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_7
[WARNING]                                     org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_6
[WARNING]                                         org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_5
[WARNING]                                             org.openrewrite.java.spring.boot2.UpgradeSpringBoot_2_4
[WARNING]                                                 org.openrewrite.java.spring.boot2.SpringBoot2JUnit4to5Migration
[WARNING]                                                     org.openrewrite.java.testing.junit5.JUnit4to5Migration
[WARNING]                                                         org.openrewrite.java.testing.junit5.UseMockitoExtension
[WARNING]                                                             org.openrewrite.java.testing.mockito.Mockito1to4Migration
[WARNING]                                                                 org.openrewrite.java.testing.mockito.Mockito1to3Migration
[WARNING]                                                                     org.openrewrite.java.testing.mockito.CleanupMockitoImports
[WARNING]                                                                     org.openrewrite.java.testing.mockito.AddMockitoExtensionIfAnnotationsUsed

(log is similar for the other classes)

Are you interested in contributing a fix to OpenRewrite?

I’m not too sure how to implement all these negative cases, especially the ones related to parent classes.

Maybe for the time being, a temporary solution would be to edge on the safe side and only make the change when SpringExtension is used, and only during the Spring Boot 4 migration? This appears to have been the main target from #848 (by @MBoegers).

[MBoegers]

@DidierLoiseau You are right, replacing SpringExtension in the Spring Boot 4 migration was the root intention.
Then we thought: Why not add the annotation in general when moving from Mockito 1 to 3, back when these were introduced?

@ExtendWith(SpringExtension.class) does not include @ExtendWith(MockitoExtension.class). SpringExtension spins up a Spring Context and injects MockBeans into the annotated fields, and MockitoExtension creates plain Mockito mocks into the fields. So only one of them made sense in pre Spring Boot 4.0 codebase.

If a test uses @Mock/Spy but does not have the MockitoExtension than

1. There is a SpringExtension annotated; a replacement would break the test semantics.
2. An extended class or interface (multiple levels possible) defined either MockitoExtension or SpringExtension
3. There is a custom annotation that defined MockitoExtension or SpringExtension at any level.
4. The test never worked as expected, as the mocks were never created.

Case 4 is a missing Mockito1to3Migration migration that we tried to address with the inclusion that deep down.

In case 1 they should have used @MockBean instead of @Mock (same for Spy), we have to dig deeper to fully understand. Same holds for case 2 and 3 if the base test classes or annotations use SpringExtension.

Case 2 and 3 for MockitoExtension usage require more in-depth analysis, because we have to collect all annotations of super classes/interfaces and annotations they inherit by their annotations.

@DidierLoiseau you would help me a lot with fact-checking me on this one.

[DidierLoiseau]

@MBoegers thanks for the quick reply!

Your list is missing the case of using plain MockitoAnnotations.initMocks(testClass) (required in Mockito 1, I believe) or MockitoAnnotations.openMocks(testClass) (as in ExplicitInitTest above), either in the test class or a parent class. Such usage should still work today, and it might still be required in specific cases where MockitoExtension wouldn’t work.

I agree that case 1 is a strange usage, and people should probably not do that. I have seen it in our code and basically it was a unit test where replacing SpringExtension by MockitoExtension worked the same, so this is what I did. This is maybe the only case where I see OpenRewrite could help, but it’s also one where I think a developer has to stop and think about the best approach:

• Is it really appropriate to mix SpringExtension and @Mock? I.e. mix a Spring context with plain Mockito mocks? If yes, MockitoExtension could simply be added, but that seems awkward.
• Should MockitoExtension be used instead?
• Should @MockBean be used instead?

I doubt a recipe could make that choice automatically.

Cases 2 and 3 seem rather complex, and detecting initMocks()/openMocks() even more. In these cases, it seems clear that the extension shouldn’t be added.

I don’t see how case 4 could be handled by Mockito1to3Migration. Is it really the purpose of this recipe to fix broken Mockito 1 or 2 usage? I would expect that the migration keeps the tests working like-for-like. Adding the extension may actually break a working test (because the Mockito annotations were actually irrelevant).

According to me, only case 1 should thus be covered. I think just adding MockitoExtension in that case should be fine, as someone should always review OpenRewrite’s changes and notice the strange combination of extensions. I would thus propose to modify the recipe such that it only makes the change when SpringExtension is present. There could still be cases where it is unnecessary, but at least it gives some visibility of the problem to the reviewer.

Maybe the recipe could also add a comment about the reason of the change? Something like

MockitoExtension added by OpenRewrite. @Captor, @Mock, @Spy or @InjectMocks detected in this class together with SpringExtension. This is unusual and will not work as intended as of Spring Boot 4. See https://github.com/spring-projects/spring-boot/wiki/Spring-Boot-4.0-Migration-Guide#mockito-captor-and-mock-annotations

[DidierLoiseau]

Some additional thoughts as I was considering the recipe goals here: for me, migration recipes should only make changes related to the ongoing migration and should thus generally not try to fix issues that were already present in the code (there might be specific recipes for that, but they shouldn't be included in migrations).

This is somewhat related to the Do no harm guideline:

Relatedly, when making changes, always strive to make the most minimal, least invasive version of that change. For instance, in the NoGuavaImmutableSetOf recipe, we restrict the recipe from changing any code that does not use at least Java 9.

In that sense, I am a bit confused by the

Then we thought: Why not add the annotation in general when moving from Mockito 1 to 3, back when these were introduced?

You ask “Why not?”, I ask “Why?” 😄 Was there something wrong in Mockito1to3Migration that was breaking tests?

In fact, adding the extension should certainly not be done in Mockito1to3Migration since, at this point, you don’t know which version of JUnit the project is using (or am I missing something?). At best it could only be part of JUnit4to5Migration.

I tried to dig into Mockito’s git history and found that the first JUnit Runner was introduced in mockito/mockito@67a7c70, for Mockito 1.6. Before that, only initMocks() was available (as I understand it, they provided a runner example, which was removed in the same commit) – but this usage should still work today.

So to migrate Mockito 1.x or 2.x to 3.x you need:

• for initMocks() calls, either:
  • keep them (but the method is deprecated),
  • replace them with openMocks() (but then you need to deal with the returned AutoCloseable)
  • convert them to MockitoJUnitRunner/MockitoExtension (probably too difficult because there might be a good reason why it was not used in the first place. Maybe some trivial cases can be identified, like a setup method with only that call.)
  • remove them if the runner/extension is already used (covered already by RemoveInitMocksIfRunnersSpecified, I see)
• for MockitoJUnit*Runner: convert them to MockitoJUnitRunner/MockitoExtension depending on the JUnit version used

I don’t think there is any case where the extension can be blindly added without considering anything else. Moreover, it is not required for the migration, as far as I can tell.