cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Hugo Trippaers <HTrippa...@schubergphilis.com>
Subject Re: [MERGE]Storage refactor branch
Date Fri, 22 Feb 2013 20:32:33 GMT


Verstuurd vanaf mijn iPad

Op 22 feb. 2013 om 19:44 heeft "Edison Su" <Edison.su@citrix.com> het volgende geschreven:

> 
> 
>> -----Original Message-----
>> From: Hugo Trippaers [mailto:HTrippaers@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.su@citrix.com> het
>> volgende geschreven:
>> 
>>> 
>>> 
>>>> -----Original Message-----
>>>> From: Hugo Trippaers [mailto:HTrippaers@schubergphilis.com]
>>>> Sent: Friday, February 22, 2013 2:46 AM
>>>> To: cloudstack-dev@incubator.apache.org; Edison Su
>>>> Cc: John Burwell <jburwell@basho.com> (jburwell@basho.com); Mike
>>>> Tutkowski (mike.tutkowski@solidfire.com)
>>>> Subject: RE: [MERGE]Storage refactor branch
>>>> 
>>>> 
>>>> 
>>>>> -----Original Message-----
>>>>> From: Chip Childers [mailto:chip.childers@sungard.com]
>>>>> Sent: donderdag 21 februari 2013 21:17
>>>>> To: cloudstack-dev@incubator.apache.org; Edison.su@citrix.com
>>>>> Cc: John Burwell <jburwell@basho.com> (jburwell@basho.com); Mike
>>>>> Tutkowski (mike.tutkowski@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

Mime
View raw message