Skip to content

Commit 7ebd072

Browse files
committed
Fix array comparison bug in OpAMP client test
The whenServerProvidesNewInstanceUid_useIt test was using reference comparison (!=) instead of content comparison (Arrays.equals) for byte arrays. This caused the test to be flaky and sometimes fail in CI environments. Changes: - Fixed array comparison from '!=' to '!Arrays.equals()' - Changed server-provided UID from {1,2,3} to {4,5,6} to ensure it differs from initial UID - Cleaned up debug logging added during investigation Fixes the flaky test behavior reported in CI runs.
1 parent d071dd3 commit 7ebd072

File tree

1 file changed

+21
-117
lines changed

1 file changed

+21
-117
lines changed

opamp-client/src/test/java/io/opentelemetry/opamp/client/internal/impl/OpampClientImplTest.java

Lines changed: 21 additions & 117 deletions
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@
6060
import org.mockito.junit.jupiter.MockitoExtension;
6161

6262
@ExtendWith(MockitoExtension.class)
63-
@SuppressWarnings({"SystemOut", "CatchingUnchecked", "InterruptedExceptionSwallowed"})
63+
@SuppressWarnings({"CatchAndPrintStackTrace", "SystemOut", "CatchingUnchecked", "InterruptedExceptionSwallowed"})
6464
class OpampClientImplTest {
6565
private RequestService requestService;
6666
private OpampClientState state;
@@ -71,8 +71,6 @@ class OpampClientImplTest {
7171

7272
@BeforeEach
7373
void setUp() {
74-
System.out.println("[DEBUG] Test setUp - Thread: " + Thread.currentThread().getName() +
75-
", Time: " + System.currentTimeMillis());
7674
effectiveConfig =
7775
new TestEffectiveConfig(
7876
new EffectiveConfig.Builder()
@@ -89,33 +87,26 @@ void setUp() {
8987
new State.Flags((long) AgentToServerFlags.AgentToServerFlags_Unspecified.getValue()),
9088
effectiveConfig);
9189
requestService = createHttpService();
92-
System.out.println("[DEBUG] Test setUp complete - Server URL: " + server.url("/v1/opamp"));
9390
}
9491

9592
@AfterEach
9693
void tearDown() {
97-
System.out.println("[DEBUG] Test tearDown - Thread: " + Thread.currentThread().getName() +
98-
", Time: " + System.currentTimeMillis());
9994
if (client != null) {
10095
try {
10196
client.stop();
102-
System.out.println("[DEBUG] Client stopped successfully");
10397
} catch (Exception e) {
104-
System.out.println("[DEBUG] Error stopping client: " + e.getMessage());
10598
e.printStackTrace();
10699
}
107100
}
108101

109102
// Clear any remaining server requests
110103
try {
111104
while (server.takeRequest(100, TimeUnit.MILLISECONDS) != null) {
112-
System.out.println("[DEBUG] Cleared remaining request from server queue");
105+
// Clear queue
113106
}
114107
} catch (InterruptedException e) {
115108
Thread.currentThread().interrupt();
116109
}
117-
118-
System.out.println("[DEBUG] Test tearDown complete");
119110
}
120111

121112
@Test
@@ -222,7 +213,6 @@ void verifyStartOnlyOnce() {
222213

223214
@Test
224215
void onSuccess_withChangesToReport_notifyCallbackOnMessage() {
225-
System.out.println("[DEBUG] Starting onSuccess_withChangesToReport_notifyCallbackOnMessage test");
226216
initializeClient();
227217
AgentRemoteConfig remoteConfig =
228218
new AgentRemoteConfig.Builder()
@@ -231,51 +221,30 @@ void onSuccess_withChangesToReport_notifyCallbackOnMessage() {
231221
ServerToAgent serverToAgent = new ServerToAgent.Builder().remote_config(remoteConfig).build();
232222
enqueueServerToAgentResponse(serverToAgent);
233223

234-
System.out.println("[DEBUG] Before sendRequest - onMessageCalls: " + callbacks.onMessageCalls.get());
235224
// Force request
236225
requestService.sendRequest();
237226

238-
System.out.println("[DEBUG] After sendRequest - Starting await for callback");
239-
// Await for onMessage call - increased timeout for debugging
227+
// Await for onMessage call
240228
await().atMost(Duration.ofSeconds(10)).pollInterval(Duration.ofMillis(100))
241-
.until(() -> {
242-
int calls = callbacks.onMessageCalls.get();
243-
System.out.println("[DEBUG] Current onMessageCalls: " + calls +
244-
", Thread: " + Thread.currentThread().getName() +
245-
", Time: " + System.currentTimeMillis());
246-
return calls == 1;
247-
});
229+
.until(() -> callbacks.onMessageCalls.get() == 1);
248230

249-
System.out.println("[DEBUG] Await completed - verifying callback");
250231
verify(callbacks).onMessage(MessageData.builder().setRemoteConfig(remoteConfig).build());
251-
System.out.println("[DEBUG] Test completed successfully");
252232
}
253233

254234
@Test
255235
void onSuccess_withNoChangesToReport_doNotNotifyCallbackOnMessage() {
256-
System.out.println("[DEBUG] Starting onSuccess_withNoChangesToReport_doNotNotifyCallbackOnMessage test");
257236
initializeClient();
258237
ServerToAgent serverToAgent = new ServerToAgent.Builder().build();
259238
enqueueServerToAgentResponse(serverToAgent);
260239

261-
System.out.println("[DEBUG] Before sendRequest - onMessageCalls: " + callbacks.onMessageCalls.get());
262240
// Force request
263241
requestService.sendRequest();
264242

265-
System.out.println("[DEBUG] After sendRequest - Starting await during period");
266-
// Giving some time for the callback to get called - increased timeout for debugging
243+
// Giving some time for the callback to get called
267244
await().during(Duration.ofSeconds(3)).pollInterval(Duration.ofMillis(100))
268-
.untilAsserted(() -> {
269-
int calls = callbacks.onMessageCalls.get();
270-
System.out.println("[DEBUG] Current onMessageCalls (should stay 0): " + calls +
271-
", Thread: " + Thread.currentThread().getName() +
272-
", Time: " + System.currentTimeMillis());
273-
assertThat(calls).isEqualTo(0);
274-
});
245+
.untilAsserted(() -> assertThat(callbacks.onMessageCalls.get()).isEqualTo(0));
275246

276-
System.out.println("[DEBUG] Await completed - verifying no callback was called");
277247
verify(callbacks, never()).onMessage(any());
278-
System.out.println("[DEBUG] Test completed successfully");
279248
}
280249

281250
@Test
@@ -314,23 +283,13 @@ void verifyRemoteConfigStatusSetter() {
314283

315284
@Test
316285
void onConnectionSuccessful_notifyCallback() {
317-
System.out.println("[DEBUG] Starting onConnectionSuccessful_notifyCallback test");
318286
initializeClient();
319287

320-
System.out.println("[DEBUG] Client initialized - Starting await for connect callback");
321288
await().atMost(Duration.ofSeconds(10)).pollInterval(Duration.ofMillis(100))
322-
.until(() -> {
323-
int calls = callbacks.onConnectCalls.get();
324-
System.out.println("[DEBUG] Current onConnectCalls: " + calls +
325-
", Thread: " + Thread.currentThread().getName() +
326-
", Time: " + System.currentTimeMillis());
327-
return calls == 1;
328-
});
289+
.until(() -> callbacks.onConnectCalls.get() == 1);
329290

330-
System.out.println("[DEBUG] Await completed - verifying callbacks");
331291
verify(callbacks).onConnect();
332292
verify(callbacks, never()).onConnectFailed(any());
333-
System.out.println("[DEBUG] Test completed successfully");
334293
}
335294

336295
@Test
@@ -363,30 +322,19 @@ void onFailedResponse_keepFieldsForNextRequest() {
363322

364323
@Test
365324
void onFailedResponse_withServerErrorData_notifyCallback() {
366-
System.out.println("[DEBUG] Starting onFailedResponse_withServerErrorData_notifyCallback test");
367325
initializeClient();
368326

369327
ServerErrorResponse errorResponse = new ServerErrorResponse.Builder().build();
370328
enqueueServerToAgentResponse(new ServerToAgent.Builder().error_response(errorResponse).build());
371329

372-
System.out.println("[DEBUG] Before sendRequest - onErrorResponseCalls: " + callbacks.onErrorResponseCalls.get());
373330
// Force request
374331
requestService.sendRequest();
375332

376-
System.out.println("[DEBUG] After sendRequest - Starting await for error callback");
377333
await().atMost(Duration.ofSeconds(10)).pollInterval(Duration.ofMillis(100))
378-
.until(() -> {
379-
int calls = callbacks.onErrorResponseCalls.get();
380-
System.out.println("[DEBUG] Current onErrorResponseCalls: " + calls +
381-
", Thread: " + Thread.currentThread().getName() +
382-
", Time: " + System.currentTimeMillis());
383-
return calls == 1;
384-
});
334+
.until(() -> callbacks.onErrorResponseCalls.get() == 1);
385335

386-
System.out.println("[DEBUG] Await completed - verifying callbacks");
387336
verify(callbacks).onErrorResponse(errorResponse);
388337
verify(callbacks, never()).onMessage(any());
389-
System.out.println("[DEBUG] Test completed successfully");
390338
}
391339

392340
@Test
@@ -401,12 +349,10 @@ void onConnectionFailed_notifyCallback() {
401349

402350
@Test
403351
void whenServerProvidesNewInstanceUid_useIt() {
404-
System.out.println("[DEBUG] Starting whenServerProvidesNewInstanceUid_useIt test");
405352
initializeClient();
406353
byte[] initialUid = state.instanceUid.get();
407-
System.out.println("[DEBUG] Initial UID: " + java.util.Arrays.toString(initialUid));
408354

409-
byte[] serverProvidedUid = new byte[] {1, 2, 3};
355+
byte[] serverProvidedUid = new byte[] {4, 5, 6};
410356
ServerToAgent response =
411357
new ServerToAgent.Builder()
412358
.agent_identification(
@@ -416,34 +362,20 @@ void whenServerProvidesNewInstanceUid_useIt() {
416362
.build();
417363

418364
enqueueServerToAgentResponse(response);
419-
System.out.println("[DEBUG] Before sendRequest - current UID: " +
420-
java.util.Arrays.toString(state.instanceUid.get()));
421365
requestService.sendRequest();
422366

423-
System.out.println("[DEBUG] After sendRequest - Starting await for UID change");
424367
await().atMost(Duration.ofSeconds(10)).pollInterval(Duration.ofMillis(100))
425368
.until(() -> {
426369
byte[] currentUid = state.instanceUid.get();
427-
boolean changed = currentUid != initialUid;
428-
System.out.println("[DEBUG] Current UID: " + java.util.Arrays.toString(currentUid) +
429-
", Changed: " + changed +
430-
", Thread: " + Thread.currentThread().getName() +
431-
", Time: " + System.currentTimeMillis());
432-
return changed;
370+
return !java.util.Arrays.equals(currentUid, initialUid);
433371
});
434372

435-
System.out.println("[DEBUG] Await completed - verifying UID change");
436373
assertThat(state.instanceUid.get()).isEqualTo(serverProvidedUid);
437-
System.out.println("[DEBUG] Test completed successfully - Final UID: " +
438-
java.util.Arrays.toString(state.instanceUid.get()));
439374
}
440375

441376
@Test
442377
void flakiness_stress_test_all_timing_operations() {
443-
System.out.println("[DEBUG] Starting stress test for flakiness detection");
444378
for (int i = 1; i <= 10; i++) {
445-
System.out.println("[DEBUG] ===== ITERATION " + i + " =====");
446-
447379
try {
448380
// Test connection callback timing
449381
initializeClient();
@@ -481,20 +413,16 @@ void flakiness_stress_test_all_timing_operations() {
481413
await().atMost(Duration.ofSeconds(5)).pollInterval(Duration.ofMillis(50))
482414
.until(() -> !java.util.Arrays.equals(state.instanceUid.get(), beforeUid));
483415

484-
System.out.println("[DEBUG] Iteration " + i + " completed successfully");
485-
486-
// Force cleanup between iterations
487-
client.stop();
488-
Thread.sleep(100); // Small delay to ensure cleanup
416+
// Force cleanup between iterations - ensure client is stopped before next iteration
417+
if (client != null) {
418+
client.stop();
419+
}
420+
Thread.sleep(50); // Small delay to ensure cleanup
489421

490422
} catch (Exception e) {
491-
System.out.println("[ERROR] Iteration " + i + " failed: " + e.getMessage());
492-
e.printStackTrace();
493423
throw new RuntimeException("Stress test failed at iteration " + i, e);
494424
}
495425
}
496-
497-
System.out.println("[DEBUG] All stress test iterations completed successfully");
498426
}
499427

500428
private static AgentToServer getAgentToServerMessage(RecordedRequest request) {
@@ -507,13 +435,8 @@ private static AgentToServer getAgentToServerMessage(RecordedRequest request) {
507435

508436
private RecordedRequest takeRequest() {
509437
try {
510-
System.out.println("[DEBUG] Taking request from server - Thread: " + Thread.currentThread().getName());
511-
RecordedRequest request = server.takeRequest(5, TimeUnit.SECONDS); // Increased timeout
512-
System.out.println("[DEBUG] Request taken: " + (request != null ?
513-
"SUCCESS - Method: " + request.getMethod() : "TIMEOUT"));
514-
return request;
438+
return server.takeRequest(5, TimeUnit.SECONDS);
515439
} catch (InterruptedException e) {
516-
System.out.println("[DEBUG] takeRequest interrupted: " + e.getMessage());
517440
throw new RuntimeException(e);
518441
}
519442
}
@@ -556,19 +479,14 @@ private RecordedRequest initializeClient() {
556479
}
557480

558481
private RecordedRequest initializeClient(ServerToAgent initialResponse) {
559-
System.out.println("[DEBUG] Initializing client with response: " + initialResponse);
560482
client = OpampClientImpl.create(requestService, state);
561483

562484
// Prepare first request on start
563485
enqueueServerToAgentResponse(initialResponse);
564486

565487
callbacks = spy(new TestCallbacks());
566-
System.out.println("[DEBUG] Starting client - Thread: " + Thread.currentThread().getName());
567488
client.start(callbacks);
568-
System.out.println("[DEBUG] Client started - taking request from server");
569-
RecordedRequest request = takeRequest();
570-
System.out.println("[DEBUG] Request taken: " + (request != null ? "SUCCESS" : "NULL"));
571-
return request;
489+
return takeRequest();
572490
}
573491

574492
private static class TestEffectiveConfig extends State.EffectiveConfig {
@@ -622,36 +540,22 @@ private static class TestCallbacks implements OpampClient.Callbacks {
622540

623541
@Override
624542
public void onConnect() {
625-
int count = onConnectCalls.incrementAndGet();
626-
System.out.println("[DEBUG] TestCallbacks.onConnect() called - count: " + count +
627-
", Thread: " + Thread.currentThread().getName() +
628-
", Time: " + System.currentTimeMillis());
543+
onConnectCalls.incrementAndGet();
629544
}
630545

631546
@Override
632547
public void onConnectFailed(@Nullable Throwable throwable) {
633-
int count = onConnectFailedCalls.incrementAndGet();
634-
System.out.println("[DEBUG] TestCallbacks.onConnectFailed() called - count: " + count +
635-
", Exception: " + (throwable != null ? throwable.getMessage() : "null") +
636-
", Thread: " + Thread.currentThread().getName() +
637-
", Time: " + System.currentTimeMillis());
548+
onConnectFailedCalls.incrementAndGet();
638549
}
639550

640551
@Override
641552
public void onErrorResponse(ServerErrorResponse errorResponse) {
642-
int count = onErrorResponseCalls.incrementAndGet();
643-
System.out.println("[DEBUG] TestCallbacks.onErrorResponse() called - count: " + count +
644-
", Thread: " + Thread.currentThread().getName() +
645-
", Time: " + System.currentTimeMillis());
553+
onErrorResponseCalls.incrementAndGet();
646554
}
647555

648556
@Override
649557
public void onMessage(MessageData messageData) {
650-
int count = onMessageCalls.incrementAndGet();
651-
System.out.println("[DEBUG] TestCallbacks.onMessage() called - count: " + count +
652-
", Thread: " + Thread.currentThread().getName() +
653-
", Time: " + System.currentTimeMillis() +
654-
", MessageData: " + messageData);
558+
onMessageCalls.incrementAndGet();
655559
}
656560
}
657561
}

0 commit comments

Comments
 (0)