jbonofre commented on code in PR #1852:
URL: https://github.com/apache/activemq/pull/1852#discussion_r3005726861
##########
activemq-unit-tests/src/test/java/org/apache/activemq/broker/policy/MessageInterceptorStrategyTest.java:
##########
@@ -207,7 +208,16 @@ public void testForceExpirationZeroOverrideDLQ() throws
Exception {
Message sendMessageP =
session.createTextMessage("expiration=zero-no-dlq-expiry");
producer.send(queue, sendMessageP);
- Thread.sleep(250l);
+ boolean reachedDlq = Wait.waitFor(new Wait.Condition() {
+ @Override
+ public boolean isSatisified() throws Exception {
+ boolean originalQueueEmpty =
!session.createBrowser(queue).getEnumeration().hasMoreElements();
+ QueueBrowser dlqQueueBrowser =
session.createBrowser(createQueue("mis.forceExpiration.zero-no-dlq-expiry.dlq"));
+ boolean dlqHasMessage =
dlqQueueBrowser.getEnumeration().hasMoreElements();
+ return originalQueueEmpty && dlqHasMessage;
+ }
+ });
+ assertTrue("Message should be routed to DLQ", reachedDlq);
queueBrowser = session.createBrowser(queue);
Review Comment:
Nit: you are doing the verification in the `Wait.watiFor()` and you are
doing the verification again here. It's useless: if you are out of the
iteration, you have the condition satisfied.
##########
activemq-unit-tests/src/test/java/org/apache/activemq/broker/policy/MessageInterceptorStrategyTest.java:
##########
@@ -207,7 +208,16 @@ public void testForceExpirationZeroOverrideDLQ() throws
Exception {
Message sendMessageP =
session.createTextMessage("expiration=zero-no-dlq-expiry");
producer.send(queue, sendMessageP);
- Thread.sleep(250l);
+ boolean reachedDlq = Wait.waitFor(new Wait.Condition() {
+ @Override
+ public boolean isSatisified() throws Exception {
+ boolean originalQueueEmpty =
!session.createBrowser(queue).getEnumeration().hasMoreElements();
+ QueueBrowser dlqQueueBrowser =
session.createBrowser(createQueue("mis.forceExpiration.zero-no-dlq-expiry.dlq"));
Review Comment:
You create a browser here, but you never close it. With a default wait of 30
seconds with 1-second polling, it means it could create 60 unclosed
`QueueBrowser` instances.
You should use try-with-resources or explicity `close()` in the iteration.
That's not a huge blocker here because we are in a test, but worth to
address.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information, visit: https://activemq.apache.org/contact
