astitcher commented on pull request #346: URL: https://github.com/apache/qpid-proton/pull/346#issuecomment-999598356
> > 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. I'm not sure we're actually disagreeing very much here! I agreee it makes the most sense to test the API in the way that it would be actually used - my major point is simply that the binding code has no magic in it to ensure correct AMQP semantics and so this cannot be tested here. A minor point we do disagree on is that I think testing that the values get transferred correctly is the most important point and I think @gemmellr is saying that is not much use unless it actually tests what might be used - I don't think this disagreement matters much! Doing it the @gemmellr way satisfies me too! > > 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? > Yes it would be really good to have that test here too! But currently we have no C++ test for this at all. -- 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]
