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
