I see your point, I think it should be case by case, In this feature, 100 line change are in 6 "manager" class, there is no unit framework for them, to be honest, I don't know to create the unit framework for manager class, I believe it is a big task. If the framework is there, I'm glad to add test cases inside framework to test the path I changed.
If every function change needs unit test change, and as you said there is <1% unit coverage. This will be a big burden for developer, may slow down development. We may need to balance between them. For this case, what's the next step? Call for vote, or Call for review? Thanks, Anthony > -----Original Message----- > From: Chip Childers [mailto:chip.child...@sungard.com] > Sent: Friday, February 08, 2013 11:20 AM > To: Anthony Xu > Cc: cloudstack-dev@incubator.apache.org > Subject: Re: [MERGE] Security Group in Advanced zone > > On Fri, Feb 08, 2013 at 11:04:09AM -0800, Anthony Xu wrote: > > But it is unfair developer should write a complete unit test for > class if the unit test doesn't exist and he modifies the class, because > he may not understand class well, he may not be able to write a unit > test for the class. > > That's not what I suggested. What I suggested was that the minimum is > to create the scaffolding required to test the new code paths. If > someone doesn't know enough about how a class works to test their own > changes, should they be making that change? > > This is obviously not going to always apply, but I think that we can > use > our subjective judgement to know if modifications are significant or > not. If they are significant, they would have required enough of an > understanding of the class to help setup the structure for testing it, > OR the developer should take the opportunity to learn it. > > Now if you *also* have the time and knowledge to add tests for the > pre-existing code, then that's a bonus that helps all of us out. > > > I think we should file a bug for each class if unit test for this > class doesn't exist to add unit test for the class, and assign them to > proper people or volunteers. > > I disagree, because of the statement above. Significant changes to a > class should be made with enough of an understanding of how the class > works to create tests that cover the new / changed code path. > > > I checked other feature merges, unit test for new class were added, > but few of them have unit test for modified class, in the case , it is > unfair to this feature. > > Sorry, but I'm not trying to single you out. It's time for us to do > this, > period. You just happened to be one with a merge that got noticed. > Please don't take it personally. > > We let a bunch of things go by for 4.1, even though we had all agreed > that unit tests were a requirement for new features before we started > working on 4.1. The IP clearance issues became the major problem area, > so I didn't focus as much on checking for unit tests. Now that we're > moving on to new features for post-4.1, I think we actually need to > apply the practices that we all agreed on. > > Frankly, we should be self policing on this topic. Gerrit will help, > but folks need to actually embrace and execute on the goal of adding > tests with changes. > > -chip