@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
> >>
>
