+1 to Naba's proposal, I appreciate helpful tools On Fri, Oct 18, 2019 at 3:15 PM Nabarun Nag <n...@apache.org> wrote:
> @Bruce Schuchardt <bschucha...@pivotal.io> > I completely agree with that assessment that not all flaky tests are > related to the test but may involve issues with the product itself. > > @Ernie > As you can see with the example that you provided, even when committers are > expected to do their due diligence they sometimes end up doing something > that needs to be reverted. > > It's ok to have some safeguards. I plan to look at them like seatbelts in a > car, even when we are diligent, unexpected things happen. > > My intention with this email was *never* to blame others / point out PRs > that broke the build, etc. > I want guard rails that will help even me from making mistakes. > > I understand that the consensus is that distributed/integration/upgrade > tests at the moment are flaky, hence blocking the merge button because of > them will not be an efficient call. > > *NEW PROPOSAL*: baby steps. > *Github's Branch Protection Rule *has the following rule settings that I > propose to activate: > - Require pull request reviews before merging > - Require status checks to pass before merging > [Only for > - concourse-ci/Build > - concourse-ci/UnitTestOpenJDK11 > - concourse-ci/UnitTestOpenJDK8 > - concourse-ci/StressNewTestOpenJDK11] > > *Advantages:* > - Prevent us from accidentally merging PRs without reviews, ensure that we > follow *the Apache way* of involving the community in what code is going > into the repo. > - Our build is not flaky, hence it helps us in avoiding merging code which > will break the build > - Avoid mistakenly merging spotless errors > - Unit tests are not flaky hence they can be included > - Any new tests that are being added can't be merged if they fail the > stress test. > > > Apache values people over process, I highly appreciate that sentiment but > they never said not to take help from tools to save time and avoid > mistakes. > > If we search for this feature in INFRA tickets, a lot of Apache projects > have enabled this feature for their repositories. > > Regards > Naba > > > > On Fri, Oct 18, 2019 at 1:39 PM Bruce Schuchardt <bschucha...@pivotal.io> > wrote: > > > Given the current state of affairs I don't think we should disable the > > merge button. > > > > Ultimately it would be nice if we fixed all of the flaky tests. > > Personally I don't think all of the tests that we think are "flaky" are > > test problems. In the last few months I've fixed product problems that > > were hit by supposedly "flaky" tests. Those tests were just unable to > > reproduce the product problem 100% of the time. A flickering test > > doesn't always mean it's a problem with the test. > > > > On 10/18/19 12:46 PM, Ernest Burghardt wrote: > > > I had one recently that was Approved and I merged pre-maturely and had > to > > > be reverted: d63638e4654bc6c71a232838b745dec6ef476ec9 > > > > > > Subsequently I have run into some test flakiness, but if a PR submitter > > has > > > a pre-checkin failure it could be tricky to tell that its a Flaky > > > situation... In my last go at a Flaky failure in pre-checkin, I was > able > > to > > > search the Geode Jira and found the failure was a known flaky like this > > one > > > <https://issues.apache.org/jira/browse/GEODE-6324> > > > > > > I'd prefer to trust our committers to perform their due diligence and > > make > > > good choices. > > > > > > EB > > > > > > On Fri, Oct 18, 2019 at 12:18 PM Owen Nichols <onich...@pivotal.io> > > wrote: > > > > > >> Do you have a recent example of a PR that was merged despite failed PR > > >> checks, which then broke the build? > > >> > > >> At last discussion, one concern raised was providing a way that anyone > > in > > >> the community could re-trigger a failed PR check if it hit an > unrelated > > >> flaky failure. > > >> > > >> Let’s be sure we've identified the problem before assuming the > solution. > > >> Apache values people over process. > > >> > > >>> On Oct 18, 2019, at 11:48 AM, Nabarun Nag <n...@apache.org> wrote: > > >>> > > >>> Hi devs, > > >>> > > >>> A few months ago a proposal was brought up regarding blocking the > merge > > >>> button on the github PR page in case of failing tests in the > precheck. > > >>> > > >>> What is the sentiment regarding this now? Do we feel that it should > be > > >>> implemented? > > >>> > > >>> Or at least take the minimal step of not allowing merge till all > tests > > >> are > > >>> done? > > >>> > > >>> > > >>> Regards > > >>> Nabarun Nag > > >> > > >
