Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MockitoTestExecutionListener no longer opens mocks in the prepareTestInstance() callback #33690

Closed
wilkinsona opened this issue Oct 11, 2024 · 8 comments
Assignees
Labels
in: test Issues in the test module status: superseded An issue that has been superseded by another type: regression A bug that is also a regression

Comments

@wilkinsona
Copy link
Member

Affects: 6.2.0-SNAPSHOT

This is a recent regression from 6.2.0-RC1. I believe it was introduced in eb4bf1c#diff-02b9cc5ba2d6121e1a5f63ad339bb82fa8f3531d568c43d07fac61ade962c0cd where the override of prepareTestInstance was removed. The result is that trying to call a @Mock-annotated field in a @BeforeAll method fails with an NPE.

This should reproduce the regression:

package com.example;

import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.BDDMockito.given;

import java.util.List;

import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.Mock;
import org.springframework.test.context.junit.jupiter.SpringExtension;

@ExtendWith(SpringExtension.class)
class MockInitInBeforeAllTests {
	
	@Mock
	private static List<String> mock;

	@BeforeAll
	static void setUp() {
		given(mock.size()).willReturn(1);
	}

	@Test
	void shouldSetUpSuccessfully() {
		assertThat(mock.size()).isEqualTo(1);
	}

}

Another, more advanced variant that uses a custom test instance lifecycle and which is closer to the Spring Boot test that found the regression also fails with 6.2.0-SNAPSHOT:

package com.example;

import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.BDDMockito.given;

import java.util.List;

import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.TestInstance;
import org.junit.jupiter.api.TestInstance.Lifecycle;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.Mock;
import org.springframework.test.context.junit.jupiter.SpringExtension;

@TestInstance(Lifecycle.PER_CLASS)
@ExtendWith(SpringExtension.class)
class MockInitInBeforeAllTests {
	
	@Mock
	private List<String> mock;

	@BeforeAll
	void setUp() {
		given(this.mock.size()).willReturn(1);
	}

	@Test
	void shouldSetUpSuccessfully() {
		assertThat(mock.size()).isEqualTo(1);
	}

}
@wilkinsona wilkinsona changed the title Static @Mock fields are no longer initialized before a @BeforeAll method is call resulting in an NPE Static @Mock fields are no longer initialized before a @BeforeAll method is called resulting in an NPE Oct 11, 2024
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Oct 11, 2024
@jhoeller jhoeller added in: test Issues in the test module type: regression A bug that is also a regression and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Oct 11, 2024
@jhoeller jhoeller added this to the 6.2.0-RC2 milestone Oct 11, 2024
@sbrannen sbrannen self-assigned this Oct 12, 2024
@sbrannen
Copy link
Member

Thanks for reporting the issue, @wilkinsona. 👍🏻

The result is that trying to call a @Mock-annotated field in a @BeforeAll method fails with an NPE.

I confirm that this is a change in behavior in Spring Framework since 6.2 RC1, and it is in fact caused by the removal of the prepareTestInstance() override in MockitoTestExecutionListener.

However, that change was made in order to provide better support for @Nested test classes with the MockitoTestExecutionListener.

If we reinstate the prepareTestInstance() override as it was, we end up closing the MockitoSession for the enclosing class when we open a new MockitoSession for the @Nested class, and that means that mocks created in the enclosing class will not be tracked by the MockitoSession created for the @Nested class: in other words, strictness and unnecessary stubbing would not be supported for mocks created in the enclosing class.

On the other hand, if we reintroduce the prepareTestInstance() override and do not close the existing MockitoSession, we get an UnfinishedMockingSessionException for the @⁠Nested test class due to the fact that a MockitoSession was already created for the current thread for the enclosing test class.

In retrospect, neither of those scenarios is desirable, and I'll address that topic in a new GitHub issue.

In addition, I confirmed that Spring Boot never supported the use cases you have provided, and Mockito's own MockitoExtension also does not support initialization for @Mock on a static field before a @BeforeAll method is invoked.

For example, the following all fail due to a NullPointerException.

@SpringBootTest
@TestExecutionListeners({
	org.springframework.boot.test.mock.mockito.MockitoTestExecutionListener.class,
	org.springframework.boot.test.mock.mockito.ResetMocksTestExecutionListener.class
})
class MockInitInBeforeAllWithSpringBootTests {

	@Mock
	static List<String> mock;

	@BeforeAll
	static void setUp() {
		given(mock.size()).willReturn(1);
	}

	@Test
	void shouldSetUpSuccessfully() {
		assertThat(mock.size()).isEqualTo(1);
	}

}
@SpringBootTest
@TestExecutionListeners({
	org.springframework.boot.test.mock.mockito.MockitoTestExecutionListener.class,
	org.springframework.boot.test.mock.mockito.ResetMocksTestExecutionListener.class
})
class MockInitInBeforeEachWithSpringBootTests {

	@Mock
	List<String> mock;

	@BeforeEach
	void setUp() {
		given(mock.size()).willReturn(1);
	}

	@Test
	void shouldSetUpSuccessfully() {
		assertThat(mock.size()).isEqualTo(1);
	}

}
@ExtendWith(MockitoExtension.class)
class MockInitInBeforeAllWithMockitoExtensionTests {

	@Mock
	private static List<String> mock;

	@BeforeAll
	static void setUp() {
		given(mock.size()).willReturn(1);
	}

	@Test
	void shouldSetUpSuccessfully() {
		assertThat(mock.size()).isEqualTo(1);
	}

}

However, the MockitoExtension and Spring Framework 6.2 snapshots do support initialization for @Mock on an instance field before a @BeforeEach method is invoked, as demonstrated in the following examples which succeed.

@ExtendWith(MockitoExtension.class)
class MockitoInitInBeforeEachWithMockitoExtensionTests {

	@Mock
	private List<String> mock;

	@BeforeEach
	void setUp() {
		given(mock.size()).willReturn(1);
	}

	@Test
	void shouldSetUpSuccessfully() {
		assertThat(mock.size()).isEqualTo(1);
	}

}
@ExtendWith(SpringExtension.class)
class MockitoInitInBeforeEachWithSpringExtensionTests {

	@Mock
	private List<String> mock;

	@BeforeEach
	void setUp() {
		given(mock.size()).willReturn(1);
	}

	@Test
	void shouldSetUpSuccessfully() {
		assertThat(mock.size()).isEqualTo(1);
	}

}

In summary, if we wish to continue using the MockitoSession I do not think we can reliably support the inialization of static @Mock, @Spy or @Captor fields before a @BeforeAll method is invoked, since we need to properly support @Nested test classes as well.

In light of that, I am closing this issue.

@sbrannen sbrannen closed this as not planned Won't fix, can't repro, duplicate, stale Oct 12, 2024
@sbrannen sbrannen removed this from the 6.2.0-RC2 milestone Oct 12, 2024
@sbrannen
Copy link
Member

@sbrannen sbrannen added the status: superseded An issue that has been superseded by another label Oct 12, 2024
@wilkinsona
Copy link
Member Author

wilkinsona commented Oct 12, 2024

I’m surprised that this has been closed, particularly with the assertion that “Spring Boot never supported the use cases you have provided” when, as I mentioned above, there is a test in Spring Boot that started failing with 6.2.0-SNAPSHOT. It succeeds with 6.1.x (Boot 3.3) and with 6.2.0-RC1. You can find the test class here. It’s ConfigureMockInBeforeAll that is affected by the regression.

@sbrannen
Copy link
Member

Hi Andy,

Thanks for the feedback.

I’m surprised that this has been closed, particularly with the assertion that “Spring Boot never supported the use cases you have provided” when, as I mentioned above, there is a test in Spring Boot that started failing with 6.2.0-SNAPSHOT. It succeeds with 6.1.x (Boot 3.3) and with 6.2.0-RC1.

I misunderstood your original claim. I wrongly assumed you meant the regression was for behavior introduced in Spring Framework that did not exist previously in Spring Boot, but now I see that the MockitoTestExecutionListener used to open mocks in the prepareTestInstance() callback here.

My apologies!

I'm reopening this issue to make sure we take that into consideration.

@sbrannen sbrannen reopened this Oct 13, 2024
@sbrannen sbrannen changed the title Static @Mock fields are no longer initialized before a @BeforeAll method is called resulting in an NPE MockitoTestExecutionListener no longer opens mocks in the prepareTestInstance() callback Oct 13, 2024
@sbrannen sbrannen removed the status: superseded An issue that has been superseded by another label Oct 13, 2024
@sbrannen sbrannen added this to the 6.2.0-RC2 milestone Oct 13, 2024
snicoll added a commit to spring-projects/spring-boot that referenced this issue Oct 14, 2024
@ryanjbaxter
Copy link

The Spring Cloud team is running into an issue with this change as well (I think). This test has started failing because the variables annotated with @Mock are null.

Caused by: org.springframework.beans.BeanInstantiationException: Failed to instantiate [org.springframework.cloud.client.circuitbreaker.Customizer]: Factory method 'slowCustomizer' threw exception with message: 
Argument passed to when() is null!
Example of correct stubbing:
    doThrow(new RuntimeException()).when(mock).someMethod();
Also, if you use @Mock annotation don't miss openMocks()
	at org.springframework.beans.factory.support.SimpleInstantiationStrategy.lambda$instantiate$0(SimpleInstantiationStrategy.java:199) ~[spring-beans-6.2.0-SNAPSHOT.jar:6.2.0-SNAPSHOT]
	at org.springframework.beans.factory.support.SimpleInstantiationStrategy.instantiateWithFactoryMethod(SimpleInstantiationStrategy.java:88) ~[spring-beans-6.2.0-SNAPSHOT.jar:6.2.0-SNAPSHOT]
	at org.springframework.beans.factory.support.SimpleInstantiationStrategy.instantiate(SimpleInstantiationStrategy.java:168) ~[spring-beans-6.2.0-SNAPSHOT.jar:6.2.0-SNAPSHOT]
	at org.springframework.beans.factory.support.ConstructorResolver.instantiate(ConstructorResolver.java:653) ~[spring-beans-6.2.0-SNAPSHOT.jar:6.2.0-SNAPSHOT]
	... 110 common frames omitted
Caused by: org.mockito.exceptions.misusing.NullInsteadOfMockException: 
Argument passed to when() is null!
Example of correct stubbing:
    doThrow(new RuntimeException()).when(mock).someMethod();
Also, if you use @Mock annotation don't miss openMocks()
	at org.springframework.cloud.circuitbreaker.resilience4j.Resilience4JBulkheadIntegrationTest$Application.slowCustomizer(Resilience4JBulkheadIntegrationTest.java:209) ~[test-classes/:na]
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) ~[na:na]
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77) ~[na:na]
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) ~[na:na]
	at java.base/java.lang.reflect.Method.invoke(Method.java:569) ~[na:na]
	at org.springframework.beans.factory.support.SimpleInstantiationStrategy.lambda$instantiate$0(SimpleInstantiationStrategy.java:171) ~[spring-beans-6.2.0-SNAPSHOT.jar:6.2.0-SNAPSHOT]
	... 113 common frames omitted

@wilkinsona
Copy link
Member Author

We've come full circle here and decided to reinstate support for the init of Mockito's mocks in Spring Boot. I think this can be closed in favor of spring-projects/spring-boot#42708. The goals of reinstating the support in Boot are two-fold:

  1. It provides a smoother upgrade experience for users as it should avoid the problems described above
  2. It allows Framework's new Mockito-related support to get off on the right foot by not initing Mockito's mocks

Hopefully this buys us some time to provide a better solution in Boot 4.0/Framework 7.0 while not making things rough for Boot users when they upgrade to 3.4.

@ryanjbaxter
Copy link

Thanks @wilkinsona! The changes in Boot seem to be working for us!

@sbrannen
Copy link
Member

@sbrannen sbrannen closed this as not planned Won't fix, can't repro, duplicate, stale Oct 16, 2024
@sbrannen sbrannen added the status: superseded An issue that has been superseded by another label Oct 16, 2024
@sbrannen sbrannen removed this from the 6.2.0-RC2 milestone Oct 16, 2024
ryanjbaxter added a commit to spring-cloud/spring-cloud-circuitbreaker that referenced this issue Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: test Issues in the test module status: superseded An issue that has been superseded by another type: regression A bug that is also a regression
Projects
None yet
Development

No branches or pull requests

5 participants