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

Log warning when one Bean Override overrides another Bean Override #34056

Closed
sbrannen opened this issue Dec 9, 2024 · 0 comments
Closed

Log warning when one Bean Override overrides another Bean Override #34056

sbrannen opened this issue Dec 9, 2024 · 0 comments
Assignees
Labels
in: test Issues in the test module type: enhancement A general enhancement
Milestone

Comments

@sbrannen
Copy link
Member

sbrannen commented Dec 9, 2024

Overview

As mentioned in cd60a00, it is possible for a bean override to override another logically equivalent bean override.

For example, a @TestBean can override a @MockitoBean, and vice versa.

In fact, it's also possible for a @MockitoBean to override another @MockitoBean, as can be seen in the following test case.

@SpringJUnitConfig
class OverrideMockitoBeanWithMockitoBeanTests {

	@MockitoBean(reset = MockReset.BEFORE)
	MessageService service1;

	@MockitoBean(reset = MockReset.AFTER)
	MessageService service2;

	@Autowired
	List<MessageService> services;

	@Test
	void test() {
		MockingDetails mockingDetails1 = Mockito.mockingDetails(service1);
		MockingDetails mockingDetails2 = Mockito.mockingDetails(service2);

		SoftAssertions.assertSoftly(softly -> {
			softly.assertThat(mockingDetails1.isMock()).as("isMock(service1)").isTrue();
			softly.assertThat(mockingDetails2.isMock()).as("isMock(service2)").isTrue();
			softly.assertThat(getMockReset(service1)).as("MockReset for service1)").isEqualTo(MockReset.BEFORE);
			softly.assertThat(getMockReset(service2)).as("MockReset for service2)").isEqualTo(MockReset.AFTER);
			softly.assertThat(service1).isNotSameAs(service2);
			softly.assertThat(services).hasSize(2);
			softly.assertThat(service1.getMessage()).isNull();
			softly.assertThat(service2.getMessage()).isNull();
		});
	}

	private static MockReset getMockReset(Object mock) {
		Method method = ReflectionUtils.findMethod(MockReset.class, "get", Object.class);
		ReflectionUtils.makeAccessible(method);
		return (MockReset) ReflectionUtils.invokeMethod(method, null, mock);
	}

	interface MessageService {
		String getMessage();
	}

	@Configuration
	static class Config {

		@Bean
		MessageService messageService() {
			return () -> "@Bean";
		}
	}

}

OverrideMockitoBeanWithMockitoBeanTests currently fails as follows.

Multiple Failures (3 failures)

-- failure 1 --
[MockReset for service2)] 
expected: AFTER
 but was: BEFORE

-- failure 2 --
Expected not same: messageService

-- failure 3 --
Expected size: 2 but was: 1 in:
[messageService]

Note, however, that this behavior is not specific to the Bean Override support in Spring Framework: the same behavior exists in Spring Boot for @MockBean and @SpyBean.

Analysis

Now that #34054 has been addressed, we do reject "identical" bean overrides, but we still permit multiple logically equivalent bean overrides.

The challenge lies in figuring out when bean overrides are logically equivalent, and we have to keep the following in mind.

  1. The implementations of equals() and hashCode() in concrete implementations of BeanOverrideHandler (such as TestBeanOverrideHandler and MockitoBeanOverrideHandler) consider two handlers equal based not only on what beans they should override but also on how they should override those beans.
  2. Even if equals() and hashCode() in concrete implementations of BeanOverrideHandler were based solely on what beans they should override, the "phantom read" issue still exists (see aa7b459).

In other words, if we were to change the equals() and hashCode() implementations in handlers, that would allow for some level of static, upfront rejection, but it would still not be possible to reliably predict all cases where a bean override may potentially override another bean override.

Rather, we likely have to track the bean names of all overrides created by BeanOverrideHandlers and ensure that no two handlers attempt to override the same bean.

Proposal

Detect when two bean override handlers attempt to override the same bean and log a warning.

Related Issues

@sbrannen sbrannen added in: test Issues in the test module type: enhancement A general enhancement labels Dec 9, 2024
@sbrannen sbrannen added this to the 6.2.1 milestone Dec 9, 2024
@sbrannen sbrannen self-assigned this Dec 9, 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 type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

1 participant