OSDir

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [VOTE] Stricter commit guidelines


+1 Default retry on failing test should help. Another way to identify test
issues is to run them in the same batch as being run in the precommit job.

On Tue, May 15, 2018 at 4:11 PM, Deepak Jaiswal <djaiswal@xxxxxxxxxxxxxxx>
wrote:

> +1
>
> On 5/15/18, 4:06 PM, "Jesus Camacho Rodriguez" <jcamacho@xxxxxxxxxx>
> wrote:
>
>     That is already part of the policy although it apparently remained
> unnoticed:
>
>     *If a commit introduces new test failures, the preferred process is to
> revert the patch, rather than opening a new JIRA to fix the new failures.*
>
>     It can be reworded to be stricter... But in any case, we should all
> enforce it from now on.
>
>     -Jesús
>
>
>     On 5/15/18, 3:55 PM, "Sergey Shelukhin" <sergey@xxxxxxxxxxxxxxx>
> wrote:
>
>         +1. Can we also add something about revert-first policy if some
> patch
>         breaks tests?
>         So that it’s ok to revert if the tests aren’t fixed quickly with a
>         follow-up.
>
>         On 18/5/15, 13:08, "Jesus Camacho Rodriguez" <jcamacho@xxxxxxxxxx>
> wrote:
>
>         >I was hoping that by being stricter, we are going to make an
> effort to
>         >fix the flaky
>         >tests. The reason is precisely what you mention: if you cannot
> commit,
>         >you need
>         >to fix this situation. That is what I meant with providing an
> additional
>         >incentive to
>         >make testing more robust: currently there is no incentive. I do
> not think
>         >that
>         >improving tests is a responsibility of a single developer, but
> rather a
>         >responsibility
>         >of all of us. Disabling the tests is a one-time solution to get
> to a
>         >clean run, trying to
>         >accelerate the process to get to it, as we did not want to block
>         >development for
>         >weeks. Then flaky tests should just not go in, and if they do, we
> can
>         >just revert
>         >the patch (this is what [2] says btw).
>         >
>         >The first thing we need to do is identifying why a test is flaky.
> After
>         >examining runs
>         >for the last few days, I saw many of them fall in following
> categories:
>         >- Many of them are flaky because of estimations such as data
> size. One
>         >possible
>         >solution is to mask data size for those tests, as we already mask
> some
>         >environment
>         >dependent information.
>         >- Some of them are flaky because environment issues, e.g., I see
> this a
>         >lot with
>         >TestTriggersMoveWorkloadManager. If their logic cannot be
> rewritten, a
>         >possible
>         >solution is to add a max number of retries selectively for those
> tests
>         >(surefire has
>         >a rerunFailingTestsCount option that I am not familiar with),
> expecting
>         >that they
>         >pass at least once.
>         >
>         >Not sure if you have other ideas?
>         >
>         >-Jesús
>         >
>         >
>         >On 5/15/18, 11:59 AM, "Vihang Karajgaonkar" <vihang@xxxxxxxxxxxx>
> wrote:
>         >
>         >    Can we also define a standard process to identify a flaky
> test and
>         >thereby
>         >    making it eligible to be disabled? I am worried that the
> intermittent
>         >the
>         >    flaky ones will stall the patches when we restart allowing the
>         >commits.
>         >
>         >    On Tue, May 15, 2018 at 10:50 AM, Vineet Garg <
> vgarg@xxxxxxxxxxxxxxx>
>         >wrote:
>         >
>         >    > +1
>         >    >
>         >    > > On May 15, 2018, at 9:13 AM, Alan Gates <
> alanfgates@xxxxxxxxx>
>         >wrote:
>         >    > >
>         >    > > +1.
>         >    > >
>         >    > > Alan.
>         >    > >
>         >    > > On Tue, May 15, 2018 at 9:12 AM, Sergio Pena
>         ><sergio.pena@xxxxxxxxxxxx>
>         >    > > wrote:
>         >    > >
>         >    > >> +1
>         >    > >>
>         >    > >> On Tue, May 15, 2018 at 11:05 AM, Gunther Hagleitner <
>         >    > >> ghagleitner@xxxxxxxxxxxxxxx> wrote:
>         >    > >>
>         >    > >>> +1
>         >    > >>> ________________________________________
>         >    > >>> From: Sankar Hariappan <shariappan@xxxxxxxxxxxxxxx>
>         >    > >>> Sent: Tuesday, May 15, 2018 9:03 AM
>         >    > >>> To: dev@xxxxxxxxxxxxxxx
>         >    > >>> Subject: Re: [VOTE] Stricter commit guidelines
>         >    > >>>
>         >    > >>> +1
>         >    > >>>
>         >    > >>>
>         >    > >>> On 15/05/18, 9:30 PM, "Sahil Takiar" <
> takiar.sahil@xxxxxxxxx>
>         >wrote:
>         >    > >>>
>         >    > >>>> +1
>         >    > >>>>
>         >    > >>>> On Tue, May 15, 2018 at 10:56 AM, Owen O'Malley <
>         >    > owen.omalley@xxxxxxxxx
>         >    > >>>
>         >    > >>>> wrote:
>         >    > >>>>
>         >    > >>>>> +1
>         >    > >>>>>
>         >    > >>>>> On Tue, May 15, 2018 at 8:55 AM, Peter Vary
>         ><pvary@xxxxxxxxxxxx>
>         >    > >> wrote:
>         >    > >>>>>
>         >    > >>>>>> +1 - Hoping for something like this for a long
> while! Thanks
>         >for
>         >    > >>> taking
>         >    > >>>>>> this up all!
>         >    > >>>>>>
>         >    > >>>>>>> On May 15, 2018, at 5:44 PM, Jesus Camacho
> Rodriguez <
>         >    > >>>>>> jcamacho@xxxxxxxxxx> wrote:
>         >    > >>>>>>>
>         >    > >>>>>>> Forgot to mention the length of the vote in original
>         >message.
>         >    > >>>>>>>
>         >    > >>>>>>> Let's leave the vote open for a shorter period than
> usual,
>         >for
>         >    > >>> instance
>         >    > >>>>>> 48 hours, i.e., till Wednesday 10pm PST. Situation
> can only
>         >get
>         >    > >> worse
>         >    > >>>>> than
>         >    > >>>>>> it is now if we do not take action for a longer
> period.
>         >    > >>>>>>>
>         >    > >>>>>>> As Alan suggested, vote passes if there is a lazy
> majority
>         >(at
>         >    > >>> least 3
>         >    > >>>>>> votes, more +1s than -1s).
>         >    > >>>>>>>
>         >    > >>>>>>> Thanks,
>         >    > >>>>>>> Jesús
>         >    > >>>>>>>
>         >    > >>>>>>>
>         >    > >>>>>>> On 5/15/18, 8:37 AM, "Andrew Sherman"
>         ><asherman@xxxxxxxxxxxx>
>         >    > >>> wrote:
>         >    > >>>>>>>
>         >    > >>>>>>>   +1
>         >    > >>>>>>>
>         >    > >>>>>>>   On Tue, May 15, 2018 at 2:34 AM Rui Li
>         ><lirui.fudan@xxxxxxxxx>
>         >    > >>>>> wrote:
>         >    > >>>>>>>
>         >    > >>>>>>>> +1
>         >    > >>>>>>>>
>         >    > >>>>>>>> On Tue, May 15, 2018 at 2:24 PM, Prasanth
> Jayachandran <
>         >    > >>>>>>>> pjayachandran@xxxxxxxxxxxxxxx> wrote:
>         >    > >>>>>>>>
>         >    > >>>>>>>>> +1
>         >    > >>>>>>>>>
>         >    > >>>>>>>>>
>         >    > >>>>>>>>>
>         >    > >>>>>>>>> Thanks
>         >    > >>>>>>>>> Prasanth
>         >    > >>>>>>>>>
>         >    > >>>>>>>>>
>         >    > >>>>>>>>>
>         >    > >>>>>>>>> On Mon, May 14, 2018 at 10:44 PM -0700, "Jesus
> Camacho
>         >    > >> Rodriguez"
>         >    > >>> <
>         >    > >>>>>>>>> jcamacho@xxxxxxxxxx<mailto:jcamacho@xxxxxxxxxx>>
> wrote:
>         >    > >>>>>>>>>
>         >    > >>>>>>>>>
>         >    > >>>>>>>>> After work has been done to ignore most of the
> tests that
>         >were
>         >    > >>>>> failing
>         >    > >>>>>>>>> consistently/intermittently [1], I wanted to
> start this
>         >vote to
>         >    > >>>>> gather
>         >    > >>>>>>>>> support from the community to be stricter wrt
> committing
>         >patches
>         >    > >>> to
>         >    > >>>>>> Hive.
>         >    > >>>>>>>>> The committers guide [2] already specifies that a
> +1
>         >should be
>         >    > >>>>> obtained
>         >    > >>>>>>>>> before committing, but there is another clause
> that allows
>         >    > >>> committing
>         >    > >>>>>>>> under
>         >    > >>>>>>>>> the presence of flaky tests (clause 4). Flaky
> tests are
>         >as good
>         >    > >> as
>         >    > >>>>>> having
>         >    > >>>>>>>>> no tests, hence I propose to remove clause 4 and
> enforce
>         >the +1
>         >    > >>> from
>         >    > >>>>>>>>> testing infra before committing.
>         >    > >>>>>>>>>
>         >    > >>>>>>>>>
>         >    > >>>>>>>>>
>         >    > >>>>>>>>> As I see it, by enforcing that we always get a +1
> from the
>         >    > >> testing
>         >    > >>>>>> infra
>         >    > >>>>>>>>> before committing, 1) we will have a more stable
> project,
>         >and 2)
>         >    > >>> we
>         >    > >>>>>> will
>         >    > >>>>>>>>> have another incentive as a community to create a
> more
>         >robust
>         >    > >>> testing
>         >    > >>>>>>>>> infra, e.g., replacing flaky tests for similar
> unit tests
>         >that
>         >    > >> are
>         >    > >>>>> not
>         >    > >>>>>>>>> flaky, trying to decrease running time for tests,
> etc.
>         >    > >>>>>>>>>
>         >    > >>>>>>>>>
>         >    > >>>>>>>>>
>         >    > >>>>>>>>> Please, share your thoughts about this.
>         >    > >>>>>>>>>
>         >    > >>>>>>>>>
>         >    > >>>>>>>>>
>         >    > >>>>>>>>> Here is my +1.
>         >    > >>>>>>>>>
>         >    > >>>>>>>>>
>         >    > >>>>>>>>>
>         >    > >>>>>>>>> Thanks,
>         >    > >>>>>>>>>
>         >    > >>>>>>>>> Jes?s
>         >    > >>>>>>>>>
>         >    > >>>>>>>>>
>         >    > >>>>>>>>>
>         >    > >>>>>>>>> [1]
>         >http://mail-archives.apache.org/mod_mbox/hive-dev/201805.
>         >    > >>>>>>>>>
>         >mbox/%3C63023673-AEE5-41A9-BA52-5A5DFB2078B6%40apache.org%3E
>         >    > >>>>>>>>>
>         >    > >>>>>>>>> [2] https://cwiki.apache.org/
> confluence/display/Hive/
>         >    > >>>>>>>>> HowToCommit#HowToCommit-PreCommitruns,
> andcommittingpatches
>         >    > >>>>>>>>>
>         >    > >>>>>>>>>
>         >    > >>>>>>>>>
>         >    > >>>>>>>>>
>         >    > >>>>>>>>>
>         >    > >>>>>>>>
>         >    > >>>>>>>>
>         >    > >>>>>>>> --
>         >    > >>>>>>>> Best regards!
>         >    > >>>>>>>> Rui Li
>         >    > >>>>>>>>
>         >    > >>>>>>>
>         >    > >>>>>>>
>         >    > >>>>>>>
>         >    > >>>>>>
>         >    > >>>>>>
>         >    > >>>>>
>         >    > >>>>
>         >    > >>>>
>         >    > >>>>
>         >    > >>>> --
>         >    > >>>> Sahil Takiar
>         >    > >>>> Software Engineer
>         >    > >>>> takiar.sahil@xxxxxxxxx | (510) 673-0309
>         >    > >>>
>         >    > >>>
>         >    > >>>
>         >    > >>
>         >    >
>         >    >
>         >
>         >
>         >
>
>
>
>
>
>
>