-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Fix flaky tests in async retry by using a latch instead of sleep. #3563
Fix flaky tests in async retry by using a latch instead of sleep. #3563
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for looking into this @chickenchickenlove !
If you think it is OK now, then consider to remove that @DisabledIfEnvironmentVariable
in the AsyncCompletableFutureRetryTopicScenarioTests
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have missed to remove an import org.junit.jupiter.api.condition.DisabledIfEnvironmentVariable;
.
It is still going to fail for Checkstyle.
That's why it is better to run ./gradlew check
locally before pushing changes to the PR.
@artembilan Thanks for your quick review!
When you have time, please take a look 🙇♂️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it is not OK yet:
AsyncCompletableFutureRetryTopicScenarioTests > moreComplexAsyncScenarioTest(TestTopicListener6, MyCustomDltProcessor) FAILED
org.opentest4j.AssertionFailedError:
Expecting actual:
["fail0",
"success1",
"success2",
"fail3",
"success4",
"fail0",
"fail3",
"fail0",
"fail3"]
to contain exactly (and in same order):
["fail0",
"success1",
"success2",
"fail3",
"success4",
"fail3",
"fail3",
"fail0",
"fail0"]
but there were differences at these indexes:
- element at index 5: expected "fail3" but was "fail0"
- element at index 8: expected "fail0" but was "fail3"
at [email protected]/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
at [email protected]/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:77)
at [email protected]/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
at [email protected]/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:500)
at app//org.springframework.kafka.retrytopic.AsyncCompletableFutureRetryTopicScenarioTests.moreComplexAsyncScenarioTest(AsyncCompletableFutureRetryTopicScenarioTests.java:572)
Thanks for your review, and I apologize for the inconvenience 🙇♂️. |
Looks like some are still non-stable: https://github.com/spring-projects/spring-kafka/actions/runs/11433571335/job/31805772798. Feel free to disable them or remove altogether . |
@artembilan , sorry to bother you. |
@chickenchickenlove , no rush. Just sharing with you a status. I don’t think those tests are failing because of some bug in the production code, but more as a design flaw of the tests themselves. Sometimes we are really just cannot model the situation and verify it at the same time. So, that is better to be present as a sample application . |
Motivation
In the previous version, all test cases depended on sleep.
However, it made all test cases flaky.
(Please refer to this comment #3523 (comment))
Modification
Result
AsyncCompletableFutureRetryTopicScenarioTests
,AsyncMonoRetryTopicScenarioTests
.To Reviewer,
I removed a test case. that involved a random situation.
However, it is very hard to handle a random situation with a latch.
Therefore, I decided to remove it and create a new test with a more complex scenario.