gemmellr commented on code in PR #4710:
URL: https://github.com/apache/activemq-artemis/pull/4710#discussion_r1469480482


##########
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/mqtt5/MQTT5Test.java:
##########
@@ -83,6 +83,29 @@ public void messageArrived(String topic, MqttMessage 
message) {
       assertTrue(latch.await(500, TimeUnit.MILLISECONDS));
    }
 
+   @Test(timeout = DEFAULT_TIMEOUT)
+   public void testTopicNameEscape() throws Exception {
+      final String topic = "foo1.0/bar/baz";
+
+      CountDownLatch latch = new CountDownLatch(1);
+      MqttClient subscriber = createPahoClient("subscriber");
+      subscriber.connect();
+      subscriber.setCallback(new DefaultMqttCallback() {
+         @Override
+         public void messageArrived(String t, MqttMessage message) {
+            logger.info("Message received from topic {}, message={}", topic, 
message);
+            assertEquals(topic, t);

Review Comment:
   Would be better to remember the value here and then assert outside the 
callback after the latch trips. That way the test will fail quicker and in a 
more helpful manner if the value isnt as expected, having the assertion 
directly cause it rather than e.g just timing out after 500ms on the latch 
(because it wasnt tripped) and requiring someone go look at logs and hope that 
there is a stack logged that might very obviously explain it (or then figuring 
out from the info log if there isnt). It would also just be clearer what is 
really being tested, took me a bit to even notice what the test really cared 
about while doing the send+receive.



-- 
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]

Reply via email to