openstack team mailing list archive

[Eric Day <eday@xxxxxxxxxxxx>]

I also agree, except for the last part. While you can break a test
case but not a feature, we fail at providing adequate coverage for
the feature (one of the criteria stated below). Not everyone will have
the ability to verify functionality without test cases, so we should
not depend on manual external testing, and we shouldn't disable a
test case to make a merge successful. I know we just did with the
eventlet branch because of the twisted issues around object store,
so there are special exceptions that can be made.

-Eric

On Tue, Dec 21, 2010 at 12:47:47PM -0500, Jay Pipes wrote:
> FWIW, I agree with all your points on this, Rick.
> 
> On Tue, Dec 21, 2010 at 11:43 AM, Rick Clark <rick@xxxxxxxxxxxxx> wrote:
> > As most of you know, the Nova merge queue is starting to back up.  In an
> > effort to help clear the log jam, I would like to propose some criteria
> > for how we do code reviews and what we approve.
> >
> > Statement of Fact: All code has bugs. Bugs are normal and can have no
> > impact, or can be critical.  We have all types in our code
> >
> > Statement of Fact: Regressions, bugs that break previously working
> > features, are bad, and should be avoided.
> >
> > I think that we should focus on finding regressions at review time not bugs.
> >
> > I propose that the following criteria be used for approving code reviews:
> >  * Architectural soundness
> >  * regression free
> >  * Code cleanliness (Pep8 compliance) and style
> >  * Complete test coverage
> >  * Documentation
> >
> >
> > Any obvious non-regressing bugs should be filed in Launchpad at review
> > time by the reviewer.
> >
> > Basically I think this will solve a couple problems.  We spend quite a
> > bit of time in an iterative process with a patches author fixing issues
> > in a patch.  Some of these need to be fixed by the original author prior
> > to merge, but many do not.  As soon as a patch is merged it is owned by
> > everyone and anyone can fix the issues.  This also will allow new
> > developers to grab relatively simple bugs from merges to cut their teeth
> > on the code.
> >
> > I would also say that breaking a test is not a regression it is a bug.
> > If a merge breaks a test, but does not break the  actual feature it is a
> > bug in the test.  But I think this is more controversial, so I'm just
> > bringing it up for discussion and don't propose any changes.
> >
> >
> > Thoughts?
> >
> > Rick
> >
> >
> > _______________________________________________
> > Mailing list: https://launchpad.net/~openstack
> > Post to     : openstack@xxxxxxxxxxxxxxxxxxx
> > Unsubscribe : https://launchpad.net/~openstack
> > More help   : https://help.launchpad.net/ListHelp
> >
> >
> 
> _______________________________________________
> Mailing list: https://launchpad.net/~openstack
> Post to     : openstack@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~openstack
> More help   : https://help.launchpad.net/ListHelp