incubator-cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Chip Childers <chip.child...@sungard.com>
Subject Re: [MERGE] Security Group in Advanced zone
Date Fri, 08 Feb 2013 19:19:48 GMT
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

Mime
View raw message