Skip to content

Conversation

@hydrationn
Copy link

This PR removes Thread.sleep from async tests in DefaultAsyncServerResponseTests
and replaces it with a non-blocking CompletableFuture pattern. This makes the
test more deterministic and avoids timing-based flakiness.

🔍 What was changed

  • Removed Thread.sleep(500) inside CompletableFuture.supplyAsync
  • Replaced it with a non-blocking test pattern:
    • Created an incomplete CompletableFuture
    • Completed it asynchronously using new Thread(() -> future.complete(...))
  • This makes the test more deterministic and avoids relying on timing-based behavior

💡 Why this change

Using Thread.sleep in tests is generally discouraged because:

  • It increases test execution time
  • It may lead to unstable test results depending on system timing
  • It does not reflect real asynchronous behavior

The updated approach ensures:

  • Faster test execution
  • More reliable async behavior
  • Clearer test intent

📌 Notes

No functional changes were made to production code.
This update improves the internal test quality and stability.

Signed-off-by: suhwa Park <suhwaparkk1031@gmail.com>
Signed-off-by: hydrationn <suhwaparkk1031@gmail.com>
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Dec 10, 2025
@bclozel
Copy link
Member

bclozel commented Dec 17, 2025

I don't think this change really improves things.
This change starts a new Thread that immediately completes the future. I don't understand why a new Thread should be started here, and completing immediately the future means that the tested component will never wait for the value since it will be present from the start.

We might be able to improve tests here, but this change doesn't look right to me.

@bclozel bclozel closed this Dec 17, 2025
@bclozel bclozel added status: declined A suggestion or change that we don't feel we should currently apply and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Dec 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status: declined A suggestion or change that we don't feel we should currently apply

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants