+1 On Fri, Jan 7, 2022 at 5:06 AM Vikram Koka <[email protected]> wrote:
> +1 > I vehemently agree > > On Thu, Jan 6, 2022 at 2:53 PM Aizhamal Nurmamat kyzy > <[email protected]> wrote: > >> +1 >> >> On Thu, Jan 6, 2022 at 6:31 AM Kaxil Naik <[email protected]> wrote: >> >>> +1 - Agree with the Proposal, will take care of it myself too >>> >>> On Thu, Jan 6, 2022 at 5:21 PM Ash Berlin-Taylor <[email protected]> wrote: >>> >>>> Given the work you and the Outreachy interns?) have put in to fix the >>>> previous flaky tests I 100% agree. >>>> >>>> Main is now in much better state with greatly reduced number of flaky >>>> tests/false negatives, so yes, if we see a build fail it should be treated >>>> as a real failure. >>>> >>>> On 6 January 2022 10:39:49 GMT, Jarek Potiuk <[email protected]> wrote: >>>>> >>>>> Hey everyone, >>>>> >>>>> I know we had quite a long period of flaky tests and accepting the >>>>> fact that we merge PRs with some tests failing because of the >>>>> flakiness. >>>>> >>>>> However I think over a couple of months or so we have invested heavily >>>>> into fixing it - a number of people tracked and fixed a big number of >>>>> flaky tests and what we have now is mostly "Green". >>>>> >>>>> Yeah - sometimes it happens we - by mistake merge a change that causes >>>>> "main" failure (for example because our test harness is not perfect) >>>>> but we should fix those cases quickly (mostly by reverting the >>>>> offending commit and redoing it). >>>>> >>>>> But I think we should (and I am talking about committers) stop the >>>>> case of merging "failed" PRs if we are not absolutely sure that the >>>>> failure is already fixed in PR (or being fixed) . >>>>> >>>>> We had some changes merged recently (and I was as guilty as others) >>>>> where we merged a "real" failure without properly investigating the >>>>> root cause. The effect of that is the "broken window" effect - once >>>>> such PR gets merged, it fails other PRs (until fixed) and it makes >>>>> people impatient to merge PR with the failure because this is >>>>> "normal". It should be normal to only merge "green" PRs. >>>>> >>>>> I propose that we change our approach and whenever we see a "red" >>>>> build every committer's approach should be : >>>>> >>>>> * investigate the root cause >>>>> * if it's main - attempt to fix it in main first before merging (could >>>>> be by reverting the failed commit) >>>>> * or discuss it in #development /devlist if it is not easy to find >>>>> * and generally only merge a failed PR if you are absolutely sure the >>>>> failure has already been fixed (or you know someone works on fixing >>>>> it) - and ALWAYS comment about it in the PR explaining why you merge >>>>> failed PR >>>>> >>>>> This is a proposal, happy to discuss it if others think differently. WDYT >>>>> ? >>>>> >>>>> J. >>>>> >>>>> -- > > > > Vikram Koka > > *SVP Engineering* > > Email: [email protected] > > Mobile: +1 408 966 2203 >
