james-server-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Bernd Fondermann <bf_...@brainlounge.de>
Subject Re: [jira] Updated: (JAMES-509) Cleanup/Refactor FetchMail code
Date Sat, 27 May 2006 12:57:45 GMT
Stefano,

One question: The refactoring you are providing, does it work? (Did you 
run it? Did you test all the features and functionality beforehand and 
afterwards?)

I agree with Steve that a refactoring is not worth applying if we don't 
know if it doesn't breaks something, regardeless how simple the 
refactoring is, how ugly the code was and how shining it is afterwards. 
I made this mistake too many times before myself.

Of course, we cannot catch every newly introduced bug at first commit, 
but we should try. It's much less effort than hunt it anytime later.

Even if there _are_ unit tests, I see it as my duty as a committer do 
run additional functional test by hand. Some things simply can't be done 
with unit tests.

IMHO, if you find out that the touched code works like before or better, 
go ahead and apply the patch.

And if you already did all this, please ignore my rant. ;-)

   Bernd


Stefano Bagnara wrote:
> I would like to add an example:
> 
> In James 2.1 we had FetchPOP
> 
> In James 2.2.0 FetchMail has been introduced and FetchPop deprecated.
> 
> Fetchmail was a complete rewrite and had no unit tests, and has been 
> introduced in a minor (2.#) release.
> 
> Does this mean that if I rename the refactored code to "GrabMail" we can 
> introduce it in James 2.4.0 and deprecate James 2.2.0 ? ;-)
> I know that FetchMail was a big improvement over FetchPOP in the 
> features aspect, but I'm trying to add evidence to my reasons ;-)
> 
> I just want to note that if we require people to unittests every aspect 
> of James in order to change anything this could be the death of James.
> 
> The best effort for unit tests on James I ever seen has been done by 
> Bernd and we all really appreciated this. I also wrote as much unit 
> tests as possible when working on hard things (mainly MimeMessageWrapper 
> and the CoW proxy). I also wrote most of the unit tests in jSPF, because 
> I think tests are really usefull, but I don't think James will get any 
> new feature if the prerequisite is to write unit tests for the old code.
> 
> That said I don't want to enforce a bad practice only because that 
> practive has been followed in past, I just want to have a better 
> "measure" for our discussion.
> 
> And now a generic comment on this discussions:
> 
> What I expect from the PMC committers when anyone make a proposal is a 
> clean vote in the range -1...+1 with possible technical explanations, 
> alternative solutions, and blocking issues to change a -X vote to +0, if 
> any.
> 
> Imho if we want to release James much more than once in 2 years we have 
> to enforce a more agile decision making solution.
> 
> Stefano
> 
> Steve Brewin wrote:
> 
>> My main issue is purely practical. I think it seriously unwise to make 
>> changes on such a scale to any component without unit tests with 
>> sufficient coverage to validate it still works across all usage the 
>> scenarios. This is not a minor incrementl refactoring, of which I 
>> fully approve. This is a total rewrite.
>>
>> Without unit tests, I can't understand how we can have confidence in 
>> such a rewrite when your main argument for doing so is that you do not 
>> understand the existing code, "To me it's something like obfuscated: I 
>> feel worst than browsing decompiled code ;-)".
>>
>> This is not me being protective. This is me voicing me fears of 
>> treading such a path for any component in a minor (2.#) release. As I 
>> said, "We all know that major changes are prone to creating new 
>> issues". I don't think any component with so few issues open against 
>> it justifies the risk of introducing new bugs this degree of change 
>> will inevitably entail.
>>
>> I'm more than happy for you to prove me wrong by way of unit tests. 
>> Otherwise I think this belongs in the bucket of things to consider for 
>> 3.0.
>>
>> Cheers
>>
>> -- Steve
>>
>>
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
>> For additional commands, e-mail: server-dev-help@james.apache.org
>>
>>
> 
> 
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
> For additional commands, e-mail: server-dev-help@james.apache.org
> 
> 


---------------------------------------------------------------------
To unsubscribe, e-mail: server-dev-unsubscribe@james.apache.org
For additional commands, e-mail: server-dev-help@james.apache.org


Mime
View raw message