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

Add MonoFirstWithValue missing tests #3915

Merged
merged 3 commits into from
Nov 7, 2024

Conversation

subbarao
Copy link
Contributor

@subbarao subbarao commented Nov 4, 2024

Adding missing tests for MonoFirstWithValue.

MonoFirstWithValue tests which verifies cancellation of in flight publishers when first value is emitted is missing.
If you comment https://github.com/reactor/reactor-core/blob/main/reactor-core/src/main/java/reactor/core/publisher/FluxFirstWithValue.java#L283 current tests will pass.
In this test case we start three monos with 3 different sleep. Once Mono with lowest delay completes we verify remaining
monos are cancelled.

@subbarao subbarao requested a review from a team as a code owner November 4, 2024 01:04
@subbarao subbarao changed the title verify inflight mono's cancelled for mono firstWithValue Add MonoFirstWithValue missing tests Nov 4, 2024
@subbarao subbarao force-pushed the mono-first-with-value-tests branch 2 times, most recently from c2a07d6 to d5d79d0 Compare November 4, 2024 01:54
@subbarao
Copy link
Contributor Author

subbarao commented Nov 5, 2024

@violetagg can this pr be reviewed?

@violetagg
Copy link
Member

@subbarao We definitely will make a review so bear with us.

Copy link
Member

@chemicL chemicL left a comment

Choose a reason for hiding this comment

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

@subbarao thanks for the PR. Please rewrite your test in a way that avoids Thread.sleep() but relies on more reliable and timely synchronization mechanisms, e.g. CountDownLatch. Using Thread.sleep in tests and relying on actual time passing contributes negatively to the overall CI duration and can also be prone to flakiness due to different scheduling, delays, etc.

@subbarao
Copy link
Contributor Author

subbarao commented Nov 7, 2024

@subbarao thanks for the PR. Please rewrite your test in a way that avoids Thread.sleep() but relies on more reliable and timely synchronization mechanisms, e.g. CountDownLatch. Using Thread.sleep in tests and relying on actual time passing contributes negatively to the overall CI duration and can also be prone to flakiness due to different scheduling, delays, etc.

updated pr

Copy link
Member

@chemicL chemicL 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 applying the review comment and your contribution! 🚢

@chemicL chemicL added the type/test A change in test sources label Nov 7, 2024
@chemicL chemicL added this to the 3.7.0 milestone Nov 7, 2024
@chemicL chemicL merged commit 3ba3cc1 into reactor:main Nov 7, 2024
7 checks passed
@chemicL
Copy link
Member

chemicL commented Nov 12, 2024

This test seems to be flaky though. I just had it fail after a PR merge in https://github.com/reactor/reactor-core/actions/runs/11793828102

Test class reactor.core.publisher.MonoFirstWithValueTest


MonoFirstWithValueTest > cancelInflightMono() FAILED
    java.lang.AssertionError: 
    Expecting AtomicLong(1) to have value:
      2L
    but did not.
        at reactor.core.publisher.MonoFirstWithValueTest.cancelInflightMono(MonoFirstWithValueTest.java:205)

I will revert. Please try evaluating it by running the test e.g. 1k times (Intellij IDEA allows you to do that using non-gradle test run) and make sure there's some synchronization.

@subbarao my guess is that the validation can happen while the cancellation lambdas are being executed. The thing I'd try would be to have another latch which the test would wait on before all cancellations are done and only then perform assertions. Maybe StepVerifier is not the right tool for this test as this asynchrony is getting in the way - completion can happen simultaneously to the cancellations being processed and that simple API doesn't make it easy to get things properly sequenced.

chemicL added a commit that referenced this pull request Nov 12, 2024
chemicL added a commit that referenced this pull request Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/test A change in test sources
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants