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

Fixed non-idempotent test ConfigRefreshTests.verifyGetApplications #4279

Merged
merged 2 commits into from
Jun 27, 2024

Conversation

kaiyaok2
Copy link
Contributor

@kaiyaok2 kaiyaok2 commented Apr 26, 2024

Motivation:

The test org.springframework.cloud.netflix.eureka.config.ConfigRefreshTests.verifyGetApplications is not idempotent and fails in the second run in the same environment, because it pollutes a state reused by itself. Specifically, it has a mocked EurekaClient instance shared across different test runs. The test verifyGetApplications() checks whether getApplications() is called the correct number of times (3) when a refresh event is fired, and the check fails in the second run as re-execution of publisher.publishEvent() makes more calls to getApplications() on the shared EurekaClient instance. A fix is recommended since unit tests should be idempotent (deterministically passing in repeated runs).

Stacktrace of failure in the second run:

org.mockito.exceptions.verification.TooManyActualInvocations: 
cloudEurekaClient.getApplications();
Wanted 3 times:
-> at com.netflix.discovery.DiscoveryClient.getApplications(DiscoveryClient.java:566)
But was 5 times:
-> at org.springframework.cloud.netflix.eureka.serviceregistry.EurekaServiceRegistry.maybeInitializeClient(EurekaServiceRegistry.java:83)
-> at org.springframework.cloud.netflix.eureka.EurekaDiscoveryClientConfiguration$EurekaClientConfigurationRefresher.onApplicationEvent(EurekaDiscoveryClientConfiguration.java:91)
-> at org.springframework.cloud.netflix.eureka.serviceregistry.EurekaServiceRegistry.maybeInitializeClient(EurekaServiceRegistry.java:83)
-> at org.springframework.cloud.netflix.eureka.EurekaDiscoveryClientConfiguration$EurekaClientConfigurationRefresher.onApplicationEvent(EurekaDiscoveryClientConfiguration.java:91)
-> at org.springframework.cloud.netflix.eureka.serviceregistry.EurekaServiceRegistry.maybeInitializeClient(EurekaServiceRegistry.java:83)
	at com.netflix.discovery.DiscoveryClient.getApplications(DiscoveryClient.java:566)
	at org.springframework.cloud.netflix.eureka.config.ConfigRefreshTests.verifyGetApplications(ConfigRefreshTests.java:55)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)

Proposed Fix

Add a flag to only invoke publisher.publishEvent() in the first run.

Copy link
Collaborator

@OlgaMaciaszek OlgaMaciaszek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, @kaiyaok2. Makes sense. Please add your full name with @author tag to the class javadoc comment. Please update the date in the license String to 2013-2024.

@kaiyaok2
Copy link
Contributor Author

@OlgaMaciaszek Changes made. Thanks!

Copy link
Collaborator

@OlgaMaciaszek OlgaMaciaszek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @kaiyaok2. LGTM.

@OlgaMaciaszek OlgaMaciaszek merged commit f47e93c into spring-cloud:main Jun 27, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants