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. Thanks, Anthony > -----Original Message----- > From: Chip Childers [mailto:chip.child...@sungard.com] > Sent: Friday, February 08, 2013 10:32 AM > To: Anthony Xu > Cc: cloudstack-dev@incubator.apache.org > 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 CreateVlanIpRangeCmd.java > 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
