Skip to content

refactor(showcase): migrate Thread.sleep to Awaitility in integration tests #13519

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.

Sign up for GitHub

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

Open
blakeli0 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

refactor(showcase): migrate Thread.sleep to Awaitility in integration tests #13519

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions java-showcase/gapic-showcase/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,12 @@
<version>${slf4j2-logback.version}</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.awaitility</groupId>
<artifactId>awaitility</artifactId>
<version>4.3.0</version>
<scope>test</scope>
</dependency>

</dependencies>
<profiles>
Expand Down
11 changes: 5 additions & 6 deletions ...howcase/gapic-showcase/src/test/java/com/google/showcase/v1beta1/it/ITClientShutdown.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import com.google.showcase.v1beta1.EchoClient;
import com.google.showcase.v1beta1.EchoRequest;
import com.google.showcase.v1beta1.it.util.TestClientInitializer;
import org.awaitility.Awaitility;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.Timeout;
import org.threeten.bp.Duration;
Expand Down Expand Up @@ -143,12 +144,10 @@ private void assertClientTerminated(EchoClient echoClient) throws InterruptedExc
// check that everything is properly terminated after close() is called.
echoClient.close();

// Loop until the client has terminated successfully. For tests that use this,
// try to ensure there is a timeout associated, otherwise this may run forever.
// Future enhancement: Use awaitility instead of busy waiting
while (!echoClient.isTerminated()) {
Thread.sleep(500L);
}
Awaitility.await()
.atMost(java.time.Duration.ofMillis(DEFAULT_CLIENT_TERMINATION_MS))
.pollInterval(java.time.Duration.ofMillis(500))
.until(echoClient::isTerminated);
// The busy-wait time won't be accurate, so account for a bit of buffer
long end = System.currentTimeMillis();

Expand Down
11 changes: 6 additions & 5 deletions ...owcase/gapic-showcase/src/test/java/com/google/showcase/v1beta1/it/ITCompositeTracer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,11 @@
import io.opentelemetry.sdk.trace.SdkTracerProvider;
import io.opentelemetry.sdk.trace.data.SpanData;
import io.opentelemetry.sdk.trace.export.SimpleSpanProcessor;
import java.time.Duration;
import java.util.Arrays;
import java.util.Collection;
import java.util.List;
import org.awaitility.Awaitility;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
Expand Down Expand Up @@ -124,13 +126,12 @@ void testCompositeTracer() throws Exception {
.get(AttributeKey.stringKey(ObservabilityAttributes.SERVER_ADDRESS_ATTRIBUTE)))
.isEqualTo(SHOWCASE_SERVER_ADDRESS);

Thread.sleep(100);
// Verify metric name and one basic attribute server.address
Awaitility.await()
.atMost(Duration.ofSeconds(10))
.pollInterval(Duration.ofMillis(100))
.until(() -> !metricReader.collectAllMetrics().isEmpty());
Comment on lines +130 to +133

@gemini-code-assist gemini-code-assist Bot Jun 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

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()));

Collection<MetricData> actualMetrics = metricReader.collectAllMetrics();
for (int i = 0; i < 10 && actualMetrics.isEmpty(); i++) {
Thread.sleep(1000L);
actualMetrics = metricReader.collectAllMetrics();
}

assertThat(actualMetrics).isNotEmpty();
MetricData metricData =
Expand Down
41 changes: 33 additions & 8 deletions ...case/gapic-showcase/src/test/java/com/google/showcase/v1beta1/it/ITOtelGoldenMetrics.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@
import java.io.InputStream;
import java.time.Duration;
import java.util.Collection;
import org.awaitility.Awaitility;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
Expand Down Expand Up @@ -106,7 +107,10 @@ void testMetrics_successfulEcho_grpc() throws Exception {
// This is implemented by adding a TraceFinisher to ApiFuture as a callback in
// TracedUnaryCallable,
// which could be executed in a different thread.
Thread.sleep(100);
Awaitility.await()
.atMost(Duration.ofSeconds(5))
.pollInterval(Duration.ofMillis(10))
.until(() -> !metricReader.collectAllMetrics().isEmpty());
Comment on lines +110 to +113

@gemini-code-assist gemini-code-assist Bot Jun 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

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.

Collection<MetricData> metrics = metricReader.collectAllMetrics();
assertThat(metrics).isNotEmpty();

Expand Down Expand Up @@ -192,7 +196,10 @@ public void sendMessage(ReqT message) {}
UnavailableException.class,
() -> client.echo(EchoRequest.newBuilder().setContent("metrics-test").build()));

Thread.sleep(100);
Awaitility.await()
.atMost(Duration.ofSeconds(5))
.pollInterval(Duration.ofMillis(10))
.until(() -> !metricReader.collectAllMetrics().isEmpty());
Collection<MetricData> metrics = metricReader.collectAllMetrics();
assertThat(metrics).isNotEmpty();

Expand Down Expand Up @@ -224,7 +231,10 @@ void testMetrics_successfulEcho_httpjson() throws Exception {

client.echo(EchoRequest.newBuilder().setContent("metrics-test").build());

Thread.sleep(100);
Awaitility.await()
.atMost(Duration.ofSeconds(5))
.pollInterval(Duration.ofMillis(10))
.until(() -> !metricReader.collectAllMetrics().isEmpty());
Collection<MetricData> metrics = metricReader.collectAllMetrics();
assertThat(metrics).isNotEmpty();

Expand Down Expand Up @@ -366,7 +376,10 @@ public String getHeaderValue(int index) {
UnavailableException.class,
() -> client.echo(EchoRequest.newBuilder().setContent("metrics-test").build()));

Thread.sleep(100);
Awaitility.await()
.atMost(Duration.ofSeconds(5))
.pollInterval(Duration.ofMillis(10))
.until(() -> !metricReader.collectAllMetrics().isEmpty());
Collection<MetricData> metrics = metricReader.collectAllMetrics();
assertThat(metrics).isNotEmpty();

Expand Down Expand Up @@ -415,7 +428,10 @@ void testMetrics_clientTimeout_grpc() throws Exception {
Exception.class,
() -> client.echo(EchoRequest.newBuilder().setContent("metrics-test").build()));

Thread.sleep(100);
Awaitility.await()
.atMost(Duration.ofSeconds(5))
.pollInterval(Duration.ofMillis(10))
.until(() -> !metricReader.collectAllMetrics().isEmpty());
Collection<MetricData> metrics = metricReader.collectAllMetrics();
assertThat(metrics).isNotEmpty();

Expand Down Expand Up @@ -458,7 +474,10 @@ void testMetrics_clientTimeout_httpjson() throws Exception {
Exception.class,
() -> client.echo(EchoRequest.newBuilder().setContent("metrics-test").build()));

Thread.sleep(100);
Awaitility.await()
.atMost(Duration.ofSeconds(5))
.pollInterval(Duration.ofMillis(10))
.until(() -> !metricReader.collectAllMetrics().isEmpty());
Collection<MetricData> metrics = metricReader.collectAllMetrics();
assertThat(metrics).isNotEmpty();

Expand Down Expand Up @@ -538,7 +557,10 @@ public void sendMessage(ReqT message) {}

assertThat(attemptCount.get()).isEqualTo(3);

Thread.sleep(100);
Awaitility.await()
.atMost(Duration.ofSeconds(5))
.pollInterval(Duration.ofMillis(10))
.until(() -> !metricReader.collectAllMetrics().isEmpty());
Collection<MetricData> metrics = metricReader.collectAllMetrics();
assertThat(metrics).hasSize(1);

Expand Down Expand Up @@ -709,7 +731,10 @@ public String getHeaderValue(int index) {

assertThat(requestCount.get()).isEqualTo(3);

Thread.sleep(100);
Awaitility.await()
.atMost(Duration.ofSeconds(5))
.pollInterval(Duration.ofMillis(10))
.until(() -> !metricReader.collectAllMetrics().isEmpty());
Collection<MetricData> metrics = metricReader.collectAllMetrics();
assertThat(metrics).hasSize(1);

Expand Down
30 changes: 17 additions & 13 deletions java-showcase/gapic-showcase/src/test/java/com/google/showcase/v1beta1/it/ITOtelMetrics.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@
import java.util.concurrent.TimeUnit;
import java.util.function.Predicate;
import java.util.stream.Collectors;
import org.awaitility.Awaitility;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeEach;
Expand Down Expand Up @@ -296,18 +297,21 @@ private List<MetricData> getMetricDataList() throws InterruptedException {
*/
private List<MetricData> getMetricDataList(InMemoryMetricReader metricReader)
throws InterruptedException {
for (int i = 0; i < NUM_DEFAULT_FLUSH_ATTEMPTS; i++) {
Thread.sleep(1000L);
List<MetricData> metricData = new ArrayList<>(metricReader.collectAllMetrics());
// Depending on the OpenTelemetry instance (i.e. OpenTelemetry, GrpcOpenTelemetry, etc.)
// there may be additional metrics recorded. Only check to ensure the Gax Metrics
// are recorded properly. Any additional metrics are fine to be passed.
if (metricData.size() >= NUM_GAX_OTEL_METRICS && areAllGaxMetricsRecorded(metricData)) {
return metricData;
}
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<>();
}
Comment on lines +300 to 314

@gemini-code-assist gemini-code-assist Bot Jun 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

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.

Suggested change
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<>();
}

Assertions.fail("Unable to collect all the GAX metrics required for the test");
return new ArrayList<>();
}

private boolean areAllGaxMetricsRecorded(List<MetricData> metricData) {
Expand Down Expand Up @@ -370,7 +374,7 @@ void testGrpc_operationCancelled_recordsMetrics() throws Exception {
UnaryCallable<BlockRequest, BlockResponse> blockCallable = grpcClient.blockCallable();
ApiFuture<BlockResponse> blockResponseApiFuture = blockCallable.futureCall(blockRequest);
// Sleep 1s before cancelling to let the request go through
Thread.sleep(1000);
Awaitility.await().pollDelay(java.time.Duration.ofSeconds(1)).until(() -> true);
blockResponseApiFuture.cancel(true);

List<MetricData> actualMetricDataList = getMetricDataList();
Expand All @@ -397,7 +401,7 @@ void testHttpJson_operationCancelled_recordsMetrics() throws Exception {
UnaryCallable<BlockRequest, BlockResponse> blockCallable = httpClient.blockCallable();
ApiFuture<BlockResponse> blockResponseApiFuture = blockCallable.futureCall(blockRequest);
// Sleep 1s before cancelling to let the request go through
Thread.sleep(1000);
Awaitility.await().pollDelay(java.time.Duration.ofSeconds(1)).until(() -> true);
blockResponseApiFuture.cancel(true);

List<MetricData> actualMetricDataList = getMetricDataList();
Expand Down
Loading