On Sun, Sep 04, 2016 at 07:43:04PM +0200, Niels de Vos wrote: > On Fri, Sep 02, 2016 at 12:12:00PM -0400, Jeff Darcy wrote: > > > We already only merge after NetBSD-regression and CentOS-regression have > > > voted > > > back. All I'm changing is that you don't need to do the merge manually or > > > do > > > Verified +1 for regression to run.. Zuul will run the tests after you get > > > Code-Review +2 and merge it for you with patches ordered correctly. > > > > The problem is that some reviewers (including myself) might not even look at > > a patch until it already has CentOS+1 and NetBSD+1. Reviewing code, having > > it fail regressions, reviewing a substantially new version, having *that* > > fail regressions, etc. tends to be very frustrating for both authors and > > reviewers. Fighting with the regression tests *prior* to review can still > > be very frustrating for authors, but at least it doesn't frustrate reviewers > > as much and doesn't contribute to author/reviewer animosity (apparently a > > real problem in this group) as much. > > This is the main problem I see with this proposal too. There are quite > regular patches posted that break things in related regression tests. > Those corner cases can be very difficult to spot in code review, but > automated testing catches them. It is one of the reasons I prefer to do > code reviews for changes that are known to be (mostly) correct. Other > times changes (they should!) add test-cases, and these fail with the > initial versions...
This is a problem I hadn't realized we had. This makes it mostly a no-go to do regressions after code review. > > > That said, it would be nice to have *something* as a gate between +2 and > > merge - certainly a build, and at least a few basic tests (more than smoke > > does IMO). If Zuul can help us avoid broken builds due to improper merge > > order, which seem to be the most common kind of broken builds, I'm all for > > it. > > Our regression test suite allows running tests in a subdirectory, so > changes to the tests should not really be needed. In addition to the > simple smoke test, this might do: > > # ./run-tests.sh tests/basic/ Perhaps, we could do build + smoke + tests/basic pre-merge so we catch *some* of the edge cases. > > Niels -- nigelb _______________________________________________ Gluster-infra mailing list [email protected] http://www.gluster.org/mailman/listinfo/gluster-infra
