refactor(showcase): migrate Thread.sleep to Awaitility in integration tests#13519
refactor(showcase): migrate Thread.sleep to Awaitility in integration tests#13519blakeli0 wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces Awaitility to replace manual busy-waiting and Thread.sleep calls across several integration tests, improving test reliability. The review feedback highlights opportunities to optimize these changes by avoiding redundant and potentially expensive calls to metricReader.collectAllMetrics(). The reviewer suggests using Hamcrest matchers with Awaitility to return the evaluated value directly, or utilizing an AtomicReference to capture the metrics during the polling phase.
| Awaitility.await() | ||
| .atMost(Duration.ofSeconds(10)) | ||
| .pollInterval(Duration.ofMillis(100)) | ||
| .until(() -> !metricReader.collectAllMetrics().isEmpty()); |
There was a problem hiding this comment.
Efficiency & Readability Improvement: Calling metricReader.collectAllMetrics() inside the until condition and then calling it again immediately after to assign to actualMetrics is redundant and inefficient. InMemoryMetricReader.collectAllMetrics() can be an expensive operation.
Instead, you can use Awaitility's ability to return the evaluated value that satisfies a Hamcrest matcher, which avoids the duplicate call entirely:
Collection<MetricData> actualMetrics =
Awaitility.await()
.atMost(Duration.ofSeconds(10))
.pollInterval(Duration.ofMillis(100))
.until(metricReader::collectAllMetrics, org.hamcrest.Matchers.not(org.hamcrest.Matchers.empty()));| Awaitility.await() | ||
| .atMost(Duration.ofSeconds(5)) | ||
| .pollInterval(Duration.ofMillis(10)) | ||
| .until(() -> !metricReader.collectAllMetrics().isEmpty()); |
There was a problem hiding this comment.
Efficiency & Readability Improvement: Calling metricReader.collectAllMetrics() inside the until condition and then calling it again immediately after to assign to metrics is redundant and inefficient.
You can use Awaitility's ability to return the evaluated value that satisfies a Hamcrest matcher to avoid the duplicate call:
Collection<MetricData> metrics =
Awaitility.await()
.atMost(Duration.ofSeconds(5))
.pollInterval(Duration.ofMillis(10))
.until(metricReader::collectAllMetrics, org.hamcrest.Matchers.not(org.hamcrest.Matchers.empty()));Note: This pattern occurs in multiple places throughout this file (e.g., lines 200, 235, 380, 432, 478, 561, 735). Consider applying this improvement to all of them.
| try { | ||
| Awaitility.await() | ||
| .atMost(java.time.Duration.ofSeconds(NUM_DEFAULT_FLUSH_ATTEMPTS)) | ||
| .pollInterval(java.time.Duration.ofSeconds(1)) | ||
| .until( | ||
| () -> { | ||
| List<MetricData> metricData = new ArrayList<>(metricReader.collectAllMetrics()); | ||
| return metricData.size() >= NUM_GAX_OTEL_METRICS && areAllGaxMetricsRecorded(metricData); | ||
| }); | ||
| return new ArrayList<>(metricReader.collectAllMetrics()); | ||
| } catch (org.awaitility.core.ConditionTimeoutException e) { | ||
| Assertions.fail("Unable to collect all the GAX metrics required for the test", e); | ||
| return new ArrayList<>(); | ||
| } |
There was a problem hiding this comment.
Efficiency & Readability Improvement: Calling metricReader.collectAllMetrics() inside the until block and then calling it again immediately after the await block is redundant and inefficient.
We can use an AtomicReference to capture the matching list of metrics during the polling phase, avoiding the extra call to collectAllMetrics() after the loop completes.
| try { | |
| Awaitility.await() | |
| .atMost(java.time.Duration.ofSeconds(NUM_DEFAULT_FLUSH_ATTEMPTS)) | |
| .pollInterval(java.time.Duration.ofSeconds(1)) | |
| .until( | |
| () -> { | |
| List<MetricData> metricData = new ArrayList<>(metricReader.collectAllMetrics()); | |
| return metricData.size() >= NUM_GAX_OTEL_METRICS && areAllGaxMetricsRecorded(metricData); | |
| }); | |
| return new ArrayList<>(metricReader.collectAllMetrics()); | |
| } catch (org.awaitility.core.ConditionTimeoutException e) { | |
| Assertions.fail("Unable to collect all the GAX metrics required for the test", e); | |
| return new ArrayList<>(); | |
| } | |
| java.util.concurrent.atomic.AtomicReference<List<MetricData>> metricDataRef = new java.util.concurrent.atomic.AtomicReference<>(); | |
| try { | |
| Awaitility.await() | |
| .atMost(java.time.Duration.ofSeconds(NUM_DEFAULT_FLUSH_ATTEMPTS)) | |
| .pollInterval(java.time.Duration.ofSeconds(1)) | |
| .until( | |
| () -> { | |
| List<MetricData> metricData = new ArrayList<>(metricReader.collectAllMetrics()); | |
| if (metricData.size() >= NUM_GAX_OTEL_METRICS && areAllGaxMetricsRecorded(metricData)) { | |
| metricDataRef.set(metricData); | |
| return true; | |
| } | |
| return false; | |
| }); | |
| return metricDataRef.get(); | |
| } catch (org.awaitility.core.ConditionTimeoutException e) { | |
| Assertions.fail("Unable to collect all the GAX metrics required for the test", e); | |
| return new ArrayList<>(); | |
| } |
blakeli0
force-pushed
the
migrate-showcase-sleep-to-awaitility
branch
from
2317594 to
b972782
Compare
1 participant
Migrates all remaining usages of Thread.sleep to Awaitility in the java-showcase module, improving test reliability and reducing unnecessary busy wait delays.