> -----Original Message----- > From: Hugo Trippaers [mailto:htrippa...@schubergphilis.com] > Sent: Friday, February 22, 2013 10:03 AM > To: Edison Su > Cc: cloudstack-dev@incubator.apache.org > Subject: Re: [MERGE]Storage refactor branch > > > > Verstuurd vanaf mijn iPad > > Op 22 feb. 2013 om 18:32 heeft "Edison Su" <edison...@citrix.com> het > volgende geschreven: > > > > > > >> -----Original Message----- > >> From: Hugo Trippaers [mailto:htrippa...@schubergphilis.com] > >> Sent: Friday, February 22, 2013 2:46 AM > >> To: cloudstack-dev@incubator.apache.org; Edison Su > >> Cc: John Burwell <jburw...@basho.com> (jburw...@basho.com); Mike > >> Tutkowski (mike.tutkow...@solidfire.com) > >> Subject: RE: [MERGE]Storage refactor branch > >> > >> > >> > >>> -----Original Message----- > >>> From: Chip Childers [mailto:chip.child...@sungard.com] > >>> Sent: donderdag 21 februari 2013 21:17 > >>> To: cloudstack-dev@incubator.apache.org; edison...@citrix.com > >>> Cc: John Burwell <jburw...@basho.com> (jburw...@basho.com); Mike > >>> Tutkowski (mike.tutkow...@solidfire.com) > >>> Subject: Re: [MERGE]Storage refactor branch > >>> > >>> On Wed, Feb 20, 2013 at 03:55:59PM -0500, Chip Childers wrote: > >>>> On Fri, Feb 15, 2013 at 11:03:27AM -0800, Edison Su wrote: > >>>>> My branch is not a feature branch, while other features are > >>>>> depended on > >>> it. I didn't add any new feature on the branch, all the existing > >>> marvin automated tests should work. Instead of testing and fixing on > >>> my branch then merge, is it better to test and fix on master after > >>> the merge, using existing marvin test? > >>>> > >>>> IMO, it's not ever good to intentionally to break master. > >>> > >>> Edison - I see that you merged this into master today. Is master > >>> now in a state where it's broken? Did you run the marvin tests > >>> against your branch prior to the merge? > >> > >> I'm pretty surprised by this merge. We have about three running > >> threads on the developer list regarding testing and the overall > >> quality of the master branch. This particular merge thread on the ML > >> has valid concerns for testing of the branch, which have not been > >> addressed. Yet all this is ignored and the branch is merged anyway? > >> This is not what we all agreed to do, Edison, could you please explain why > you did this? > > > > I send out the request, there is no objection in 72 hours, so I think I can > merge it in. > > I guess I should have been more clear in choosing my words, I didn't approve > of this to be merged without unit tests, but I think I should have stated this > more clearly. > > > I am also trying to setup a marvin test, but the smoke test is > > disabled: > > http://jenkins.cloudstack.org/view/cloudstack-qa/job/test-cloudstack-s > > moke/ There are few functions(like migration volume between pools, > > create template from snapshot, etc) I haven't tested by myself, but these > functions will not block developer's daily work, even if they don't work. The > reason I don't want to test these features, because, my next next task(after > zone-wide storage) is to refactor nfs secondary storage, so I'll change that > part of code again. Then the purpose of fully test at these stage has not > much value. > > We should never merge in incomplete or untested code into master. If you > have a certain piece of code that you didn't test, you should not commit it to > master. It's not about developers daily work not being blocked, it's about the > quality of the code people push into master. If you don't want to test the > code yet because of the next feature, fine, but don't merge it then. Testing > (and specifically unit testing) is about quality of the code. Proving that the > logic does what it is supposed to do and that it can handle errors by design. > It's more than just testing and seeing if something works. But I think enough > has been said about unit testing in other threads. > > We are so far behind in unit tests that any piece of the code we touch should > result in a unit test. So please plan for this when you are working on the > next > feature and I think it would be a nice gesture if you could write and submit > the missing unit tests for this storage refactor before going to the next > feature. The change is so huge, in order to test all my changes, that will take long time. What I'll do, is gradually adding more unit tests after the merge. I wish I am just writing some new features, man, refactor is not an easy task....:)
> > > > > >> > >>> > >>> -chip
