Copilot commented on code in PR #24376:
URL: https://github.com/apache/camel/pull/24376#discussion_r3524540097
##########
components/camel-sjms/src/test/java/org/apache/camel/component/sjms/producer/InOutQueueProducerAsyncLoadTest.java:
##########
@@ -87,19 +89,19 @@ public void run() {
assertNotNull(response);
assertEquals(responseText, response);
} catch (Exception e) {
- log.error("TODO Auto-generated catch block", e);
+ log.error("Failed to process message {}", tempI, e);
}
}
};
executor.execute(worker);
}
- while (context.getInflightRepository().size() > 0) {
- Thread.sleep(100);
- }
+ // wait for inflight messages to complete
+ await().atMost(30, SECONDS)
+ .untilAsserted(() -> assertEquals(0,
context.getInflightRepository().size()));
+
executor.shutdown();
Review Comment:
The inflight wait runs before the executor is shut down / drained, so it can
pass while no tasks have started yet (inflight size is 0 initially). That can
lead to shutting down the executor early and making the test flaky or silently
skipping work. Shut down + await executor termination first, then assert
inflight is 0.
##########
components/camel-sjms/src/test/java/org/apache/camel/component/sjms/consumer/JmsPollingConsumerTest.java:
##########
@@ -53,17 +61,23 @@ public void testJmsPollingConsumerNoWait() throws Exception
{
MockEndpoint mock = getMockEndpoint("mock:result");
mock.expectedBodiesReceived("Hello Claus");
- // use another thread for polling consumer to demonstrate that we can
wait before
- // the message is sent to the queue
+ // use another thread for polling consumer to demonstrate that we can
+ // wait before the message is sent to the queue
+ CountDownLatch consumerReady = new CountDownLatch(1);
+
CompletableFuture.runAsync(() -> {
Review Comment:
Avoid using Thread.sleep() for coordination in tests. In this case you can
deterministically wait until the no-wait poll has executed (and asserted null)
by using a second latch, eliminating the fixed delay.
##########
components/camel-sjms/src/test/java/org/apache/camel/component/sjms/consumer/JmsPollingConsumerTest.java:
##########
@@ -105,13 +125,19 @@ public void testJmsPollingConsumerHighTimeout() throws
Exception {
// use another thread for polling consumer to demonstrate that we can
wait before
// the message is sent to the queue
+ CountDownLatch consumerReady = new CountDownLatch(1);
+
CompletableFuture.runAsync(() -> {
+ consumerReady.countDown();
String body =
consumer.receiveBody("sjms:queue.start.JmsPollingConsumerTest", 3000,
String.class);
template.sendBody("sjms:queue.foo.JmsPollingConsumerTest", body +
" Claus");
});
- // wait a little to demonstrate we can start poll before we have a msg
on the queue
- Thread.sleep(500);
+ // wait for async thread to signal it's ready, then add small delay
+ // for blocking call consumer.receiveBody() to start
+ assertTrue(consumerReady.await(2, TimeUnit.SECONDS), "Consumer should
be ready");
+ // additional small delay to ensure receiveBody() has executed
+ Thread.sleep(100);
Review Comment:
This adds another fixed Thread.sleep() in a test. It isn’t required for
correctness (the message will remain in the queue until the consumer receives
it) and can make the test slower/flakier. Rely on the latch and remove the
sleep.
##########
components/camel-sjms/src/test/java/org/apache/camel/component/sjms/consumer/JmsPollingConsumerTest.java:
##########
@@ -34,14 +37,19 @@ public void testJmsPollingConsumerWait() throws Exception {
// use another thread for polling consumer to demonstrate that we can
wait before
// the message is sent to the queue
+ CountDownLatch consumerReady = new CountDownLatch(1);
CompletableFuture.runAsync(() -> {
+ consumerReady.countDown();
String body =
consumer.receiveBody("sjms:queue.start.JmsPollingConsumerTest", String.class);
template.sendBody("sjms:queue.foo.JmsPollingConsumerTest", body +
" Claus");
});
- // wait a little to demonstrate we can start poll before we have a msg
on the queue
- Thread.sleep(500);
+ // wait for async thread to signal it's ready, then add small delay
+ // for blocking call consumer.receiveBody() to start
+ assertTrue(consumerReady.await(2, TimeUnit.SECONDS), "Consumer should
be ready");
+ // additional small delay to ensure receiveBody() has started blocking
+ Thread.sleep(100);
Review Comment:
This reintroduces a fixed Thread.sleep() in a test. It’s unnecessary for
correctness here (the queue will retain the message until the polling consumer
starts) and adds nondeterministic delay/flakiness. Prefer removing the sleep
and rely on the existing latch.
##########
components/camel-sjms/src/test/java/org/apache/camel/component/sjms/consumer/JmsPollingConsumerTest.java:
##########
@@ -80,15 +94,21 @@ public void testJmsPollingConsumerLowTimeout() throws
Exception {
// use another thread for polling consumer to demonstrate that we can
wait before
// the message is sent to the queue
+ CountDownLatch consumerReady = new CountDownLatch(1);
+
CompletableFuture.runAsync(() -> {
+ consumerReady.countDown();
String body =
consumer.receiveBody("sjms:queue.start.JmsPollingConsumerTest", 100,
String.class);
assertNull(body, "Should be null");
template.sendBody("sjms:queue.foo.JmsPollingConsumerTest", "Hello
Claus");
});
- // wait a little to demonstrate we can start poll before we have a msg
on the queue
- Thread.sleep(500);
+ // wait for async thread to signal it's ready, then add small delay
+ // for blocking call consumer.receiveBody() to start
+ assertTrue(consumerReady.await(2, TimeUnit.SECONDS), "Consumer should
be ready");
+ // additional small delay to ensure receiveBody() has executed
+ Thread.sleep(200);
Review Comment:
This timed-receive test still uses a fixed Thread.sleep() to try to ensure
the 100ms receive times out before sending the message, which is
timing-dependent and can be flaky. Use a latch to wait until the timed receive
has completed (returned null) before sending to direct:start.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
