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

Fix flaky tests in async retry by using a latch instead of sleep. #3563

Merged
merged 5 commits into from
Oct 18, 2024

Conversation

chickenchickenlove
Copy link
Contributor

@chickenchickenlove chickenchickenlove commented Oct 18, 2024

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

  • Removed sleep.
  • Used latch instead.

Result

  • Fixes flaky test cases in 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.

Copy link
Member

@artembilan artembilan left a 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.

Copy link
Member

@artembilan artembilan left a 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.

@chickenchickenlove
Copy link
Contributor Author

chickenchickenlove commented Oct 18, 2024

@artembilan Thanks for your quick review!
I think it's okay. Therefore, I removed @DisabledIfEnvironmentVariable.
I believe the test execution time has been reduced by approximately 40 to 50 seconds.
image

  • Decrease retry count from 5 to 3.
  • Decrease msg count from 100 to 50.
  • Removed sleep.
  • Used latch.

When you have time, please take a look 🙇‍♂️

Copy link
Member

@artembilan artembilan left a 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 java.base@17.0.12/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
        at java.base@17.0.12/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:77)
        at java.base@17.0.12/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
        at java.base@17.0.12/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:500)
        at app//org.springframework.kafka.retrytopic.AsyncCompletableFutureRetryTopicScenarioTests.moreComplexAsyncScenarioTest(AsyncCompletableFutureRetryTopicScenarioTests.java:572)

@artembilan artembilan added this to the 3.3.0-RC1 milestone Oct 18, 2024
@artembilan artembilan merged commit fbb6d88 into spring-projects:main Oct 18, 2024
3 checks passed
@chickenchickenlove
Copy link
Contributor Author

Thanks for your review, and I apologize for the inconvenience 🙇‍♂️.
I decided to use containsExactlyInAnyOther() instead of containsExactly().
Because, I think an issue which i cannot control seems occurs when kafkaTemplate.send(...)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants