Verstuurd vanaf mijn iPad
Op 22 feb. 2013 om 19:44 heeft "Edison Su" <edison...@citrix.com> het volgende geschreven: > > >> -----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....:) Granted :-) looking forward to seeing more unit tests popping up in the near future. I'll gladly help if you point me in the right direction. > >> >> >>> >>>> >>>>> >>>>> -chip
