I see both side here, I think the fundamental issue for me is that I hadn't previously picked up that when the change to the default addressing syntax was changed in the 0.8 release, the tests were made to explicitly run with the Binding URL as the default. As such we are not exposing the "default" that we are giving to users when we run the tests - thus the expected normal code path is not being exercised in our tests other than in the one test that Robbie highlighted (and this uses the explicit "ADDR:" syntax).
As such I don't feel like simply running the tests gives me the confidence that this change will not have unforseen consequences. In truth I actually now feel quite nervous about what we did in 0.8. I strongly feel that we should update the tests to use the default (which is now ADDR) syntax - changing any test where the BindingURL is implicitly expected. Given that this is a "change" rather than a "bug fix", the fact that we are supposed to be in code freeze in preparation for release, and that I don't have confidence that we are adequately testing the "default" behaviour using the standard tests - I don't feel that it is appropriate to include in 0.10. -- Rob On 15 March 2011 16:34, Rajith Attapattu <[email protected]> wrote: > On Tue, Mar 15, 2011 at 12:08 PM, Robbie Gemmell > <[email protected]>wrote: > > > I feel that this change should not be included in the 0.10 release. > > > > I think we are too close to release (RC1 is due tomorrow yes?) and that > > there isnt significant enough testing in this area to provide enough > > confidence when making what is actually an appreciable change in client > > behaviour at this stage. > > > > > I actually disagree with your assessment. > I certainly consider this a **very low risk** change, given that addressing > is the *default*. > The only difference here is that the create option is not selected and the > impact of that is an exception will be thrown if the queue is not present. > > There is absolutely no other **extra** risk involved here. > The risk you perceive here is the impact of the addressing implementation > due to lack of tests which is a valid concern. > However that risk is there whether or not this commit is included or not. > > This commit does not include anymore risk in the client than what is > already > there due to addressing being the default. > Does that make sense ? Do you agree with that ? > > > > So far as I can tell from the quick look I've taken so far > > AddressBasedDestinationTest is the only test which exercises the > Addressing > > code to any extent, and does so often by prefixing the address strings > with > > 'ADDR:' because the test profiles are all running set to use the old > > BindingURL syntax. As a result it would seem we ultimately arent testing > > the > > precise execution paths for many operations users will undertake at all. > > > > > This is one of the biggest obstacles I had when testing the new address > based implementation. > Switching the default will create many test failures that is impossible to > handle on my own. > Also switching the default is not a good solution either as we still > support > the older binding URL mechanism as well. > > The addressing scheme actually exercised code paths in a certain way that > it > highlighted several race conditions, a few dead locks and many other errors > that were there in the code base for a long time. Therefore it would have > been nice had I been able to catch these issues using our test suites. Ex > issues in durable subscribers, queue receivers etc.. > > I also believe this highlights a weakness in a our test suite - all though > this is separate discussion on it's own. > We should probably try to use the JMS interfaces as much as possible when > writing our integration tests and have enough provisions to abstract out as > many details as possible. > This allows us to easily configure a single test to run under different > configurations. > > Rajith > > Robbie > > > > On 14 March 2011 16:48, Rajith Attapattu <[email protected]> wrote: > > > > > The attached patch in QPID-3143 contains a simple one line fix and a > > > modified test case to test the change. > > > The same is committed to trunk at rev 1081460. > > > The fix is low risk IMHO. > > > > > > However the fix is important as it makes the session.createQueue method > > > spec > > > complaint. > > > It also allows the default behaviour to be easily overridden if the > user > > > chooses do so. > > > > > > Regards, > > > > > > Rajith > > > > > >
