[
https://issues.apache.org/jira/browse/CAMEL-19542?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18093176#comment-18093176
]
Torsten Mielke commented on CAMEL-19542:
----------------------------------------
There are 13 unit test classes in {{camel-sjms}} that make use of
{{Thread.sleep()}} calls.
Here is the analysis:
h4.
{{camel-sjms/src/test/java/org/apache/camel/component/sjms/producer/InOutQueueProducerAsyncLoadTest.java}}
replace with Awaitility
h4.
{{camel-sjms/src/test/java/org/apache/camel/component/sjms/support/JmsExclusiveTestSupport.java}}
replace with Awaitility
h4.
{{camel-sjms/src/test/java/org/apache/camel/component/sjms/consumer/JmsPollingConsumerTest.java}}
Replace {{Thread.sleep(500)}} with a {{CountDownLatch}} and a potential short
sleep of 100 msec thereafter.
The problem, there's no way to signal "I am now blocking and waiting for a
message" because {{consumer.receiveBody()}} is itself the blocking call. We
can't put a signal inside it.
Even with the fix there's still a small race window between {{countDown()}} and
the blocking {{receiveBody()}} call. The additional 100ms sleep minimises this,
but doesn't eliminate it entirely. This is an inherent limitation when
coordinating around a blocking call we don't control. This approach is better
than the original 500ms blind sleep while accepting this small remaining race
condition.
h4.
{{camel-sjms/src/test/java/org/apache/camel/component/sjms/support/MyAsyncProducer.java}}
legitimate use of {{Thread.sleep()}} to simulate processing time
h4.
{{camel-sjms/src/test/java/org/apache/camel/component/sjms/consumer/InOnlyTopicDurableConsumerTest.java}}
The {{Thread.sleep()}} call is unnecessary here.
In the Camel test framework, the {{CamelContext}} is created and started before
the @Test method runs. This is part of the {{@BeforeEach}} lifecycle. The
routes are added during context creation and started when the context starts.
1. {{@BeforeEach}} runs (sets up context)
2. Context is created via {{createCamelContext()}}
3. Routes are added via {{createRouteBuilder()}}
4. Context is started (routes and consumers start)
5. Then {{@Test }}method runs
So by the time {{testDurableTopic()}} executes, both durable topic consumers
should already be fully started and subscribed.
The {{Thread.sleep(1000)}} call is unnecessary!
h4.
{{camel-sjms/src/test/java/org/apache/camel/component/sjms/consumer/InOnlyConsumerAsyncFalseTest.java}}
The intention of this test is not very clear, it needs to be changed slightly.
The {{Thread.sleep()}} call can be avoided by using
{{MockEndpoint.assertIsSatisfied(context, 3, TimeUnit.SECONDS);}}
It seems the goal of the test is to verify that messages are processed {*}in
the same order in which they are sent{*}, when not using {{asyncConumser=true}}
and having the {{Thread.sleep()}} call on the first message.
The test class name contain "AsyncFalse", so it explicitly wants to disable
{{asyncConsumer}} (which it does). The other test
{{{}AsyncConsumerInOutTest{}}}, also checks that messages are received in
reverse order but explicitly enables {{{}asyncConsumer=true{}}}.
So likey the note in this test that says
{{// Hello World is received first despite its send last}}
is wrong (seems a copy paste from other tests)
The current test logic does expect messages to be received in the same order as
they are sent as per
{{getMockEndpoint(MOCK_RESULT).expectedBodiesReceived("Hello Camel", "Hello
World");}}
So I propose to change the test so
* it verifies that messages are received in same order, as
{{asyncConsumer=false}} is in effect
* Remove the comment
{{// Hello World is received first despite its send last}}
* rename test method from {{testInOnlyConsumerAsyncTrue}} to
{{testInOnlyConsumerAsyncFalse}}
* keep the check for the thread names
This was discussed with Otavio Piske.
The following unit test use {{Thread.sleep()}} to simulate processing time of
the message. This is a simple and legitimate use.
The tests could be modified to use a delayer EIP (together with a content based
router in some cases) to avoid the use of {{Thread.sleep()}} but that would not
gain any real benefits and complicates the routes under test. I discussed with
Otavio Piske and we agreed to leave these tests as is
*
{{camel-sjms/src/test/java/org/apache/camel/component/sjms/consumer/InOutConsumerQueueTest.java}}
*
{{camel-sjms/src/test/java/org/apache/camel/component/sjms/consumer/InOnlyConsumerTopicTest.java}}
*
{{camel-sjms/src/test/java/org/apache/camel/component/sjms/consumer/InOutConsumerTempQueueAsyncTest.java}}
*
{{camel-sjms/src/test/java/org/apache/camel/component/sjms/consumer/InOutConcurrentConsumerTest.java}}
*
{{camel-sjms/src/test/java/org/apache/camel/component/sjms/consumer/InOutConsumerTopicTest.java}}
*
{{camel-sjms/src/test/java/org/apache/camel/component/sjms/consumer/InOutConsumerQueueAsyncTest.java}}
*
{{camel-sjms/src/test/java/org/apache/camel/component/sjms/consumer/InOutConsumerTempQueueTest.java}}
> camel-sjms: replace Thread.sleep in tests
> -----------------------------------------
>
> Key: CAMEL-19542
> URL: https://issues.apache.org/jira/browse/CAMEL-19542
> Project: Camel
> Issue Type: Task
> Components: camel-sjms, tests
> Affects Versions: 4.0.0
> Reporter: Otavio Rodolfo Piske
> Assignee: Torsten Mielke
> Priority: Minor
> Labels: easy, help-wanted
>
> We have many tests which use Thread.sleep for synchronization. This is bug
> prone and can introduce flakiness when running on environments with different
> capacities.
> Ideally we should replace these with:
> * [Awaitility|http://www.awaitility.org/]
> * Java's native syncronization mechanism (Latches, Phasers, Locks, etc)
> * Nothing (i.e.; in some cases the sleep can simply be removed)
>
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
