cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Anthony Xu <>
Subject RE: [MERGE] Security Group in Advanced zone
Date Fri, 08 Feb 2013 19:04:09 GMT
Hi Chip,

I reverted my commits,

I agree with you, we should write more unit tests for CloudStack. Developer should write unit
test for class he introduces, developer may need to change unit test for a class if the unit
test exist and he modifies the class, 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.

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 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.


> -----Original Message-----
> From: Chip Childers []
> Sent: Friday, February 08, 2013 10:32 AM
> To: Anthony Xu
> Cc:
> Subject: Re: [MERGE] Security Group in Advanced zone
> On Fri, Feb 08, 2013 at 10:20:45AM -0800, Anthony Xu wrote:
> > Sorry, I missed your first email, I'll revert the commits soon.
> >
> > I have following questions here,
> > -What's the criteria when unit test is needed, take this as an
> example, these commits changed two API behaviors, CreateVlanIpRangeCmd
> and CreateNetworkCmd, the total change is about 100 lines, as you said,
> it only changed existing class, I think it is like a patch. what kinds
> of unit test I should write? Unit test for
> and CreateNetworkCmd? There is no existing unit tests for them, writing
> unit tests for them might be much bigger effort compared to these
> commits.
> IMO, we should be adding new unit tests as often as possible, and in
> particular for each new feature / improvement.  I understand that
> current test coverage is basically <1%, but we're never going to
> improve
> that if we keep skipping it.
> So my personal belief, is that this specific merge should, at a minimum,
> build the required unit test structure for the impacted class files to
> allow them to be tested, as well as include tests that cover the code
> paths that were introduced.
> Beyond the minimum, adding primary-path tests that cover existing code
> would be a bonus.
> I'm not trying to be an ass about it, but it seems like we expect it
> from folks submitting new features via reviewboard, and I struggle to
> understand why we wouldn't hold ourselves (committers) to that same
> level of rigor for merges into master.
> > -should we always merge master to feature branch first, then merge
> back to master? The reason I didn't do it is, the change in sg-in-
> advanced-zone is small, the change in master is big(it includes javelin
> check), merging master to sg-in-advanced-zone branch is harder. I
> merged sg-in-advanced to master, and did basic integrated test.
> >
> Totally up to you.  Hearing why the diff was so large, it makes sense.
> I
> just wasn't sure what was going on, hence my question.
> > - should we have staging tree for master? All commits go to staging
> tree first, after they pass the automation test, they will be merged to
> master automatically.
> >
> I think this goes back to the gerrit discussion, and it's a great idea
> when we have the right tooling in place to support reviews + CI
> confirmation of any commits.
> >
> > Thanks,
> > Anthony

View raw message