[GitHub] [activemq-artemis] gemmellr commented on a diff in pull request #4302: ARTEMIS-4106 do not set prop w/empty key when converting to OpenWire

[GitBox <gi...@apache.org>]

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


##########
artemis-protocols/artemis-openwire-protocol/src/test/java/org/apache/activemq/artemis/core/protocol/openwire/OpenWireMessageConverterTest.java:
##########
@@ -248,6 +248,7 @@ public void testBadPropertyConversion() throws Exception {
       coreMessage.putStringProperty(hdrBrokerInTime, "5678");
       coreMessage.putStringProperty(hdrCommandId, "foo");
       coreMessage.putStringProperty(hdrDroppable, "true");
+      coreMessage.putStringProperty("", "");

Review Comment:
   I'd agree either the test or the impl should be doing something else than it currently is.
   
   The test doesnt look to do anything to check what happens to this empty prop, so it seems like it would just behave the same if the impl behaviour went back to what it was, i.e it doesnt really test the fixed issue/behaviour.
   
   Perhaps the test could do something like verify the count of expected props to ensure no unexpected props are there beyond those its checking values for. Or as Dom suggests the prop setter impl could also throw (the test could still check there isnt anything unexpected after conversion). Though that will obviously break whatever situation allowed the position there was to be an empty prop key in the Core message to begin with.



-- 
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: gitbox-unsubscribe@activemq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org