On Tue, Mar 15, 2011 at 1:24 PM, Robert Godfrey <[email protected]>wrote:
> 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. > I actually consider this a bug as I didn't really implement what I should have in the first place. The current behaviour is neither backwards compatible nor correct as per the JMS spec. As for your comments about the tests, I fully agree with you and have said the same in my response to Robbie with some examples. But as I mentioned, that risk is **already there in 0.10** and this commit is not adding anything to it. Rajith > -- 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 >> > > >> > >> > >
