[GitHub] [qpid-proton] gemmellr commented on pull request #346: PROTON-2308: Add support for setting Dynamic Node Properties

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

gemmellr commented on pull request #346:
URL: https://github.com/apache/qpid-proton/pull/346#issuecomment-999480504


   > Although @gemmellr is correct in his analysis of what is going on in a real AMQP messaging interaction, this is probably not all that important as the C++ binding (at least currently) has no logic at either sender or receiver to do anything special for dynamic nodes - it just passes the info on to the application which is expected to validate and act on that information.
   So from the point of view of this test it is only important to check that the properties get correctly carried in both directions. 
   
   I very much disagree. The test should verify functionality actually works the way it needs to be used, and thus help ensure someone doesn't go breaking it later. A test of 'dynamic node [properties]' usage that doesnt actually use the handling/options for dynamic nodes in the required way is simply not a good test. It means it has never been verified to fully work in the situation it needs to, and its never being fully verified it isnt broken later. Assuming things work a certain way is how so many bugs come up when it later turns out it doesnt.
   
   If there is a test of the 'no node properties' dynamic usage elsewhere then that would at least mean the 2 bits were somewhat tested in isolation even if the node-props bit was still not actually in full (which would still be poor but not quite as much). I might expect that other testing to be in this same test class, but it doesnt appear so. Is that bit actually tested right now anywhere?
   
   >Unless the tester itself generates the requested nodes the API is not going to so there really isn't anything to validate in the test.
   In a future change to the C++ binding it would definitely make sense to validate that you can't set address and set dynamic if you are sender/target or receiver/source. And also that if dynamic node properties are set the dynamic must also be set for the node, but that isn't done by the binding (or proton-c) currently so testing for it makes no sense. Producing realistic protocol interactions makes some sense but only to make the test "more realistic".
   
   Youd simply check the request, set an address value to pass back, mark it dynamic, and then verify the receiver got that address and flag. That would in fact verify that the client options to set dynamic work as expected, that the client is able to read back the provided dynamic address as it needs to be able to, and inspect the dynamic flag was set appropriately. Thats not 'nothing to validate', and is not about just being 'more realistic', but about actually testing what needs to work to use this functionality.
   
   I didnt suggest adding negative tests thats you cant set the things in the illegal way you can currently, only that the test should cover testing it actually does the things it should do when used the way it should be, which appears untested.


-- 
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: dev-unsubscribe@qpid.apache.org

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



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org