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-smoke/ > 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. > >> >>> >>> -chip
