You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Dan Smith <ds...@pivotal.io> on 2020/03/01 16:55:42 UTC
Re: [PROPOSAL] eliminate file count loophole in PR StressNewTest
Won't this make it impossible to merge refactoring changes that touch a lot
of tests?
-Dan
On Sat, Feb 29, 2020, 12:37 PM Robert Houghton <rh...@pivotal.io> wrote:
> Yes, as it should
>
> On Sat, Feb 29, 2020, 12:25 Dan Smith <ds...@pivotal.io> wrote:
>
> > Doesn't the build fail when concourse times out?
>
>
Re: [PROPOSAL] eliminate file count loophole in PR StressNewTest
Posted by Nabarun Nag <nn...@apache.org>.
+1
On Fri, Mar 6, 2020 at 6:33 PM Donal Evans <do...@pivotal.io> wrote:
> With fairly apt timing, we've recently had a PR merged (
> https://github.com/apache/geode/pull/4745) that introduced a test that has
> resulted in fairly consistently flaky failures in the pipeline (JIRA
> ticket: https://issues.apache.org/jira/projects/GEODE/issues/GEODE-7843).
> The PR was quite large and touched/created a lot of tests, so StressNewTest
> never ran on it:
> https://concourse.apachegeode-ci.info/builds/136784#L5e3ac3fc:2. Given how
> frequently the test is failing in the pipeline (11 failures on various
> IntegrationTest jobs over the past 6 commits), it seems very likely that
> had StressNewTest been allowed to run, this issue would have been caught at
> the PR stage and could have been remedied then, instead of resulting in a
> nearly constantly red pipeline.
>
> I've already given my +1 to this proposal, but this situation has prompted
> me to make it a *+1*.
>
> On Tue, Mar 3, 2020 at 2:08 PM Anilkumar Gingade <ag...@pivotal.io>
> wrote:
>
> > The stress test is to identify the flaky-ness within the test; and
> assuming
> > any changes to the test may have introduced the flaky-ness.
> > It's about paying the cost upfront or later when the test is determined
> to
> > be flaky.
> > If 25+ tests have been changed in a PR, the cost of running stress test
> for
> > those; and gating the PR for so long.
> > Knowing how much pain it causes to fix a flaky test after a certain/long
> > duration of time; I am +1 for doing this change.
> >
> > On Tue, Mar 3, 2020 at 10:06 AM Dan Smith <ds...@pivotal.io> wrote:
> >
> > > What's the current timeout for StressNewTest? Maybe if we just up the
> > > threshold to 100 tests or so and up the timeout to match we can catch
> > > pretty much all PRs.
> > >
> > > I'm not sure why the job is flagging more tests than it should. It
> looks
> > > like at some point @rhoughon changed it to read the merge base from
> some
> > > file created by concourse as an optimization [1] - I suspect maybe that
> > > file is inaccurate?
> > >
> > > I originally wrote this job. It's definitely not a panacea, it will
> only
> > > catch a new flaky test if
> > > - the test is really flaky (likely to fail more than 1/50 times)
> > > - the change actually happened in the test file itself, and not the
> > > product or some other test file.
> > >
> > > [1]
> > >
> > >
> >
> https://github.com/apache/geode/commit/4c06ba4625e69d44a5165aa9f2fccddfc064de87
> > >
> > > -Dan
> > >
> > > On Sun, Mar 1, 2020 at 9:00 PM Owen Nichols <on...@pivotal.io>
> wrote:
> > >
> > > > We don’t tend to look too closely at successful PR checks to see
> > whether
> > > > they actually checked anything at all.
> > > >
> > > > One example I found is
> > > >
> > >
> >
> https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/5957
> > > > <
> > > >
> > >
> >
> https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/5957
> > > > >:
> > > > 32 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing.
> > > >
> > > > Here are 92 more examples (url’s omitted for brevity — use the
> example
> > > > above as a template and just replace the last 4 digits):
> > > > 26 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6243)
> > > > 26 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6249)
> > > > 26 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6402)
> > > > 27 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6262)
> > > > 27 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6430)
> > > > 27 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6439)
> > > > 27 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6449)
> > > > 27 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6454)
> > > > 27 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6458)
> > > > 27 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6459)
> > > > 28 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6224)
> > > > 28 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6441)
> > > > 28 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6448)
> > > > 28 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6452)
> > > > 29 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6102)
> > > > 29 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6177)
> > > > 30 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 5939)
> > > > 30 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 5940)
> > > > 30 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 5949)
> > > > 30 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6473)
> > > > 31 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 5953)
> > > > 31 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6187)
> > > > 31 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6470)
> > > > 31 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6471)
> > > > 31 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6474)
> > > > 31 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6475)
> > > > 32 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 5958)
> > > > 32 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6173)
> > > > 32 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6236)
> > > > 32 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6237)
> > > > 32 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6242)
> > > > 33 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6246)
> > > > 33 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6248)
> > > > 33 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6250)
> > > > 33 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6251)
> > > > 33 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6254)
> > > > 33 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6255)
> > > > 34 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6139)
> > > > 34 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6141)
> > > > 34 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6264)
> > > > 34 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6266)
> > > > 34 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6271)
> > > > 35 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6142)
> > > > 35 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6143)
> > > > 35 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6277)
> > > > 35 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6309)
> > > > 35 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6413)
> > > > 35 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6414)
> > > > 35 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6419)
> > > > 35 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6420)
> > > > 36 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6144)
> > > > 36 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6146)
> > > > 36 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6147)
> > > > 36 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6310)
> > > > 36 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6326)
> > > > 36 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6329)
> > > > 36 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6330)
> > > > 40 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6159)
> > > > 40 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6160)
> > > > 40 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6188)
> > > > 41 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6440)
> > > > 41 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6480)
> > > > 41 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6483)
> > > > 43 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6190)
> > > > 44 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 5768)
> > > > 44 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 5772)
> > > > 46 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6191)
> > > > 46 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6218)
> > > > 46 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6219)
> > > > 46 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6225)
> > > > 46 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6244)
> > > > 46 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6245)
> > > > 48 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6129)
> > > > 49 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 5804)
> > > > 51 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 5877)
> > > > 53 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 5742)
> > > > 54 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 5757)
> > > > 66 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6232)
> > > > 66 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6233)
> > > > 66 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6234)
> > > > 66 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6259)
> > > > 66 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6260)
> > > > 66 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6278)
> > > > 79 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6481)
> > > > 103 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6493)
> > > > 106 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6206)
> > > > 106 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6207)
> > > > 115 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 5769)
> > > > 118 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6226)
> > > > 721 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6197)
> > > > 722 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6217)
> > > > 722 is too many changed tests to stress test. Allowing this job to
> pass
> > > > without stress testing. (build 6221)
> > > >
> > > > Please note, sometimes the number of files reported seems to be way
> > > higher
> > > > than it should be. For example
> > > https://github.com/apache/geode/pull/4691
> > > > <https://github.com/apache/geode/pull/4691> shows exactly 1 test
> file
> > > > touched, but
> > > >
> > >
> >
> https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/6278
> > > > <
> > > >
> > >
> >
> https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/6278
> > > >
> > > > thinks it touched 66 test files.
> > > >
> > > > There’s currently no good data to predict how long StressNew will
> take,
> > > it
> > > > just depends on the mix of tests touched. I am aware of at least 4
> > cases
> > > > (with less than 25 files) where the timeout was hit. In two of those
> > > cases
> > > > we re-ran with a longer timeout, and in two cases the PR author split
> > up
> > > > the PR into half a dozen smaller PRs to get around it.
> > > >
> > > >
> > > > > On Mar 1, 2020, at 7:52 PM, Anthony Baker <ab...@pivotal.io>
> wrote:
> > > > >
> > > > > What percentage of PR’s are currently subject to the 25 test file
> > rule?
> > > > How many would be subject to the concourse timeout?
> > > > >
> > > > > I’d like to understand the scope of the impact for this change.
> > > > >
> > > > > Anthony
> > > > >
> > > > >
> > > > >> On Mar 1, 2020, at 8:58 AM, Owen Nichols <on...@pivotal.io>
> > wrote:
> > > > >>
> > > > >> Impossible, no. Inconvenient, perhaps, but a small price to pay
> for
> > > > being
> > > > >> able to trust that green means green.
> > > > >>
> > > > >> With or without this proposed change, if anyone is having trouble
> > > > getting
> > > > >> their PR to pass StressNew, please bring it up on the dev list and
> > we
> > > > can
> > > > >> discuss the appropriate solution on a case-by-case basis (e.g.
> > > > increasing
> > > > >> timeout, fixing the logic that identifies changed test files,
> > > splitting
> > > > >> into multiple PRs, authorizing an override, etc).
> > > > >>
> > > > >> On Sun, Mar 1, 2020 at 8:56 AM Dan Smith <ds...@pivotal.io>
> wrote:
> > > > >>
> > > > >>> Won't this make it impossible to merge refactoring changes that
> > touch
> > > > a lot
> > > > >>> of tests?
> > > > >>>
> > > > >>> -Dan
> > > > >>>
> > > > >>> On Sat, Feb 29, 2020, 12:37 PM Robert Houghton <
> > rhoughton@pivotal.io
> > > >
> > > > >>> wrote:
> > > > >>>
> > > > >>>> Yes, as it should
> > > > >>>>
> > > > >>>> On Sat, Feb 29, 2020, 12:25 Dan Smith <ds...@pivotal.io>
> wrote:
> > > > >>>>
> > > > >>>>> Doesn't the build fail when concourse times out?
> > > > >>>>
> > > > >>>>
> > > > >>>
> > > > >
> > > >
> > > >
> > >
> >
>
Re: [PROPOSAL] eliminate file count loophole in PR StressNewTest
Posted by Bruce Schuchardt <bs...@pivotal.io>.
-1 on changing the check
I'm with Owen - let's leave things alone for now.
On 3/17/20, 7:43 PM, "Owen Nichols" <on...@pivotal.io> wrote:
At least one person on the thread (@anthony) raised concerns but has not
replied since.
Also since this thread was started, the bug that miscounted files has been
fixed, which is sufficient that stress tests would have been run for about
95% of PRs that previously (erroneously) were given a free pass. Maybe
that is enough?
I’m hesitant to raise the bar for a required check if it will become an
added hardship, and especially hesitant to create a situation where the
highly-unpopular “manual override” might be the only option.
It might be better to just make slight increases from 25 files/6 hours to
35 files/10 hours, and just ask that if anyone is touching more test files
than that (due to a refactor/rename/reformat) to please make that a
separate PR separate from any actual code changes. This is better for
reviewers too...
Discussion period goes through the end of this week.
On Tue, Mar 17, 2020 at 8:55 AM Dan Smith <ds...@pivotal.io> wrote:
> Seems like I'm the only one on this thread who even quibbled about removing
> the limit entirely. Let's go ahead and remove the limit.
>
> -Dan
>
> On Fri, Mar 13, 2020 at 8:26 AM Robert Houghton <rh...@pivotal.io>
> wrote:
>
> > I want the check to stay required for PR merges to be allowed. I also
> want
> > it to do real work in all cases. If I can add whitespace to 25 test
> classes
> > and get a free pass, thats a big testing hole.
> >
> > Remove the limit. Worst case, we have to A) bump timeouts in the future
> B)
> > put the limits back in C) come up with a new paradigm for verifying test
> > quality in a non-deterministic process
> >
> > On Tue, Mar 10, 2020 at 8:58 PM Dan Smith <ds...@pivotal.io> wrote:
> >
> > > I checked, and 1% of our last 500 commits touched more than 30 tests.
> Of
> > > those 1%, over half touched more than 100 tests. So I'd guess somewhere
> > > around .5% of prs will fall under the proposed changes.
> > >
> > > My preference would be to just raise the threshold given that we
> already
> > > raised the timeout. That'll catch 99% of prs without making test
> > > refactoring and cleanups that touch many tests hard.
> > >
> > > But, I'm ok with removing the limit if people really feel that a manual
> > > override process is better for that 1%.
> > >
> > > -Dan
> > > On Tue, Mar 10, 2020, 4:33 PM Owen Nichols <on...@pivotal.io>
> wrote:
> > >
> > > > To recap, the original question was: should we change StressNewTest
> to
> > > > pass ONLY if it is able to successfully run all new/changed tests 50
> > > times
> > > > (eliminating the existing loophole exempting PRs that touch 25 or
> more
> > > > files)?
> > > >
> > > > So far a handful of people have expressed support, but many have
> > > > questions/concerns:
> > > > - One concern was mis-counting of test files touched. As of today
> this
> > > is
> > > > now fixed.
> > > > - A big concern was that it might become more difficult to pass all
> > > > required PR checks.
> > > > - Another concern was that the current timeout of 6 hours might be
> too
> > > > high / too low.
> > > > - An alternative was suggested to keep the loophole but maybe
> increase
> > > the
> > > > threshold required to get the free pass.
> > > > - If we’re going to raise the bar on any required PR check, maybe we
> > > > should consider making it non-required.
> > > >
> > > > Let’s extend the comment period until the end of next week (Friday
> Mar
> > > 20)
> > > > in hopes of converging on a solution everybody is happy with (even if
> > it
> > > > isn’t what was originally proposed).
> > > >
> > > >
> > > > > On Mar 6, 2020, at 4:51 PM, Donal Evans <do...@pivotal.io>
> wrote:
> > > > >
> > > > > With fairly apt timing, we've recently had a PR merged (
> > > > > https://github.com/apache/geode/pull/4745) that introduced a test
> > that
> > > > has
> > > > > resulted in fairly consistently flaky failures in the pipeline
> (JIRA
> > > > > ticket:
> > > https://issues.apache.org/jira/projects/GEODE/issues/GEODE-7843
> > > > ).
> > > > > The PR was quite large and touched/created a lot of tests, so
> > > > StressNewTest
> > > > > never ran on it:
> > > > > https://concourse.apachegeode-ci.info/builds/136784#L5e3ac3fc:2.
> > Given
> > > > how
> > > > > frequently the test is failing in the pipeline (11 failures on
> > various
> > > > > IntegrationTest jobs over the past 6 commits), it seems very likely
> > > that
> > > > > had StressNewTest been allowed to run, this issue would have been
> > > caught
> > > > at
> > > > > the PR stage and could have been remedied then, instead of
> resulting
> > > in a
> > > > > nearly constantly red pipeline.
> > > > >
> > > > > I've already given my +1 to this proposal, but this situation has
> > > > prompted
> > > > > me to make it a +2.
> > > > >
> > > > > On Fri, Mar 6, 2020 at 1:46 PM Donal Evans <do...@pivotal.io>
> > wrote:
> > > > >
> > > > >> With fairly apt timing, we've recently had a PR merged (
> > > > >> https://github.com/apache/geode/pull/4745) that introduced a test
> > > that
> > > > >> has resulted in fairly consistently flaky failures in the pipeline
> > > (JIRA
> > > > >> ticket:
> > > https://issues.apache.org/jira/projects/GEODE/issues/GEODE-7843
> > > > ).
> > > > >> The PR was quite large and touched/created a lot of tests, so
> > > > StressNewTest
> > > > >> never ran on it:
> > > > >> https://concourse.apachegeode-ci.info/builds/136784#L5e3ac3fc:2.
> > > Given
> > > > >> how frequently the test is failing in the pipeline (11 failures on
> > > > various
> > > > >> IntegrationTest jobs over the past 6 commits), it seems very
> likely
> > > that
> > > > >> had StressNewTest been allowed to run, this issue would have been
> > > > caught at
> > > > >> the PR stage and could have been remedied then, instead of
> resulting
> > > in
> > > > a
> > > > >> nearly constantly red pipeline.
> > > > >>
> > > > >> I've already given my +1 to this proposal, but this situation has
> > > > prompted
> > > > >> me to make it a *+1*.
> > > > >>
> > > > >> On Tue, Mar 3, 2020 at 2:08 PM Anilkumar Gingade <
> > agingade@pivotal.io
> > > >
> > > > >> wrote:
> > > > >>
> > > > >>> The stress test is to identify the flaky-ness within the test;
> and
> > > > >>> assuming
> > > > >>> any changes to the test may have introduced the flaky-ness.
> > > > >>> It's about paying the cost upfront or later when the test is
> > > > determined to
> > > > >>> be flaky.
> > > > >>> If 25+ tests have been changed in a PR, the cost of running
> stress
> > > test
> > > > >>> for
> > > > >>> those; and gating the PR for so long.
> > > > >>> Knowing how much pain it causes to fix a flaky test after a
> > > > certain/long
> > > > >>> duration of time; I am +1 for doing this change.
> > > > >>>
> > > > >>> On Tue, Mar 3, 2020 at 10:06 AM Dan Smith <ds...@pivotal.io>
> > wrote:
> > > > >>>
> > > > >>>> What's the current timeout for StressNewTest? Maybe if we just
> up
> > > the
> > > > >>>> threshold to 100 tests or so and up the timeout to match we can
> > > catch
> > > > >>>> pretty much all PRs.
> > > > >>>>
> > > > >>>> I'm not sure why the job is flagging more tests than it should.
> It
> > > > looks
> > > > >>>> like at some point @rhoughon changed it to read the merge base
> > from
> > > > some
> > > > >>>> file created by concourse as an optimization [1] - I suspect
> maybe
> > > > that
> > > > >>>> file is inaccurate?
> > > > >>>>
> > > > >>>> I originally wrote this job. It's definitely not a panacea, it
> > will
> > > > only
> > > > >>>> catch a new flaky test if
> > > > >>>> - the test is really flaky (likely to fail more than 1/50 times)
> > > > >>>> - the change actually happened in the test file itself, and not
> > the
> > > > >>>> product or some other test file.
> > > > >>>>
> > > > >>>> [1]
> > > > >>>>
> > > > >>>>
> > > > >>>
> > > >
> > >
> >
> https://github.com/apache/geode/commit/4c06ba4625e69d44a5165aa9f2fccddfc064de87
> > > > >>>>
> > > > >>>> -Dan
> > > > >>>>
> > > > >>>> On Sun, Mar 1, 2020 at 9:00 PM Owen Nichols <
> onichols@pivotal.io>
> > > > >>> wrote:
> > > > >>>>
> > > > >>>>> We don’t tend to look too closely at successful PR checks to
> see
> > > > >>> whether
> > > > >>>>> they actually checked anything at all.
> > > > >>>>>
> > > > >>>>> One example I found is
> > > > >>>>>
> > > > >>>>
> > > > >>>
> > > >
> > >
> >
> https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/5957
> > > > >>>>> <
> > > > >>>>>
> > > > >>>>
> > > > >>>
> > > >
> > >
> >
> https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/5957
> > > > >>>>>> :
> > > > >>>>> 32 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing.
> > > > >>>>>
> > > > >>>>> Here are 92 more examples (url’s omitted for brevity — use the
> > > > example
> > > > >>>>> above as a template and just replace the last 4 digits):
> > > > >>>>> 26 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6243)
> > > > >>>>> 26 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6249)
> > > > >>>>> 26 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6402)
> > > > >>>>> 27 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6262)
> > > > >>>>> 27 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6430)
> > > > >>>>> 27 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6439)
> > > > >>>>> 27 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6449)
> > > > >>>>> 27 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6454)
> > > > >>>>> 27 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6458)
> > > > >>>>> 27 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6459)
> > > > >>>>> 28 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6224)
> > > > >>>>> 28 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6441)
> > > > >>>>> 28 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6448)
> > > > >>>>> 28 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6452)
> > > > >>>>> 29 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6102)
> > > > >>>>> 29 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6177)
> > > > >>>>> 30 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 5939)
> > > > >>>>> 30 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 5940)
> > > > >>>>> 30 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 5949)
> > > > >>>>> 30 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6473)
> > > > >>>>> 31 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 5953)
> > > > >>>>> 31 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6187)
> > > > >>>>> 31 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6470)
> > > > >>>>> 31 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6471)
> > > > >>>>> 31 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6474)
> > > > >>>>> 31 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6475)
> > > > >>>>> 32 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 5958)
> > > > >>>>> 32 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6173)
> > > > >>>>> 32 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6236)
> > > > >>>>> 32 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6237)
> > > > >>>>> 32 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6242)
> > > > >>>>> 33 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6246)
> > > > >>>>> 33 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6248)
> > > > >>>>> 33 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6250)
> > > > >>>>> 33 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6251)
> > > > >>>>> 33 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6254)
> > > > >>>>> 33 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6255)
> > > > >>>>> 34 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6139)
> > > > >>>>> 34 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6141)
> > > > >>>>> 34 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6264)
> > > > >>>>> 34 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6266)
> > > > >>>>> 34 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6271)
> > > > >>>>> 35 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6142)
> > > > >>>>> 35 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6143)
> > > > >>>>> 35 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6277)
> > > > >>>>> 35 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6309)
> > > > >>>>> 35 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6413)
> > > > >>>>> 35 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6414)
> > > > >>>>> 35 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6419)
> > > > >>>>> 35 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6420)
> > > > >>>>> 36 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6144)
> > > > >>>>> 36 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6146)
> > > > >>>>> 36 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6147)
> > > > >>>>> 36 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6310)
> > > > >>>>> 36 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6326)
> > > > >>>>> 36 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6329)
> > > > >>>>> 36 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6330)
> > > > >>>>> 40 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6159)
> > > > >>>>> 40 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6160)
> > > > >>>>> 40 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6188)
> > > > >>>>> 41 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6440)
> > > > >>>>> 41 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6480)
> > > > >>>>> 41 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6483)
> > > > >>>>> 43 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6190)
> > > > >>>>> 44 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 5768)
> > > > >>>>> 44 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 5772)
> > > > >>>>> 46 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6191)
> > > > >>>>> 46 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6218)
> > > > >>>>> 46 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6219)
> > > > >>>>> 46 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6225)
> > > > >>>>> 46 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6244)
> > > > >>>>> 46 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6245)
> > > > >>>>> 48 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6129)
> > > > >>>>> 49 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 5804)
> > > > >>>>> 51 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 5877)
> > > > >>>>> 53 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 5742)
> > > > >>>>> 54 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 5757)
> > > > >>>>> 66 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6232)
> > > > >>>>> 66 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6233)
> > > > >>>>> 66 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6234)
> > > > >>>>> 66 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6259)
> > > > >>>>> 66 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6260)
> > > > >>>>> 66 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6278)
> > > > >>>>> 79 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6481)
> > > > >>>>> 103 is too many changed tests to stress test. Allowing this job
> > to
> > > > >>> pass
> > > > >>>>> without stress testing. (build 6493)
> > > > >>>>> 106 is too many changed tests to stress test. Allowing this job
> > to
> > > > >>> pass
> > > > >>>>> without stress testing. (build 6206)
> > > > >>>>> 106 is too many changed tests to stress test. Allowing this job
> > to
> > > > >>> pass
> > > > >>>>> without stress testing. (build 6207)
> > > > >>>>> 115 is too many changed tests to stress test. Allowing this job
> > to
> > > > >>> pass
> > > > >>>>> without stress testing. (build 5769)
> > > > >>>>> 118 is too many changed tests to stress test. Allowing this job
> > to
> > > > >>> pass
> > > > >>>>> without stress testing. (build 6226)
> > > > >>>>> 721 is too many changed tests to stress test. Allowing this job
> > to
> > > > >>> pass
> > > > >>>>> without stress testing. (build 6197)
> > > > >>>>> 722 is too many changed tests to stress test. Allowing this job
> > to
> > > > >>> pass
> > > > >>>>> without stress testing. (build 6217)
> > > > >>>>> 722 is too many changed tests to stress test. Allowing this job
> > to
> > > > >>> pass
> > > > >>>>> without stress testing. (build 6221)
> > > > >>>>>
> > > > >>>>> Please note, sometimes the number of files reported seems to be
> > way
> > > > >>>> higher
> > > > >>>>> than it should be. For example
> > > > >>>> https://github.com/apache/geode/pull/4691
> > > > >>>>> <https://github.com/apache/geode/pull/4691> shows exactly 1
> test
> > > > file
> > > > >>>>> touched, but
> > > > >>>>>
> > > > >>>>
> > > > >>>
> > > >
> > >
> >
> https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/6278
> > > > >>>>> <
> > > > >>>>>
> > > > >>>>
> > > > >>>
> > > >
> > >
> >
> https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/6278
> > > > >>>>>
> > > > >>>>> thinks it touched 66 test files.
> > > > >>>>>
> > > > >>>>> There’s currently no good data to predict how long StressNew
> will
> > > > >>> take,
> > > > >>>> it
> > > > >>>>> just depends on the mix of tests touched. I am aware of at
> > least 4
> > > > >>> cases
> > > > >>>>> (with less than 25 files) where the timeout was hit. In two of
> > > those
> > > > >>>> cases
> > > > >>>>> we re-ran with a longer timeout, and in two cases the PR author
> > > split
> > > > >>> up
> > > > >>>>> the PR into half a dozen smaller PRs to get around it.
> > > > >>>>>
> > > > >>>>>
> > > > >>>>>> On Mar 1, 2020, at 7:52 PM, Anthony Baker <ab...@pivotal.io>
> > > > >>> wrote:
> > > > >>>>>>
> > > > >>>>>> What percentage of PR’s are currently subject to the 25 test
> > file
> > > > >>> rule?
> > > > >>>>> How many would be subject to the concourse timeout?
> > > > >>>>>>
> > > > >>>>>> I’d like to understand the scope of the impact for this
> change.
> > > > >>>>>>
> > > > >>>>>> Anthony
> > > > >>>>>>
> > > > >>>>>>
> > > > >>>>>>> On Mar 1, 2020, at 8:58 AM, Owen Nichols <
> onichols@pivotal.io>
> > > > >>> wrote:
> > > > >>>>>>>
> > > > >>>>>>> Impossible, no. Inconvenient, perhaps, but a small price to
> pay
> > > for
> > > > >>>>> being
> > > > >>>>>>> able to trust that green means green.
> > > > >>>>>>>
> > > > >>>>>>> With or without this proposed change, if anyone is having
> > trouble
> > > > >>>>> getting
> > > > >>>>>>> their PR to pass StressNew, please bring it up on the dev
> list
> > > and
> > > > >>> we
> > > > >>>>> can
> > > > >>>>>>> discuss the appropriate solution on a case-by-case basis
> (e.g.
> > > > >>>>> increasing
> > > > >>>>>>> timeout, fixing the logic that identifies changed test files,
> > > > >>>> splitting
> > > > >>>>>>> into multiple PRs, authorizing an override, etc).
> > > > >>>>>>>
> > > > >>>>>>> On Sun, Mar 1, 2020 at 8:56 AM Dan Smith <ds...@pivotal.io>
> > > > >>> wrote:
> > > > >>>>>>>
> > > > >>>>>>>> Won't this make it impossible to merge refactoring changes
> > that
> > > > >>> touch
> > > > >>>>> a lot
> > > > >>>>>>>> of tests?
> > > > >>>>>>>>
> > > > >>>>>>>> -Dan
> > > > >>>>>>>>
> > > > >>>>>>>> On Sat, Feb 29, 2020, 12:37 PM Robert Houghton <
> > > > >>> rhoughton@pivotal.io
> > > > >>>>>
> > > > >>>>>>>> wrote:
> > > > >>>>>>>>
> > > > >>>>>>>>> Yes, as it should
> > > > >>>>>>>>>
> > > > >>>>>>>>> On Sat, Feb 29, 2020, 12:25 Dan Smith <ds...@pivotal.io>
> > > wrote:
> > > > >>>>>>>>>
> > > > >>>>>>>>>> Doesn't the build fail when concourse times out?
> > > > >>>>>>>>>
> > > > >>>>>>>>>
> > > > >>>>>>>>
> > > > >>>>>>
> > > > >>>>>
> > > > >>>>>
> > > > >>>>
> > > > >>>
> > > > >>
> > > >
> > > >
> > >
> >
>
Re: [PROPOSAL] eliminate file count loophole in PR StressNewTest
Posted by Owen Nichols <on...@pivotal.io>.
At least one person on the thread (@anthony) raised concerns but has not
replied since.
Also since this thread was started, the bug that miscounted files has been
fixed, which is sufficient that stress tests would have been run for about
95% of PRs that previously (erroneously) were given a free pass. Maybe
that is enough?
I’m hesitant to raise the bar for a required check if it will become an
added hardship, and especially hesitant to create a situation where the
highly-unpopular “manual override” might be the only option.
It might be better to just make slight increases from 25 files/6 hours to
35 files/10 hours, and just ask that if anyone is touching more test files
than that (due to a refactor/rename/reformat) to please make that a
separate PR separate from any actual code changes. This is better for
reviewers too...
Discussion period goes through the end of this week.
On Tue, Mar 17, 2020 at 8:55 AM Dan Smith <ds...@pivotal.io> wrote:
> Seems like I'm the only one on this thread who even quibbled about removing
> the limit entirely. Let's go ahead and remove the limit.
>
> -Dan
>
> On Fri, Mar 13, 2020 at 8:26 AM Robert Houghton <rh...@pivotal.io>
> wrote:
>
> > I want the check to stay required for PR merges to be allowed. I also
> want
> > it to do real work in all cases. If I can add whitespace to 25 test
> classes
> > and get a free pass, thats a big testing hole.
> >
> > Remove the limit. Worst case, we have to A) bump timeouts in the future
> B)
> > put the limits back in C) come up with a new paradigm for verifying test
> > quality in a non-deterministic process
> >
> > On Tue, Mar 10, 2020 at 8:58 PM Dan Smith <ds...@pivotal.io> wrote:
> >
> > > I checked, and 1% of our last 500 commits touched more than 30 tests.
> Of
> > > those 1%, over half touched more than 100 tests. So I'd guess somewhere
> > > around .5% of prs will fall under the proposed changes.
> > >
> > > My preference would be to just raise the threshold given that we
> already
> > > raised the timeout. That'll catch 99% of prs without making test
> > > refactoring and cleanups that touch many tests hard.
> > >
> > > But, I'm ok with removing the limit if people really feel that a manual
> > > override process is better for that 1%.
> > >
> > > -Dan
> > > On Tue, Mar 10, 2020, 4:33 PM Owen Nichols <on...@pivotal.io>
> wrote:
> > >
> > > > To recap, the original question was: should we change StressNewTest
> to
> > > > pass ONLY if it is able to successfully run all new/changed tests 50
> > > times
> > > > (eliminating the existing loophole exempting PRs that touch 25 or
> more
> > > > files)?
> > > >
> > > > So far a handful of people have expressed support, but many have
> > > > questions/concerns:
> > > > - One concern was mis-counting of test files touched. As of today
> this
> > > is
> > > > now fixed.
> > > > - A big concern was that it might become more difficult to pass all
> > > > required PR checks.
> > > > - Another concern was that the current timeout of 6 hours might be
> too
> > > > high / too low.
> > > > - An alternative was suggested to keep the loophole but maybe
> increase
> > > the
> > > > threshold required to get the free pass.
> > > > - If we’re going to raise the bar on any required PR check, maybe we
> > > > should consider making it non-required.
> > > >
> > > > Let’s extend the comment period until the end of next week (Friday
> Mar
> > > 20)
> > > > in hopes of converging on a solution everybody is happy with (even if
> > it
> > > > isn’t what was originally proposed).
> > > >
> > > >
> > > > > On Mar 6, 2020, at 4:51 PM, Donal Evans <do...@pivotal.io>
> wrote:
> > > > >
> > > > > With fairly apt timing, we've recently had a PR merged (
> > > > > https://github.com/apache/geode/pull/4745) that introduced a test
> > that
> > > > has
> > > > > resulted in fairly consistently flaky failures in the pipeline
> (JIRA
> > > > > ticket:
> > > https://issues.apache.org/jira/projects/GEODE/issues/GEODE-7843
> > > > ).
> > > > > The PR was quite large and touched/created a lot of tests, so
> > > > StressNewTest
> > > > > never ran on it:
> > > > > https://concourse.apachegeode-ci.info/builds/136784#L5e3ac3fc:2.
> > Given
> > > > how
> > > > > frequently the test is failing in the pipeline (11 failures on
> > various
> > > > > IntegrationTest jobs over the past 6 commits), it seems very likely
> > > that
> > > > > had StressNewTest been allowed to run, this issue would have been
> > > caught
> > > > at
> > > > > the PR stage and could have been remedied then, instead of
> resulting
> > > in a
> > > > > nearly constantly red pipeline.
> > > > >
> > > > > I've already given my +1 to this proposal, but this situation has
> > > > prompted
> > > > > me to make it a +2.
> > > > >
> > > > > On Fri, Mar 6, 2020 at 1:46 PM Donal Evans <do...@pivotal.io>
> > wrote:
> > > > >
> > > > >> With fairly apt timing, we've recently had a PR merged (
> > > > >> https://github.com/apache/geode/pull/4745) that introduced a test
> > > that
> > > > >> has resulted in fairly consistently flaky failures in the pipeline
> > > (JIRA
> > > > >> ticket:
> > > https://issues.apache.org/jira/projects/GEODE/issues/GEODE-7843
> > > > ).
> > > > >> The PR was quite large and touched/created a lot of tests, so
> > > > StressNewTest
> > > > >> never ran on it:
> > > > >> https://concourse.apachegeode-ci.info/builds/136784#L5e3ac3fc:2.
> > > Given
> > > > >> how frequently the test is failing in the pipeline (11 failures on
> > > > various
> > > > >> IntegrationTest jobs over the past 6 commits), it seems very
> likely
> > > that
> > > > >> had StressNewTest been allowed to run, this issue would have been
> > > > caught at
> > > > >> the PR stage and could have been remedied then, instead of
> resulting
> > > in
> > > > a
> > > > >> nearly constantly red pipeline.
> > > > >>
> > > > >> I've already given my +1 to this proposal, but this situation has
> > > > prompted
> > > > >> me to make it a *+1*.
> > > > >>
> > > > >> On Tue, Mar 3, 2020 at 2:08 PM Anilkumar Gingade <
> > agingade@pivotal.io
> > > >
> > > > >> wrote:
> > > > >>
> > > > >>> The stress test is to identify the flaky-ness within the test;
> and
> > > > >>> assuming
> > > > >>> any changes to the test may have introduced the flaky-ness.
> > > > >>> It's about paying the cost upfront or later when the test is
> > > > determined to
> > > > >>> be flaky.
> > > > >>> If 25+ tests have been changed in a PR, the cost of running
> stress
> > > test
> > > > >>> for
> > > > >>> those; and gating the PR for so long.
> > > > >>> Knowing how much pain it causes to fix a flaky test after a
> > > > certain/long
> > > > >>> duration of time; I am +1 for doing this change.
> > > > >>>
> > > > >>> On Tue, Mar 3, 2020 at 10:06 AM Dan Smith <ds...@pivotal.io>
> > wrote:
> > > > >>>
> > > > >>>> What's the current timeout for StressNewTest? Maybe if we just
> up
> > > the
> > > > >>>> threshold to 100 tests or so and up the timeout to match we can
> > > catch
> > > > >>>> pretty much all PRs.
> > > > >>>>
> > > > >>>> I'm not sure why the job is flagging more tests than it should.
> It
> > > > looks
> > > > >>>> like at some point @rhoughon changed it to read the merge base
> > from
> > > > some
> > > > >>>> file created by concourse as an optimization [1] - I suspect
> maybe
> > > > that
> > > > >>>> file is inaccurate?
> > > > >>>>
> > > > >>>> I originally wrote this job. It's definitely not a panacea, it
> > will
> > > > only
> > > > >>>> catch a new flaky test if
> > > > >>>> - the test is really flaky (likely to fail more than 1/50 times)
> > > > >>>> - the change actually happened in the test file itself, and not
> > the
> > > > >>>> product or some other test file.
> > > > >>>>
> > > > >>>> [1]
> > > > >>>>
> > > > >>>>
> > > > >>>
> > > >
> > >
> >
> https://github.com/apache/geode/commit/4c06ba4625e69d44a5165aa9f2fccddfc064de87
> > > > >>>>
> > > > >>>> -Dan
> > > > >>>>
> > > > >>>> On Sun, Mar 1, 2020 at 9:00 PM Owen Nichols <
> onichols@pivotal.io>
> > > > >>> wrote:
> > > > >>>>
> > > > >>>>> We don’t tend to look too closely at successful PR checks to
> see
> > > > >>> whether
> > > > >>>>> they actually checked anything at all.
> > > > >>>>>
> > > > >>>>> One example I found is
> > > > >>>>>
> > > > >>>>
> > > > >>>
> > > >
> > >
> >
> https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/5957
> > > > >>>>> <
> > > > >>>>>
> > > > >>>>
> > > > >>>
> > > >
> > >
> >
> https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/5957
> > > > >>>>>> :
> > > > >>>>> 32 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing.
> > > > >>>>>
> > > > >>>>> Here are 92 more examples (url’s omitted for brevity — use the
> > > > example
> > > > >>>>> above as a template and just replace the last 4 digits):
> > > > >>>>> 26 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6243)
> > > > >>>>> 26 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6249)
> > > > >>>>> 26 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6402)
> > > > >>>>> 27 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6262)
> > > > >>>>> 27 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6430)
> > > > >>>>> 27 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6439)
> > > > >>>>> 27 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6449)
> > > > >>>>> 27 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6454)
> > > > >>>>> 27 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6458)
> > > > >>>>> 27 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6459)
> > > > >>>>> 28 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6224)
> > > > >>>>> 28 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6441)
> > > > >>>>> 28 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6448)
> > > > >>>>> 28 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6452)
> > > > >>>>> 29 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6102)
> > > > >>>>> 29 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6177)
> > > > >>>>> 30 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 5939)
> > > > >>>>> 30 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 5940)
> > > > >>>>> 30 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 5949)
> > > > >>>>> 30 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6473)
> > > > >>>>> 31 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 5953)
> > > > >>>>> 31 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6187)
> > > > >>>>> 31 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6470)
> > > > >>>>> 31 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6471)
> > > > >>>>> 31 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6474)
> > > > >>>>> 31 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6475)
> > > > >>>>> 32 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 5958)
> > > > >>>>> 32 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6173)
> > > > >>>>> 32 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6236)
> > > > >>>>> 32 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6237)
> > > > >>>>> 32 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6242)
> > > > >>>>> 33 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6246)
> > > > >>>>> 33 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6248)
> > > > >>>>> 33 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6250)
> > > > >>>>> 33 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6251)
> > > > >>>>> 33 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6254)
> > > > >>>>> 33 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6255)
> > > > >>>>> 34 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6139)
> > > > >>>>> 34 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6141)
> > > > >>>>> 34 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6264)
> > > > >>>>> 34 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6266)
> > > > >>>>> 34 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6271)
> > > > >>>>> 35 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6142)
> > > > >>>>> 35 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6143)
> > > > >>>>> 35 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6277)
> > > > >>>>> 35 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6309)
> > > > >>>>> 35 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6413)
> > > > >>>>> 35 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6414)
> > > > >>>>> 35 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6419)
> > > > >>>>> 35 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6420)
> > > > >>>>> 36 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6144)
> > > > >>>>> 36 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6146)
> > > > >>>>> 36 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6147)
> > > > >>>>> 36 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6310)
> > > > >>>>> 36 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6326)
> > > > >>>>> 36 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6329)
> > > > >>>>> 36 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6330)
> > > > >>>>> 40 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6159)
> > > > >>>>> 40 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6160)
> > > > >>>>> 40 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6188)
> > > > >>>>> 41 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6440)
> > > > >>>>> 41 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6480)
> > > > >>>>> 41 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6483)
> > > > >>>>> 43 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6190)
> > > > >>>>> 44 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 5768)
> > > > >>>>> 44 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 5772)
> > > > >>>>> 46 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6191)
> > > > >>>>> 46 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6218)
> > > > >>>>> 46 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6219)
> > > > >>>>> 46 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6225)
> > > > >>>>> 46 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6244)
> > > > >>>>> 46 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6245)
> > > > >>>>> 48 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6129)
> > > > >>>>> 49 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 5804)
> > > > >>>>> 51 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 5877)
> > > > >>>>> 53 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 5742)
> > > > >>>>> 54 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 5757)
> > > > >>>>> 66 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6232)
> > > > >>>>> 66 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6233)
> > > > >>>>> 66 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6234)
> > > > >>>>> 66 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6259)
> > > > >>>>> 66 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6260)
> > > > >>>>> 66 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6278)
> > > > >>>>> 79 is too many changed tests to stress test. Allowing this job
> to
> > > > pass
> > > > >>>>> without stress testing. (build 6481)
> > > > >>>>> 103 is too many changed tests to stress test. Allowing this job
> > to
> > > > >>> pass
> > > > >>>>> without stress testing. (build 6493)
> > > > >>>>> 106 is too many changed tests to stress test. Allowing this job
> > to
> > > > >>> pass
> > > > >>>>> without stress testing. (build 6206)
> > > > >>>>> 106 is too many changed tests to stress test. Allowing this job
> > to
> > > > >>> pass
> > > > >>>>> without stress testing. (build 6207)
> > > > >>>>> 115 is too many changed tests to stress test. Allowing this job
> > to
> > > > >>> pass
> > > > >>>>> without stress testing. (build 5769)
> > > > >>>>> 118 is too many changed tests to stress test. Allowing this job
> > to
> > > > >>> pass
> > > > >>>>> without stress testing. (build 6226)
> > > > >>>>> 721 is too many changed tests to stress test. Allowing this job
> > to
> > > > >>> pass
> > > > >>>>> without stress testing. (build 6197)
> > > > >>>>> 722 is too many changed tests to stress test. Allowing this job
> > to
> > > > >>> pass
> > > > >>>>> without stress testing. (build 6217)
> > > > >>>>> 722 is too many changed tests to stress test. Allowing this job
> > to
> > > > >>> pass
> > > > >>>>> without stress testing. (build 6221)
> > > > >>>>>
> > > > >>>>> Please note, sometimes the number of files reported seems to be
> > way
> > > > >>>> higher
> > > > >>>>> than it should be. For example
> > > > >>>> https://github.com/apache/geode/pull/4691
> > > > >>>>> <https://github.com/apache/geode/pull/4691> shows exactly 1
> test
> > > > file
> > > > >>>>> touched, but
> > > > >>>>>
> > > > >>>>
> > > > >>>
> > > >
> > >
> >
> https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/6278
> > > > >>>>> <
> > > > >>>>>
> > > > >>>>
> > > > >>>
> > > >
> > >
> >
> https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/6278
> > > > >>>>>
> > > > >>>>> thinks it touched 66 test files.
> > > > >>>>>
> > > > >>>>> There’s currently no good data to predict how long StressNew
> will
> > > > >>> take,
> > > > >>>> it
> > > > >>>>> just depends on the mix of tests touched. I am aware of at
> > least 4
> > > > >>> cases
> > > > >>>>> (with less than 25 files) where the timeout was hit. In two of
> > > those
> > > > >>>> cases
> > > > >>>>> we re-ran with a longer timeout, and in two cases the PR author
> > > split
> > > > >>> up
> > > > >>>>> the PR into half a dozen smaller PRs to get around it.
> > > > >>>>>
> > > > >>>>>
> > > > >>>>>> On Mar 1, 2020, at 7:52 PM, Anthony Baker <ab...@pivotal.io>
> > > > >>> wrote:
> > > > >>>>>>
> > > > >>>>>> What percentage of PR’s are currently subject to the 25 test
> > file
> > > > >>> rule?
> > > > >>>>> How many would be subject to the concourse timeout?
> > > > >>>>>>
> > > > >>>>>> I’d like to understand the scope of the impact for this
> change.
> > > > >>>>>>
> > > > >>>>>> Anthony
> > > > >>>>>>
> > > > >>>>>>
> > > > >>>>>>> On Mar 1, 2020, at 8:58 AM, Owen Nichols <
> onichols@pivotal.io>
> > > > >>> wrote:
> > > > >>>>>>>
> > > > >>>>>>> Impossible, no. Inconvenient, perhaps, but a small price to
> pay
> > > for
> > > > >>>>> being
> > > > >>>>>>> able to trust that green means green.
> > > > >>>>>>>
> > > > >>>>>>> With or without this proposed change, if anyone is having
> > trouble
> > > > >>>>> getting
> > > > >>>>>>> their PR to pass StressNew, please bring it up on the dev
> list
> > > and
> > > > >>> we
> > > > >>>>> can
> > > > >>>>>>> discuss the appropriate solution on a case-by-case basis
> (e.g.
> > > > >>>>> increasing
> > > > >>>>>>> timeout, fixing the logic that identifies changed test files,
> > > > >>>> splitting
> > > > >>>>>>> into multiple PRs, authorizing an override, etc).
> > > > >>>>>>>
> > > > >>>>>>> On Sun, Mar 1, 2020 at 8:56 AM Dan Smith <ds...@pivotal.io>
> > > > >>> wrote:
> > > > >>>>>>>
> > > > >>>>>>>> Won't this make it impossible to merge refactoring changes
> > that
> > > > >>> touch
> > > > >>>>> a lot
> > > > >>>>>>>> of tests?
> > > > >>>>>>>>
> > > > >>>>>>>> -Dan
> > > > >>>>>>>>
> > > > >>>>>>>> On Sat, Feb 29, 2020, 12:37 PM Robert Houghton <
> > > > >>> rhoughton@pivotal.io
> > > > >>>>>
> > > > >>>>>>>> wrote:
> > > > >>>>>>>>
> > > > >>>>>>>>> Yes, as it should
> > > > >>>>>>>>>
> > > > >>>>>>>>> On Sat, Feb 29, 2020, 12:25 Dan Smith <ds...@pivotal.io>
> > > wrote:
> > > > >>>>>>>>>
> > > > >>>>>>>>>> Doesn't the build fail when concourse times out?
> > > > >>>>>>>>>
> > > > >>>>>>>>>
> > > > >>>>>>>>
> > > > >>>>>>
> > > > >>>>>
> > > > >>>>>
> > > > >>>>
> > > > >>>
> > > > >>
> > > >
> > > >
> > >
> >
>
Re: [PROPOSAL] eliminate file count loophole in PR StressNewTest
Posted by Dan Smith <ds...@pivotal.io>.
Seems like I'm the only one on this thread who even quibbled about removing
the limit entirely. Let's go ahead and remove the limit.
-Dan
On Fri, Mar 13, 2020 at 8:26 AM Robert Houghton <rh...@pivotal.io>
wrote:
> I want the check to stay required for PR merges to be allowed. I also want
> it to do real work in all cases. If I can add whitespace to 25 test classes
> and get a free pass, thats a big testing hole.
>
> Remove the limit. Worst case, we have to A) bump timeouts in the future B)
> put the limits back in C) come up with a new paradigm for verifying test
> quality in a non-deterministic process
>
> On Tue, Mar 10, 2020 at 8:58 PM Dan Smith <ds...@pivotal.io> wrote:
>
> > I checked, and 1% of our last 500 commits touched more than 30 tests. Of
> > those 1%, over half touched more than 100 tests. So I'd guess somewhere
> > around .5% of prs will fall under the proposed changes.
> >
> > My preference would be to just raise the threshold given that we already
> > raised the timeout. That'll catch 99% of prs without making test
> > refactoring and cleanups that touch many tests hard.
> >
> > But, I'm ok with removing the limit if people really feel that a manual
> > override process is better for that 1%.
> >
> > -Dan
> > On Tue, Mar 10, 2020, 4:33 PM Owen Nichols <on...@pivotal.io> wrote:
> >
> > > To recap, the original question was: should we change StressNewTest to
> > > pass ONLY if it is able to successfully run all new/changed tests 50
> > times
> > > (eliminating the existing loophole exempting PRs that touch 25 or more
> > > files)?
> > >
> > > So far a handful of people have expressed support, but many have
> > > questions/concerns:
> > > - One concern was mis-counting of test files touched. As of today this
> > is
> > > now fixed.
> > > - A big concern was that it might become more difficult to pass all
> > > required PR checks.
> > > - Another concern was that the current timeout of 6 hours might be too
> > > high / too low.
> > > - An alternative was suggested to keep the loophole but maybe increase
> > the
> > > threshold required to get the free pass.
> > > - If we’re going to raise the bar on any required PR check, maybe we
> > > should consider making it non-required.
> > >
> > > Let’s extend the comment period until the end of next week (Friday Mar
> > 20)
> > > in hopes of converging on a solution everybody is happy with (even if
> it
> > > isn’t what was originally proposed).
> > >
> > >
> > > > On Mar 6, 2020, at 4:51 PM, Donal Evans <do...@pivotal.io> wrote:
> > > >
> > > > With fairly apt timing, we've recently had a PR merged (
> > > > https://github.com/apache/geode/pull/4745) that introduced a test
> that
> > > has
> > > > resulted in fairly consistently flaky failures in the pipeline (JIRA
> > > > ticket:
> > https://issues.apache.org/jira/projects/GEODE/issues/GEODE-7843
> > > ).
> > > > The PR was quite large and touched/created a lot of tests, so
> > > StressNewTest
> > > > never ran on it:
> > > > https://concourse.apachegeode-ci.info/builds/136784#L5e3ac3fc:2.
> Given
> > > how
> > > > frequently the test is failing in the pipeline (11 failures on
> various
> > > > IntegrationTest jobs over the past 6 commits), it seems very likely
> > that
> > > > had StressNewTest been allowed to run, this issue would have been
> > caught
> > > at
> > > > the PR stage and could have been remedied then, instead of resulting
> > in a
> > > > nearly constantly red pipeline.
> > > >
> > > > I've already given my +1 to this proposal, but this situation has
> > > prompted
> > > > me to make it a +2.
> > > >
> > > > On Fri, Mar 6, 2020 at 1:46 PM Donal Evans <do...@pivotal.io>
> wrote:
> > > >
> > > >> With fairly apt timing, we've recently had a PR merged (
> > > >> https://github.com/apache/geode/pull/4745) that introduced a test
> > that
> > > >> has resulted in fairly consistently flaky failures in the pipeline
> > (JIRA
> > > >> ticket:
> > https://issues.apache.org/jira/projects/GEODE/issues/GEODE-7843
> > > ).
> > > >> The PR was quite large and touched/created a lot of tests, so
> > > StressNewTest
> > > >> never ran on it:
> > > >> https://concourse.apachegeode-ci.info/builds/136784#L5e3ac3fc:2.
> > Given
> > > >> how frequently the test is failing in the pipeline (11 failures on
> > > various
> > > >> IntegrationTest jobs over the past 6 commits), it seems very likely
> > that
> > > >> had StressNewTest been allowed to run, this issue would have been
> > > caught at
> > > >> the PR stage and could have been remedied then, instead of resulting
> > in
> > > a
> > > >> nearly constantly red pipeline.
> > > >>
> > > >> I've already given my +1 to this proposal, but this situation has
> > > prompted
> > > >> me to make it a *+1*.
> > > >>
> > > >> On Tue, Mar 3, 2020 at 2:08 PM Anilkumar Gingade <
> agingade@pivotal.io
> > >
> > > >> wrote:
> > > >>
> > > >>> The stress test is to identify the flaky-ness within the test; and
> > > >>> assuming
> > > >>> any changes to the test may have introduced the flaky-ness.
> > > >>> It's about paying the cost upfront or later when the test is
> > > determined to
> > > >>> be flaky.
> > > >>> If 25+ tests have been changed in a PR, the cost of running stress
> > test
> > > >>> for
> > > >>> those; and gating the PR for so long.
> > > >>> Knowing how much pain it causes to fix a flaky test after a
> > > certain/long
> > > >>> duration of time; I am +1 for doing this change.
> > > >>>
> > > >>> On Tue, Mar 3, 2020 at 10:06 AM Dan Smith <ds...@pivotal.io>
> wrote:
> > > >>>
> > > >>>> What's the current timeout for StressNewTest? Maybe if we just up
> > the
> > > >>>> threshold to 100 tests or so and up the timeout to match we can
> > catch
> > > >>>> pretty much all PRs.
> > > >>>>
> > > >>>> I'm not sure why the job is flagging more tests than it should. It
> > > looks
> > > >>>> like at some point @rhoughon changed it to read the merge base
> from
> > > some
> > > >>>> file created by concourse as an optimization [1] - I suspect maybe
> > > that
> > > >>>> file is inaccurate?
> > > >>>>
> > > >>>> I originally wrote this job. It's definitely not a panacea, it
> will
> > > only
> > > >>>> catch a new flaky test if
> > > >>>> - the test is really flaky (likely to fail more than 1/50 times)
> > > >>>> - the change actually happened in the test file itself, and not
> the
> > > >>>> product or some other test file.
> > > >>>>
> > > >>>> [1]
> > > >>>>
> > > >>>>
> > > >>>
> > >
> >
> https://github.com/apache/geode/commit/4c06ba4625e69d44a5165aa9f2fccddfc064de87
> > > >>>>
> > > >>>> -Dan
> > > >>>>
> > > >>>> On Sun, Mar 1, 2020 at 9:00 PM Owen Nichols <on...@pivotal.io>
> > > >>> wrote:
> > > >>>>
> > > >>>>> We don’t tend to look too closely at successful PR checks to see
> > > >>> whether
> > > >>>>> they actually checked anything at all.
> > > >>>>>
> > > >>>>> One example I found is
> > > >>>>>
> > > >>>>
> > > >>>
> > >
> >
> https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/5957
> > > >>>>> <
> > > >>>>>
> > > >>>>
> > > >>>
> > >
> >
> https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/5957
> > > >>>>>> :
> > > >>>>> 32 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing.
> > > >>>>>
> > > >>>>> Here are 92 more examples (url’s omitted for brevity — use the
> > > example
> > > >>>>> above as a template and just replace the last 4 digits):
> > > >>>>> 26 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6243)
> > > >>>>> 26 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6249)
> > > >>>>> 26 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6402)
> > > >>>>> 27 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6262)
> > > >>>>> 27 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6430)
> > > >>>>> 27 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6439)
> > > >>>>> 27 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6449)
> > > >>>>> 27 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6454)
> > > >>>>> 27 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6458)
> > > >>>>> 27 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6459)
> > > >>>>> 28 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6224)
> > > >>>>> 28 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6441)
> > > >>>>> 28 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6448)
> > > >>>>> 28 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6452)
> > > >>>>> 29 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6102)
> > > >>>>> 29 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6177)
> > > >>>>> 30 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 5939)
> > > >>>>> 30 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 5940)
> > > >>>>> 30 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 5949)
> > > >>>>> 30 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6473)
> > > >>>>> 31 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 5953)
> > > >>>>> 31 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6187)
> > > >>>>> 31 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6470)
> > > >>>>> 31 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6471)
> > > >>>>> 31 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6474)
> > > >>>>> 31 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6475)
> > > >>>>> 32 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 5958)
> > > >>>>> 32 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6173)
> > > >>>>> 32 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6236)
> > > >>>>> 32 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6237)
> > > >>>>> 32 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6242)
> > > >>>>> 33 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6246)
> > > >>>>> 33 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6248)
> > > >>>>> 33 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6250)
> > > >>>>> 33 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6251)
> > > >>>>> 33 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6254)
> > > >>>>> 33 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6255)
> > > >>>>> 34 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6139)
> > > >>>>> 34 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6141)
> > > >>>>> 34 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6264)
> > > >>>>> 34 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6266)
> > > >>>>> 34 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6271)
> > > >>>>> 35 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6142)
> > > >>>>> 35 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6143)
> > > >>>>> 35 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6277)
> > > >>>>> 35 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6309)
> > > >>>>> 35 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6413)
> > > >>>>> 35 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6414)
> > > >>>>> 35 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6419)
> > > >>>>> 35 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6420)
> > > >>>>> 36 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6144)
> > > >>>>> 36 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6146)
> > > >>>>> 36 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6147)
> > > >>>>> 36 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6310)
> > > >>>>> 36 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6326)
> > > >>>>> 36 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6329)
> > > >>>>> 36 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6330)
> > > >>>>> 40 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6159)
> > > >>>>> 40 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6160)
> > > >>>>> 40 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6188)
> > > >>>>> 41 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6440)
> > > >>>>> 41 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6480)
> > > >>>>> 41 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6483)
> > > >>>>> 43 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6190)
> > > >>>>> 44 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 5768)
> > > >>>>> 44 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 5772)
> > > >>>>> 46 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6191)
> > > >>>>> 46 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6218)
> > > >>>>> 46 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6219)
> > > >>>>> 46 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6225)
> > > >>>>> 46 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6244)
> > > >>>>> 46 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6245)
> > > >>>>> 48 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6129)
> > > >>>>> 49 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 5804)
> > > >>>>> 51 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 5877)
> > > >>>>> 53 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 5742)
> > > >>>>> 54 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 5757)
> > > >>>>> 66 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6232)
> > > >>>>> 66 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6233)
> > > >>>>> 66 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6234)
> > > >>>>> 66 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6259)
> > > >>>>> 66 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6260)
> > > >>>>> 66 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6278)
> > > >>>>> 79 is too many changed tests to stress test. Allowing this job to
> > > pass
> > > >>>>> without stress testing. (build 6481)
> > > >>>>> 103 is too many changed tests to stress test. Allowing this job
> to
> > > >>> pass
> > > >>>>> without stress testing. (build 6493)
> > > >>>>> 106 is too many changed tests to stress test. Allowing this job
> to
> > > >>> pass
> > > >>>>> without stress testing. (build 6206)
> > > >>>>> 106 is too many changed tests to stress test. Allowing this job
> to
> > > >>> pass
> > > >>>>> without stress testing. (build 6207)
> > > >>>>> 115 is too many changed tests to stress test. Allowing this job
> to
> > > >>> pass
> > > >>>>> without stress testing. (build 5769)
> > > >>>>> 118 is too many changed tests to stress test. Allowing this job
> to
> > > >>> pass
> > > >>>>> without stress testing. (build 6226)
> > > >>>>> 721 is too many changed tests to stress test. Allowing this job
> to
> > > >>> pass
> > > >>>>> without stress testing. (build 6197)
> > > >>>>> 722 is too many changed tests to stress test. Allowing this job
> to
> > > >>> pass
> > > >>>>> without stress testing. (build 6217)
> > > >>>>> 722 is too many changed tests to stress test. Allowing this job
> to
> > > >>> pass
> > > >>>>> without stress testing. (build 6221)
> > > >>>>>
> > > >>>>> Please note, sometimes the number of files reported seems to be
> way
> > > >>>> higher
> > > >>>>> than it should be. For example
> > > >>>> https://github.com/apache/geode/pull/4691
> > > >>>>> <https://github.com/apache/geode/pull/4691> shows exactly 1 test
> > > file
> > > >>>>> touched, but
> > > >>>>>
> > > >>>>
> > > >>>
> > >
> >
> https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/6278
> > > >>>>> <
> > > >>>>>
> > > >>>>
> > > >>>
> > >
> >
> https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/6278
> > > >>>>>
> > > >>>>> thinks it touched 66 test files.
> > > >>>>>
> > > >>>>> There’s currently no good data to predict how long StressNew will
> > > >>> take,
> > > >>>> it
> > > >>>>> just depends on the mix of tests touched. I am aware of at
> least 4
> > > >>> cases
> > > >>>>> (with less than 25 files) where the timeout was hit. In two of
> > those
> > > >>>> cases
> > > >>>>> we re-ran with a longer timeout, and in two cases the PR author
> > split
> > > >>> up
> > > >>>>> the PR into half a dozen smaller PRs to get around it.
> > > >>>>>
> > > >>>>>
> > > >>>>>> On Mar 1, 2020, at 7:52 PM, Anthony Baker <ab...@pivotal.io>
> > > >>> wrote:
> > > >>>>>>
> > > >>>>>> What percentage of PR’s are currently subject to the 25 test
> file
> > > >>> rule?
> > > >>>>> How many would be subject to the concourse timeout?
> > > >>>>>>
> > > >>>>>> I’d like to understand the scope of the impact for this change.
> > > >>>>>>
> > > >>>>>> Anthony
> > > >>>>>>
> > > >>>>>>
> > > >>>>>>> On Mar 1, 2020, at 8:58 AM, Owen Nichols <on...@pivotal.io>
> > > >>> wrote:
> > > >>>>>>>
> > > >>>>>>> Impossible, no. Inconvenient, perhaps, but a small price to pay
> > for
> > > >>>>> being
> > > >>>>>>> able to trust that green means green.
> > > >>>>>>>
> > > >>>>>>> With or without this proposed change, if anyone is having
> trouble
> > > >>>>> getting
> > > >>>>>>> their PR to pass StressNew, please bring it up on the dev list
> > and
> > > >>> we
> > > >>>>> can
> > > >>>>>>> discuss the appropriate solution on a case-by-case basis (e.g.
> > > >>>>> increasing
> > > >>>>>>> timeout, fixing the logic that identifies changed test files,
> > > >>>> splitting
> > > >>>>>>> into multiple PRs, authorizing an override, etc).
> > > >>>>>>>
> > > >>>>>>> On Sun, Mar 1, 2020 at 8:56 AM Dan Smith <ds...@pivotal.io>
> > > >>> wrote:
> > > >>>>>>>
> > > >>>>>>>> Won't this make it impossible to merge refactoring changes
> that
> > > >>> touch
> > > >>>>> a lot
> > > >>>>>>>> of tests?
> > > >>>>>>>>
> > > >>>>>>>> -Dan
> > > >>>>>>>>
> > > >>>>>>>> On Sat, Feb 29, 2020, 12:37 PM Robert Houghton <
> > > >>> rhoughton@pivotal.io
> > > >>>>>
> > > >>>>>>>> wrote:
> > > >>>>>>>>
> > > >>>>>>>>> Yes, as it should
> > > >>>>>>>>>
> > > >>>>>>>>> On Sat, Feb 29, 2020, 12:25 Dan Smith <ds...@pivotal.io>
> > wrote:
> > > >>>>>>>>>
> > > >>>>>>>>>> Doesn't the build fail when concourse times out?
> > > >>>>>>>>>
> > > >>>>>>>>>
> > > >>>>>>>>
> > > >>>>>>
> > > >>>>>
> > > >>>>>
> > > >>>>
> > > >>>
> > > >>
> > >
> > >
> >
>
Re: [PROPOSAL] eliminate file count loophole in PR StressNewTest
Posted by Robert Houghton <rh...@pivotal.io>.
I want the check to stay required for PR merges to be allowed. I also want
it to do real work in all cases. If I can add whitespace to 25 test classes
and get a free pass, thats a big testing hole.
Remove the limit. Worst case, we have to A) bump timeouts in the future B)
put the limits back in C) come up with a new paradigm for verifying test
quality in a non-deterministic process
On Tue, Mar 10, 2020 at 8:58 PM Dan Smith <ds...@pivotal.io> wrote:
> I checked, and 1% of our last 500 commits touched more than 30 tests. Of
> those 1%, over half touched more than 100 tests. So I'd guess somewhere
> around .5% of prs will fall under the proposed changes.
>
> My preference would be to just raise the threshold given that we already
> raised the timeout. That'll catch 99% of prs without making test
> refactoring and cleanups that touch many tests hard.
>
> But, I'm ok with removing the limit if people really feel that a manual
> override process is better for that 1%.
>
> -Dan
> On Tue, Mar 10, 2020, 4:33 PM Owen Nichols <on...@pivotal.io> wrote:
>
> > To recap, the original question was: should we change StressNewTest to
> > pass ONLY if it is able to successfully run all new/changed tests 50
> times
> > (eliminating the existing loophole exempting PRs that touch 25 or more
> > files)?
> >
> > So far a handful of people have expressed support, but many have
> > questions/concerns:
> > - One concern was mis-counting of test files touched. As of today this
> is
> > now fixed.
> > - A big concern was that it might become more difficult to pass all
> > required PR checks.
> > - Another concern was that the current timeout of 6 hours might be too
> > high / too low.
> > - An alternative was suggested to keep the loophole but maybe increase
> the
> > threshold required to get the free pass.
> > - If we’re going to raise the bar on any required PR check, maybe we
> > should consider making it non-required.
> >
> > Let’s extend the comment period until the end of next week (Friday Mar
> 20)
> > in hopes of converging on a solution everybody is happy with (even if it
> > isn’t what was originally proposed).
> >
> >
> > > On Mar 6, 2020, at 4:51 PM, Donal Evans <do...@pivotal.io> wrote:
> > >
> > > With fairly apt timing, we've recently had a PR merged (
> > > https://github.com/apache/geode/pull/4745) that introduced a test that
> > has
> > > resulted in fairly consistently flaky failures in the pipeline (JIRA
> > > ticket:
> https://issues.apache.org/jira/projects/GEODE/issues/GEODE-7843
> > ).
> > > The PR was quite large and touched/created a lot of tests, so
> > StressNewTest
> > > never ran on it:
> > > https://concourse.apachegeode-ci.info/builds/136784#L5e3ac3fc:2. Given
> > how
> > > frequently the test is failing in the pipeline (11 failures on various
> > > IntegrationTest jobs over the past 6 commits), it seems very likely
> that
> > > had StressNewTest been allowed to run, this issue would have been
> caught
> > at
> > > the PR stage and could have been remedied then, instead of resulting
> in a
> > > nearly constantly red pipeline.
> > >
> > > I've already given my +1 to this proposal, but this situation has
> > prompted
> > > me to make it a +2.
> > >
> > > On Fri, Mar 6, 2020 at 1:46 PM Donal Evans <do...@pivotal.io> wrote:
> > >
> > >> With fairly apt timing, we've recently had a PR merged (
> > >> https://github.com/apache/geode/pull/4745) that introduced a test
> that
> > >> has resulted in fairly consistently flaky failures in the pipeline
> (JIRA
> > >> ticket:
> https://issues.apache.org/jira/projects/GEODE/issues/GEODE-7843
> > ).
> > >> The PR was quite large and touched/created a lot of tests, so
> > StressNewTest
> > >> never ran on it:
> > >> https://concourse.apachegeode-ci.info/builds/136784#L5e3ac3fc:2.
> Given
> > >> how frequently the test is failing in the pipeline (11 failures on
> > various
> > >> IntegrationTest jobs over the past 6 commits), it seems very likely
> that
> > >> had StressNewTest been allowed to run, this issue would have been
> > caught at
> > >> the PR stage and could have been remedied then, instead of resulting
> in
> > a
> > >> nearly constantly red pipeline.
> > >>
> > >> I've already given my +1 to this proposal, but this situation has
> > prompted
> > >> me to make it a *+1*.
> > >>
> > >> On Tue, Mar 3, 2020 at 2:08 PM Anilkumar Gingade <agingade@pivotal.io
> >
> > >> wrote:
> > >>
> > >>> The stress test is to identify the flaky-ness within the test; and
> > >>> assuming
> > >>> any changes to the test may have introduced the flaky-ness.
> > >>> It's about paying the cost upfront or later when the test is
> > determined to
> > >>> be flaky.
> > >>> If 25+ tests have been changed in a PR, the cost of running stress
> test
> > >>> for
> > >>> those; and gating the PR for so long.
> > >>> Knowing how much pain it causes to fix a flaky test after a
> > certain/long
> > >>> duration of time; I am +1 for doing this change.
> > >>>
> > >>> On Tue, Mar 3, 2020 at 10:06 AM Dan Smith <ds...@pivotal.io> wrote:
> > >>>
> > >>>> What's the current timeout for StressNewTest? Maybe if we just up
> the
> > >>>> threshold to 100 tests or so and up the timeout to match we can
> catch
> > >>>> pretty much all PRs.
> > >>>>
> > >>>> I'm not sure why the job is flagging more tests than it should. It
> > looks
> > >>>> like at some point @rhoughon changed it to read the merge base from
> > some
> > >>>> file created by concourse as an optimization [1] - I suspect maybe
> > that
> > >>>> file is inaccurate?
> > >>>>
> > >>>> I originally wrote this job. It's definitely not a panacea, it will
> > only
> > >>>> catch a new flaky test if
> > >>>> - the test is really flaky (likely to fail more than 1/50 times)
> > >>>> - the change actually happened in the test file itself, and not the
> > >>>> product or some other test file.
> > >>>>
> > >>>> [1]
> > >>>>
> > >>>>
> > >>>
> >
> https://github.com/apache/geode/commit/4c06ba4625e69d44a5165aa9f2fccddfc064de87
> > >>>>
> > >>>> -Dan
> > >>>>
> > >>>> On Sun, Mar 1, 2020 at 9:00 PM Owen Nichols <on...@pivotal.io>
> > >>> wrote:
> > >>>>
> > >>>>> We don’t tend to look too closely at successful PR checks to see
> > >>> whether
> > >>>>> they actually checked anything at all.
> > >>>>>
> > >>>>> One example I found is
> > >>>>>
> > >>>>
> > >>>
> >
> https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/5957
> > >>>>> <
> > >>>>>
> > >>>>
> > >>>
> >
> https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/5957
> > >>>>>> :
> > >>>>> 32 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing.
> > >>>>>
> > >>>>> Here are 92 more examples (url’s omitted for brevity — use the
> > example
> > >>>>> above as a template and just replace the last 4 digits):
> > >>>>> 26 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6243)
> > >>>>> 26 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6249)
> > >>>>> 26 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6402)
> > >>>>> 27 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6262)
> > >>>>> 27 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6430)
> > >>>>> 27 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6439)
> > >>>>> 27 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6449)
> > >>>>> 27 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6454)
> > >>>>> 27 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6458)
> > >>>>> 27 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6459)
> > >>>>> 28 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6224)
> > >>>>> 28 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6441)
> > >>>>> 28 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6448)
> > >>>>> 28 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6452)
> > >>>>> 29 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6102)
> > >>>>> 29 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6177)
> > >>>>> 30 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 5939)
> > >>>>> 30 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 5940)
> > >>>>> 30 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 5949)
> > >>>>> 30 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6473)
> > >>>>> 31 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 5953)
> > >>>>> 31 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6187)
> > >>>>> 31 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6470)
> > >>>>> 31 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6471)
> > >>>>> 31 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6474)
> > >>>>> 31 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6475)
> > >>>>> 32 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 5958)
> > >>>>> 32 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6173)
> > >>>>> 32 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6236)
> > >>>>> 32 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6237)
> > >>>>> 32 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6242)
> > >>>>> 33 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6246)
> > >>>>> 33 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6248)
> > >>>>> 33 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6250)
> > >>>>> 33 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6251)
> > >>>>> 33 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6254)
> > >>>>> 33 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6255)
> > >>>>> 34 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6139)
> > >>>>> 34 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6141)
> > >>>>> 34 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6264)
> > >>>>> 34 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6266)
> > >>>>> 34 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6271)
> > >>>>> 35 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6142)
> > >>>>> 35 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6143)
> > >>>>> 35 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6277)
> > >>>>> 35 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6309)
> > >>>>> 35 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6413)
> > >>>>> 35 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6414)
> > >>>>> 35 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6419)
> > >>>>> 35 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6420)
> > >>>>> 36 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6144)
> > >>>>> 36 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6146)
> > >>>>> 36 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6147)
> > >>>>> 36 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6310)
> > >>>>> 36 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6326)
> > >>>>> 36 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6329)
> > >>>>> 36 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6330)
> > >>>>> 40 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6159)
> > >>>>> 40 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6160)
> > >>>>> 40 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6188)
> > >>>>> 41 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6440)
> > >>>>> 41 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6480)
> > >>>>> 41 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6483)
> > >>>>> 43 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6190)
> > >>>>> 44 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 5768)
> > >>>>> 44 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 5772)
> > >>>>> 46 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6191)
> > >>>>> 46 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6218)
> > >>>>> 46 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6219)
> > >>>>> 46 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6225)
> > >>>>> 46 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6244)
> > >>>>> 46 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6245)
> > >>>>> 48 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6129)
> > >>>>> 49 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 5804)
> > >>>>> 51 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 5877)
> > >>>>> 53 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 5742)
> > >>>>> 54 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 5757)
> > >>>>> 66 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6232)
> > >>>>> 66 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6233)
> > >>>>> 66 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6234)
> > >>>>> 66 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6259)
> > >>>>> 66 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6260)
> > >>>>> 66 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6278)
> > >>>>> 79 is too many changed tests to stress test. Allowing this job to
> > pass
> > >>>>> without stress testing. (build 6481)
> > >>>>> 103 is too many changed tests to stress test. Allowing this job to
> > >>> pass
> > >>>>> without stress testing. (build 6493)
> > >>>>> 106 is too many changed tests to stress test. Allowing this job to
> > >>> pass
> > >>>>> without stress testing. (build 6206)
> > >>>>> 106 is too many changed tests to stress test. Allowing this job to
> > >>> pass
> > >>>>> without stress testing. (build 6207)
> > >>>>> 115 is too many changed tests to stress test. Allowing this job to
> > >>> pass
> > >>>>> without stress testing. (build 5769)
> > >>>>> 118 is too many changed tests to stress test. Allowing this job to
> > >>> pass
> > >>>>> without stress testing. (build 6226)
> > >>>>> 721 is too many changed tests to stress test. Allowing this job to
> > >>> pass
> > >>>>> without stress testing. (build 6197)
> > >>>>> 722 is too many changed tests to stress test. Allowing this job to
> > >>> pass
> > >>>>> without stress testing. (build 6217)
> > >>>>> 722 is too many changed tests to stress test. Allowing this job to
> > >>> pass
> > >>>>> without stress testing. (build 6221)
> > >>>>>
> > >>>>> Please note, sometimes the number of files reported seems to be way
> > >>>> higher
> > >>>>> than it should be. For example
> > >>>> https://github.com/apache/geode/pull/4691
> > >>>>> <https://github.com/apache/geode/pull/4691> shows exactly 1 test
> > file
> > >>>>> touched, but
> > >>>>>
> > >>>>
> > >>>
> >
> https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/6278
> > >>>>> <
> > >>>>>
> > >>>>
> > >>>
> >
> https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/6278
> > >>>>>
> > >>>>> thinks it touched 66 test files.
> > >>>>>
> > >>>>> There’s currently no good data to predict how long StressNew will
> > >>> take,
> > >>>> it
> > >>>>> just depends on the mix of tests touched. I am aware of at least 4
> > >>> cases
> > >>>>> (with less than 25 files) where the timeout was hit. In two of
> those
> > >>>> cases
> > >>>>> we re-ran with a longer timeout, and in two cases the PR author
> split
> > >>> up
> > >>>>> the PR into half a dozen smaller PRs to get around it.
> > >>>>>
> > >>>>>
> > >>>>>> On Mar 1, 2020, at 7:52 PM, Anthony Baker <ab...@pivotal.io>
> > >>> wrote:
> > >>>>>>
> > >>>>>> What percentage of PR’s are currently subject to the 25 test file
> > >>> rule?
> > >>>>> How many would be subject to the concourse timeout?
> > >>>>>>
> > >>>>>> I’d like to understand the scope of the impact for this change.
> > >>>>>>
> > >>>>>> Anthony
> > >>>>>>
> > >>>>>>
> > >>>>>>> On Mar 1, 2020, at 8:58 AM, Owen Nichols <on...@pivotal.io>
> > >>> wrote:
> > >>>>>>>
> > >>>>>>> Impossible, no. Inconvenient, perhaps, but a small price to pay
> for
> > >>>>> being
> > >>>>>>> able to trust that green means green.
> > >>>>>>>
> > >>>>>>> With or without this proposed change, if anyone is having trouble
> > >>>>> getting
> > >>>>>>> their PR to pass StressNew, please bring it up on the dev list
> and
> > >>> we
> > >>>>> can
> > >>>>>>> discuss the appropriate solution on a case-by-case basis (e.g.
> > >>>>> increasing
> > >>>>>>> timeout, fixing the logic that identifies changed test files,
> > >>>> splitting
> > >>>>>>> into multiple PRs, authorizing an override, etc).
> > >>>>>>>
> > >>>>>>> On Sun, Mar 1, 2020 at 8:56 AM Dan Smith <ds...@pivotal.io>
> > >>> wrote:
> > >>>>>>>
> > >>>>>>>> Won't this make it impossible to merge refactoring changes that
> > >>> touch
> > >>>>> a lot
> > >>>>>>>> of tests?
> > >>>>>>>>
> > >>>>>>>> -Dan
> > >>>>>>>>
> > >>>>>>>> On Sat, Feb 29, 2020, 12:37 PM Robert Houghton <
> > >>> rhoughton@pivotal.io
> > >>>>>
> > >>>>>>>> wrote:
> > >>>>>>>>
> > >>>>>>>>> Yes, as it should
> > >>>>>>>>>
> > >>>>>>>>> On Sat, Feb 29, 2020, 12:25 Dan Smith <ds...@pivotal.io>
> wrote:
> > >>>>>>>>>
> > >>>>>>>>>> Doesn't the build fail when concourse times out?
> > >>>>>>>>>
> > >>>>>>>>>
> > >>>>>>>>
> > >>>>>>
> > >>>>>
> > >>>>>
> > >>>>
> > >>>
> > >>
> >
> >
>
Re: [PROPOSAL] eliminate file count loophole in PR StressNewTest
Posted by Dan Smith <ds...@pivotal.io>.
I checked, and 1% of our last 500 commits touched more than 30 tests. Of
those 1%, over half touched more than 100 tests. So I'd guess somewhere
around .5% of prs will fall under the proposed changes.
My preference would be to just raise the threshold given that we already
raised the timeout. That'll catch 99% of prs without making test
refactoring and cleanups that touch many tests hard.
But, I'm ok with removing the limit if people really feel that a manual
override process is better for that 1%.
-Dan
On Tue, Mar 10, 2020, 4:33 PM Owen Nichols <on...@pivotal.io> wrote:
> To recap, the original question was: should we change StressNewTest to
> pass ONLY if it is able to successfully run all new/changed tests 50 times
> (eliminating the existing loophole exempting PRs that touch 25 or more
> files)?
>
> So far a handful of people have expressed support, but many have
> questions/concerns:
> - One concern was mis-counting of test files touched. As of today this is
> now fixed.
> - A big concern was that it might become more difficult to pass all
> required PR checks.
> - Another concern was that the current timeout of 6 hours might be too
> high / too low.
> - An alternative was suggested to keep the loophole but maybe increase the
> threshold required to get the free pass.
> - If we’re going to raise the bar on any required PR check, maybe we
> should consider making it non-required.
>
> Let’s extend the comment period until the end of next week (Friday Mar 20)
> in hopes of converging on a solution everybody is happy with (even if it
> isn’t what was originally proposed).
>
>
> > On Mar 6, 2020, at 4:51 PM, Donal Evans <do...@pivotal.io> wrote:
> >
> > With fairly apt timing, we've recently had a PR merged (
> > https://github.com/apache/geode/pull/4745) that introduced a test that
> has
> > resulted in fairly consistently flaky failures in the pipeline (JIRA
> > ticket: https://issues.apache.org/jira/projects/GEODE/issues/GEODE-7843
> ).
> > The PR was quite large and touched/created a lot of tests, so
> StressNewTest
> > never ran on it:
> > https://concourse.apachegeode-ci.info/builds/136784#L5e3ac3fc:2. Given
> how
> > frequently the test is failing in the pipeline (11 failures on various
> > IntegrationTest jobs over the past 6 commits), it seems very likely that
> > had StressNewTest been allowed to run, this issue would have been caught
> at
> > the PR stage and could have been remedied then, instead of resulting in a
> > nearly constantly red pipeline.
> >
> > I've already given my +1 to this proposal, but this situation has
> prompted
> > me to make it a +2.
> >
> > On Fri, Mar 6, 2020 at 1:46 PM Donal Evans <do...@pivotal.io> wrote:
> >
> >> With fairly apt timing, we've recently had a PR merged (
> >> https://github.com/apache/geode/pull/4745) that introduced a test that
> >> has resulted in fairly consistently flaky failures in the pipeline (JIRA
> >> ticket: https://issues.apache.org/jira/projects/GEODE/issues/GEODE-7843
> ).
> >> The PR was quite large and touched/created a lot of tests, so
> StressNewTest
> >> never ran on it:
> >> https://concourse.apachegeode-ci.info/builds/136784#L5e3ac3fc:2. Given
> >> how frequently the test is failing in the pipeline (11 failures on
> various
> >> IntegrationTest jobs over the past 6 commits), it seems very likely that
> >> had StressNewTest been allowed to run, this issue would have been
> caught at
> >> the PR stage and could have been remedied then, instead of resulting in
> a
> >> nearly constantly red pipeline.
> >>
> >> I've already given my +1 to this proposal, but this situation has
> prompted
> >> me to make it a *+1*.
> >>
> >> On Tue, Mar 3, 2020 at 2:08 PM Anilkumar Gingade <ag...@pivotal.io>
> >> wrote:
> >>
> >>> The stress test is to identify the flaky-ness within the test; and
> >>> assuming
> >>> any changes to the test may have introduced the flaky-ness.
> >>> It's about paying the cost upfront or later when the test is
> determined to
> >>> be flaky.
> >>> If 25+ tests have been changed in a PR, the cost of running stress test
> >>> for
> >>> those; and gating the PR for so long.
> >>> Knowing how much pain it causes to fix a flaky test after a
> certain/long
> >>> duration of time; I am +1 for doing this change.
> >>>
> >>> On Tue, Mar 3, 2020 at 10:06 AM Dan Smith <ds...@pivotal.io> wrote:
> >>>
> >>>> What's the current timeout for StressNewTest? Maybe if we just up the
> >>>> threshold to 100 tests or so and up the timeout to match we can catch
> >>>> pretty much all PRs.
> >>>>
> >>>> I'm not sure why the job is flagging more tests than it should. It
> looks
> >>>> like at some point @rhoughon changed it to read the merge base from
> some
> >>>> file created by concourse as an optimization [1] - I suspect maybe
> that
> >>>> file is inaccurate?
> >>>>
> >>>> I originally wrote this job. It's definitely not a panacea, it will
> only
> >>>> catch a new flaky test if
> >>>> - the test is really flaky (likely to fail more than 1/50 times)
> >>>> - the change actually happened in the test file itself, and not the
> >>>> product or some other test file.
> >>>>
> >>>> [1]
> >>>>
> >>>>
> >>>
> https://github.com/apache/geode/commit/4c06ba4625e69d44a5165aa9f2fccddfc064de87
> >>>>
> >>>> -Dan
> >>>>
> >>>> On Sun, Mar 1, 2020 at 9:00 PM Owen Nichols <on...@pivotal.io>
> >>> wrote:
> >>>>
> >>>>> We don’t tend to look too closely at successful PR checks to see
> >>> whether
> >>>>> they actually checked anything at all.
> >>>>>
> >>>>> One example I found is
> >>>>>
> >>>>
> >>>
> https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/5957
> >>>>> <
> >>>>>
> >>>>
> >>>
> https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/5957
> >>>>>> :
> >>>>> 32 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing.
> >>>>>
> >>>>> Here are 92 more examples (url’s omitted for brevity — use the
> example
> >>>>> above as a template and just replace the last 4 digits):
> >>>>> 26 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6243)
> >>>>> 26 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6249)
> >>>>> 26 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6402)
> >>>>> 27 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6262)
> >>>>> 27 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6430)
> >>>>> 27 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6439)
> >>>>> 27 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6449)
> >>>>> 27 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6454)
> >>>>> 27 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6458)
> >>>>> 27 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6459)
> >>>>> 28 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6224)
> >>>>> 28 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6441)
> >>>>> 28 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6448)
> >>>>> 28 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6452)
> >>>>> 29 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6102)
> >>>>> 29 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6177)
> >>>>> 30 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 5939)
> >>>>> 30 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 5940)
> >>>>> 30 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 5949)
> >>>>> 30 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6473)
> >>>>> 31 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 5953)
> >>>>> 31 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6187)
> >>>>> 31 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6470)
> >>>>> 31 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6471)
> >>>>> 31 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6474)
> >>>>> 31 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6475)
> >>>>> 32 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 5958)
> >>>>> 32 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6173)
> >>>>> 32 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6236)
> >>>>> 32 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6237)
> >>>>> 32 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6242)
> >>>>> 33 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6246)
> >>>>> 33 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6248)
> >>>>> 33 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6250)
> >>>>> 33 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6251)
> >>>>> 33 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6254)
> >>>>> 33 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6255)
> >>>>> 34 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6139)
> >>>>> 34 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6141)
> >>>>> 34 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6264)
> >>>>> 34 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6266)
> >>>>> 34 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6271)
> >>>>> 35 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6142)
> >>>>> 35 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6143)
> >>>>> 35 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6277)
> >>>>> 35 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6309)
> >>>>> 35 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6413)
> >>>>> 35 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6414)
> >>>>> 35 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6419)
> >>>>> 35 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6420)
> >>>>> 36 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6144)
> >>>>> 36 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6146)
> >>>>> 36 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6147)
> >>>>> 36 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6310)
> >>>>> 36 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6326)
> >>>>> 36 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6329)
> >>>>> 36 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6330)
> >>>>> 40 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6159)
> >>>>> 40 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6160)
> >>>>> 40 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6188)
> >>>>> 41 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6440)
> >>>>> 41 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6480)
> >>>>> 41 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6483)
> >>>>> 43 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6190)
> >>>>> 44 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 5768)
> >>>>> 44 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 5772)
> >>>>> 46 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6191)
> >>>>> 46 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6218)
> >>>>> 46 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6219)
> >>>>> 46 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6225)
> >>>>> 46 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6244)
> >>>>> 46 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6245)
> >>>>> 48 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6129)
> >>>>> 49 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 5804)
> >>>>> 51 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 5877)
> >>>>> 53 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 5742)
> >>>>> 54 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 5757)
> >>>>> 66 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6232)
> >>>>> 66 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6233)
> >>>>> 66 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6234)
> >>>>> 66 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6259)
> >>>>> 66 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6260)
> >>>>> 66 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6278)
> >>>>> 79 is too many changed tests to stress test. Allowing this job to
> pass
> >>>>> without stress testing. (build 6481)
> >>>>> 103 is too many changed tests to stress test. Allowing this job to
> >>> pass
> >>>>> without stress testing. (build 6493)
> >>>>> 106 is too many changed tests to stress test. Allowing this job to
> >>> pass
> >>>>> without stress testing. (build 6206)
> >>>>> 106 is too many changed tests to stress test. Allowing this job to
> >>> pass
> >>>>> without stress testing. (build 6207)
> >>>>> 115 is too many changed tests to stress test. Allowing this job to
> >>> pass
> >>>>> without stress testing. (build 5769)
> >>>>> 118 is too many changed tests to stress test. Allowing this job to
> >>> pass
> >>>>> without stress testing. (build 6226)
> >>>>> 721 is too many changed tests to stress test. Allowing this job to
> >>> pass
> >>>>> without stress testing. (build 6197)
> >>>>> 722 is too many changed tests to stress test. Allowing this job to
> >>> pass
> >>>>> without stress testing. (build 6217)
> >>>>> 722 is too many changed tests to stress test. Allowing this job to
> >>> pass
> >>>>> without stress testing. (build 6221)
> >>>>>
> >>>>> Please note, sometimes the number of files reported seems to be way
> >>>> higher
> >>>>> than it should be. For example
> >>>> https://github.com/apache/geode/pull/4691
> >>>>> <https://github.com/apache/geode/pull/4691> shows exactly 1 test
> file
> >>>>> touched, but
> >>>>>
> >>>>
> >>>
> https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/6278
> >>>>> <
> >>>>>
> >>>>
> >>>
> https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/6278
> >>>>>
> >>>>> thinks it touched 66 test files.
> >>>>>
> >>>>> There’s currently no good data to predict how long StressNew will
> >>> take,
> >>>> it
> >>>>> just depends on the mix of tests touched. I am aware of at least 4
> >>> cases
> >>>>> (with less than 25 files) where the timeout was hit. In two of those
> >>>> cases
> >>>>> we re-ran with a longer timeout, and in two cases the PR author split
> >>> up
> >>>>> the PR into half a dozen smaller PRs to get around it.
> >>>>>
> >>>>>
> >>>>>> On Mar 1, 2020, at 7:52 PM, Anthony Baker <ab...@pivotal.io>
> >>> wrote:
> >>>>>>
> >>>>>> What percentage of PR’s are currently subject to the 25 test file
> >>> rule?
> >>>>> How many would be subject to the concourse timeout?
> >>>>>>
> >>>>>> I’d like to understand the scope of the impact for this change.
> >>>>>>
> >>>>>> Anthony
> >>>>>>
> >>>>>>
> >>>>>>> On Mar 1, 2020, at 8:58 AM, Owen Nichols <on...@pivotal.io>
> >>> wrote:
> >>>>>>>
> >>>>>>> Impossible, no. Inconvenient, perhaps, but a small price to pay for
> >>>>> being
> >>>>>>> able to trust that green means green.
> >>>>>>>
> >>>>>>> With or without this proposed change, if anyone is having trouble
> >>>>> getting
> >>>>>>> their PR to pass StressNew, please bring it up on the dev list and
> >>> we
> >>>>> can
> >>>>>>> discuss the appropriate solution on a case-by-case basis (e.g.
> >>>>> increasing
> >>>>>>> timeout, fixing the logic that identifies changed test files,
> >>>> splitting
> >>>>>>> into multiple PRs, authorizing an override, etc).
> >>>>>>>
> >>>>>>> On Sun, Mar 1, 2020 at 8:56 AM Dan Smith <ds...@pivotal.io>
> >>> wrote:
> >>>>>>>
> >>>>>>>> Won't this make it impossible to merge refactoring changes that
> >>> touch
> >>>>> a lot
> >>>>>>>> of tests?
> >>>>>>>>
> >>>>>>>> -Dan
> >>>>>>>>
> >>>>>>>> On Sat, Feb 29, 2020, 12:37 PM Robert Houghton <
> >>> rhoughton@pivotal.io
> >>>>>
> >>>>>>>> wrote:
> >>>>>>>>
> >>>>>>>>> Yes, as it should
> >>>>>>>>>
> >>>>>>>>> On Sat, Feb 29, 2020, 12:25 Dan Smith <ds...@pivotal.io> wrote:
> >>>>>>>>>
> >>>>>>>>>> Doesn't the build fail when concourse times out?
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>
> >>>>>>
> >>>>>
> >>>>>
> >>>>
> >>>
> >>
>
>
Re: [PROPOSAL] eliminate file count loophole in PR StressNewTest
Posted by Owen Nichols <on...@pivotal.io>.
To recap, the original question was: should we change StressNewTest to pass ONLY if it is able to successfully run all new/changed tests 50 times (eliminating the existing loophole exempting PRs that touch 25 or more files)?
So far a handful of people have expressed support, but many have questions/concerns:
- One concern was mis-counting of test files touched. As of today this is now fixed.
- A big concern was that it might become more difficult to pass all required PR checks.
- Another concern was that the current timeout of 6 hours might be too high / too low.
- An alternative was suggested to keep the loophole but maybe increase the threshold required to get the free pass.
- If we’re going to raise the bar on any required PR check, maybe we should consider making it non-required.
Let’s extend the comment period until the end of next week (Friday Mar 20) in hopes of converging on a solution everybody is happy with (even if it isn’t what was originally proposed).
> On Mar 6, 2020, at 4:51 PM, Donal Evans <do...@pivotal.io> wrote:
>
> With fairly apt timing, we've recently had a PR merged (
> https://github.com/apache/geode/pull/4745) that introduced a test that has
> resulted in fairly consistently flaky failures in the pipeline (JIRA
> ticket: https://issues.apache.org/jira/projects/GEODE/issues/GEODE-7843).
> The PR was quite large and touched/created a lot of tests, so StressNewTest
> never ran on it:
> https://concourse.apachegeode-ci.info/builds/136784#L5e3ac3fc:2. Given how
> frequently the test is failing in the pipeline (11 failures on various
> IntegrationTest jobs over the past 6 commits), it seems very likely that
> had StressNewTest been allowed to run, this issue would have been caught at
> the PR stage and could have been remedied then, instead of resulting in a
> nearly constantly red pipeline.
>
> I've already given my +1 to this proposal, but this situation has prompted
> me to make it a +2.
>
> On Fri, Mar 6, 2020 at 1:46 PM Donal Evans <do...@pivotal.io> wrote:
>
>> With fairly apt timing, we've recently had a PR merged (
>> https://github.com/apache/geode/pull/4745) that introduced a test that
>> has resulted in fairly consistently flaky failures in the pipeline (JIRA
>> ticket: https://issues.apache.org/jira/projects/GEODE/issues/GEODE-7843).
>> The PR was quite large and touched/created a lot of tests, so StressNewTest
>> never ran on it:
>> https://concourse.apachegeode-ci.info/builds/136784#L5e3ac3fc:2. Given
>> how frequently the test is failing in the pipeline (11 failures on various
>> IntegrationTest jobs over the past 6 commits), it seems very likely that
>> had StressNewTest been allowed to run, this issue would have been caught at
>> the PR stage and could have been remedied then, instead of resulting in a
>> nearly constantly red pipeline.
>>
>> I've already given my +1 to this proposal, but this situation has prompted
>> me to make it a *+1*.
>>
>> On Tue, Mar 3, 2020 at 2:08 PM Anilkumar Gingade <ag...@pivotal.io>
>> wrote:
>>
>>> The stress test is to identify the flaky-ness within the test; and
>>> assuming
>>> any changes to the test may have introduced the flaky-ness.
>>> It's about paying the cost upfront or later when the test is determined to
>>> be flaky.
>>> If 25+ tests have been changed in a PR, the cost of running stress test
>>> for
>>> those; and gating the PR for so long.
>>> Knowing how much pain it causes to fix a flaky test after a certain/long
>>> duration of time; I am +1 for doing this change.
>>>
>>> On Tue, Mar 3, 2020 at 10:06 AM Dan Smith <ds...@pivotal.io> wrote:
>>>
>>>> What's the current timeout for StressNewTest? Maybe if we just up the
>>>> threshold to 100 tests or so and up the timeout to match we can catch
>>>> pretty much all PRs.
>>>>
>>>> I'm not sure why the job is flagging more tests than it should. It looks
>>>> like at some point @rhoughon changed it to read the merge base from some
>>>> file created by concourse as an optimization [1] - I suspect maybe that
>>>> file is inaccurate?
>>>>
>>>> I originally wrote this job. It's definitely not a panacea, it will only
>>>> catch a new flaky test if
>>>> - the test is really flaky (likely to fail more than 1/50 times)
>>>> - the change actually happened in the test file itself, and not the
>>>> product or some other test file.
>>>>
>>>> [1]
>>>>
>>>>
>>> https://github.com/apache/geode/commit/4c06ba4625e69d44a5165aa9f2fccddfc064de87
>>>>
>>>> -Dan
>>>>
>>>> On Sun, Mar 1, 2020 at 9:00 PM Owen Nichols <on...@pivotal.io>
>>> wrote:
>>>>
>>>>> We don’t tend to look too closely at successful PR checks to see
>>> whether
>>>>> they actually checked anything at all.
>>>>>
>>>>> One example I found is
>>>>>
>>>>
>>> https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/5957
>>>>> <
>>>>>
>>>>
>>> https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/5957
>>>>>> :
>>>>> 32 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing.
>>>>>
>>>>> Here are 92 more examples (url’s omitted for brevity — use the example
>>>>> above as a template and just replace the last 4 digits):
>>>>> 26 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6243)
>>>>> 26 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6249)
>>>>> 26 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6402)
>>>>> 27 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6262)
>>>>> 27 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6430)
>>>>> 27 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6439)
>>>>> 27 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6449)
>>>>> 27 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6454)
>>>>> 27 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6458)
>>>>> 27 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6459)
>>>>> 28 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6224)
>>>>> 28 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6441)
>>>>> 28 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6448)
>>>>> 28 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6452)
>>>>> 29 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6102)
>>>>> 29 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6177)
>>>>> 30 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 5939)
>>>>> 30 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 5940)
>>>>> 30 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 5949)
>>>>> 30 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6473)
>>>>> 31 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 5953)
>>>>> 31 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6187)
>>>>> 31 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6470)
>>>>> 31 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6471)
>>>>> 31 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6474)
>>>>> 31 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6475)
>>>>> 32 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 5958)
>>>>> 32 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6173)
>>>>> 32 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6236)
>>>>> 32 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6237)
>>>>> 32 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6242)
>>>>> 33 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6246)
>>>>> 33 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6248)
>>>>> 33 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6250)
>>>>> 33 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6251)
>>>>> 33 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6254)
>>>>> 33 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6255)
>>>>> 34 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6139)
>>>>> 34 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6141)
>>>>> 34 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6264)
>>>>> 34 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6266)
>>>>> 34 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6271)
>>>>> 35 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6142)
>>>>> 35 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6143)
>>>>> 35 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6277)
>>>>> 35 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6309)
>>>>> 35 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6413)
>>>>> 35 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6414)
>>>>> 35 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6419)
>>>>> 35 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6420)
>>>>> 36 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6144)
>>>>> 36 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6146)
>>>>> 36 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6147)
>>>>> 36 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6310)
>>>>> 36 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6326)
>>>>> 36 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6329)
>>>>> 36 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6330)
>>>>> 40 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6159)
>>>>> 40 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6160)
>>>>> 40 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6188)
>>>>> 41 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6440)
>>>>> 41 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6480)
>>>>> 41 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6483)
>>>>> 43 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6190)
>>>>> 44 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 5768)
>>>>> 44 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 5772)
>>>>> 46 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6191)
>>>>> 46 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6218)
>>>>> 46 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6219)
>>>>> 46 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6225)
>>>>> 46 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6244)
>>>>> 46 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6245)
>>>>> 48 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6129)
>>>>> 49 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 5804)
>>>>> 51 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 5877)
>>>>> 53 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 5742)
>>>>> 54 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 5757)
>>>>> 66 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6232)
>>>>> 66 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6233)
>>>>> 66 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6234)
>>>>> 66 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6259)
>>>>> 66 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6260)
>>>>> 66 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6278)
>>>>> 79 is too many changed tests to stress test. Allowing this job to pass
>>>>> without stress testing. (build 6481)
>>>>> 103 is too many changed tests to stress test. Allowing this job to
>>> pass
>>>>> without stress testing. (build 6493)
>>>>> 106 is too many changed tests to stress test. Allowing this job to
>>> pass
>>>>> without stress testing. (build 6206)
>>>>> 106 is too many changed tests to stress test. Allowing this job to
>>> pass
>>>>> without stress testing. (build 6207)
>>>>> 115 is too many changed tests to stress test. Allowing this job to
>>> pass
>>>>> without stress testing. (build 5769)
>>>>> 118 is too many changed tests to stress test. Allowing this job to
>>> pass
>>>>> without stress testing. (build 6226)
>>>>> 721 is too many changed tests to stress test. Allowing this job to
>>> pass
>>>>> without stress testing. (build 6197)
>>>>> 722 is too many changed tests to stress test. Allowing this job to
>>> pass
>>>>> without stress testing. (build 6217)
>>>>> 722 is too many changed tests to stress test. Allowing this job to
>>> pass
>>>>> without stress testing. (build 6221)
>>>>>
>>>>> Please note, sometimes the number of files reported seems to be way
>>>> higher
>>>>> than it should be. For example
>>>> https://github.com/apache/geode/pull/4691
>>>>> <https://github.com/apache/geode/pull/4691> shows exactly 1 test file
>>>>> touched, but
>>>>>
>>>>
>>> https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/6278
>>>>> <
>>>>>
>>>>
>>> https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/6278
>>>>>
>>>>> thinks it touched 66 test files.
>>>>>
>>>>> There’s currently no good data to predict how long StressNew will
>>> take,
>>>> it
>>>>> just depends on the mix of tests touched. I am aware of at least 4
>>> cases
>>>>> (with less than 25 files) where the timeout was hit. In two of those
>>>> cases
>>>>> we re-ran with a longer timeout, and in two cases the PR author split
>>> up
>>>>> the PR into half a dozen smaller PRs to get around it.
>>>>>
>>>>>
>>>>>> On Mar 1, 2020, at 7:52 PM, Anthony Baker <ab...@pivotal.io>
>>> wrote:
>>>>>>
>>>>>> What percentage of PR’s are currently subject to the 25 test file
>>> rule?
>>>>> How many would be subject to the concourse timeout?
>>>>>>
>>>>>> I’d like to understand the scope of the impact for this change.
>>>>>>
>>>>>> Anthony
>>>>>>
>>>>>>
>>>>>>> On Mar 1, 2020, at 8:58 AM, Owen Nichols <on...@pivotal.io>
>>> wrote:
>>>>>>>
>>>>>>> Impossible, no. Inconvenient, perhaps, but a small price to pay for
>>>>> being
>>>>>>> able to trust that green means green.
>>>>>>>
>>>>>>> With or without this proposed change, if anyone is having trouble
>>>>> getting
>>>>>>> their PR to pass StressNew, please bring it up on the dev list and
>>> we
>>>>> can
>>>>>>> discuss the appropriate solution on a case-by-case basis (e.g.
>>>>> increasing
>>>>>>> timeout, fixing the logic that identifies changed test files,
>>>> splitting
>>>>>>> into multiple PRs, authorizing an override, etc).
>>>>>>>
>>>>>>> On Sun, Mar 1, 2020 at 8:56 AM Dan Smith <ds...@pivotal.io>
>>> wrote:
>>>>>>>
>>>>>>>> Won't this make it impossible to merge refactoring changes that
>>> touch
>>>>> a lot
>>>>>>>> of tests?
>>>>>>>>
>>>>>>>> -Dan
>>>>>>>>
>>>>>>>> On Sat, Feb 29, 2020, 12:37 PM Robert Houghton <
>>> rhoughton@pivotal.io
>>>>>
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>>> Yes, as it should
>>>>>>>>>
>>>>>>>>> On Sat, Feb 29, 2020, 12:25 Dan Smith <ds...@pivotal.io> wrote:
>>>>>>>>>
>>>>>>>>>> Doesn't the build fail when concourse times out?
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
Re: [PROPOSAL] eliminate file count loophole in PR StressNewTest
Posted by Donal Evans <do...@pivotal.io>.
With fairly apt timing, we've recently had a PR merged (
https://github.com/apache/geode/pull/4745) that introduced a test that has
resulted in fairly consistently flaky failures in the pipeline (JIRA
ticket: https://issues.apache.org/jira/projects/GEODE/issues/GEODE-7843).
The PR was quite large and touched/created a lot of tests, so StressNewTest
never ran on it:
https://concourse.apachegeode-ci.info/builds/136784#L5e3ac3fc:2. Given how
frequently the test is failing in the pipeline (11 failures on various
IntegrationTest jobs over the past 6 commits), it seems very likely that
had StressNewTest been allowed to run, this issue would have been caught at
the PR stage and could have been remedied then, instead of resulting in a
nearly constantly red pipeline.
I've already given my +1 to this proposal, but this situation has prompted
me to make it a +2.
On Fri, Mar 6, 2020 at 1:46 PM Donal Evans <do...@pivotal.io> wrote:
> With fairly apt timing, we've recently had a PR merged (
> https://github.com/apache/geode/pull/4745) that introduced a test that
> has resulted in fairly consistently flaky failures in the pipeline (JIRA
> ticket: https://issues.apache.org/jira/projects/GEODE/issues/GEODE-7843).
> The PR was quite large and touched/created a lot of tests, so StressNewTest
> never ran on it:
> https://concourse.apachegeode-ci.info/builds/136784#L5e3ac3fc:2. Given
> how frequently the test is failing in the pipeline (11 failures on various
> IntegrationTest jobs over the past 6 commits), it seems very likely that
> had StressNewTest been allowed to run, this issue would have been caught at
> the PR stage and could have been remedied then, instead of resulting in a
> nearly constantly red pipeline.
>
> I've already given my +1 to this proposal, but this situation has prompted
> me to make it a *+1*.
>
> On Tue, Mar 3, 2020 at 2:08 PM Anilkumar Gingade <ag...@pivotal.io>
> wrote:
>
>> The stress test is to identify the flaky-ness within the test; and
>> assuming
>> any changes to the test may have introduced the flaky-ness.
>> It's about paying the cost upfront or later when the test is determined to
>> be flaky.
>> If 25+ tests have been changed in a PR, the cost of running stress test
>> for
>> those; and gating the PR for so long.
>> Knowing how much pain it causes to fix a flaky test after a certain/long
>> duration of time; I am +1 for doing this change.
>>
>> On Tue, Mar 3, 2020 at 10:06 AM Dan Smith <ds...@pivotal.io> wrote:
>>
>> > What's the current timeout for StressNewTest? Maybe if we just up the
>> > threshold to 100 tests or so and up the timeout to match we can catch
>> > pretty much all PRs.
>> >
>> > I'm not sure why the job is flagging more tests than it should. It looks
>> > like at some point @rhoughon changed it to read the merge base from some
>> > file created by concourse as an optimization [1] - I suspect maybe that
>> > file is inaccurate?
>> >
>> > I originally wrote this job. It's definitely not a panacea, it will only
>> > catch a new flaky test if
>> > - the test is really flaky (likely to fail more than 1/50 times)
>> > - the change actually happened in the test file itself, and not the
>> > product or some other test file.
>> >
>> > [1]
>> >
>> >
>> https://github.com/apache/geode/commit/4c06ba4625e69d44a5165aa9f2fccddfc064de87
>> >
>> > -Dan
>> >
>> > On Sun, Mar 1, 2020 at 9:00 PM Owen Nichols <on...@pivotal.io>
>> wrote:
>> >
>> > > We don’t tend to look too closely at successful PR checks to see
>> whether
>> > > they actually checked anything at all.
>> > >
>> > > One example I found is
>> > >
>> >
>> https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/5957
>> > > <
>> > >
>> >
>> https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/5957
>> > > >:
>> > > 32 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing.
>> > >
>> > > Here are 92 more examples (url’s omitted for brevity — use the example
>> > > above as a template and just replace the last 4 digits):
>> > > 26 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6243)
>> > > 26 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6249)
>> > > 26 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6402)
>> > > 27 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6262)
>> > > 27 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6430)
>> > > 27 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6439)
>> > > 27 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6449)
>> > > 27 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6454)
>> > > 27 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6458)
>> > > 27 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6459)
>> > > 28 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6224)
>> > > 28 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6441)
>> > > 28 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6448)
>> > > 28 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6452)
>> > > 29 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6102)
>> > > 29 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6177)
>> > > 30 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 5939)
>> > > 30 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 5940)
>> > > 30 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 5949)
>> > > 30 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6473)
>> > > 31 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 5953)
>> > > 31 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6187)
>> > > 31 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6470)
>> > > 31 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6471)
>> > > 31 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6474)
>> > > 31 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6475)
>> > > 32 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 5958)
>> > > 32 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6173)
>> > > 32 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6236)
>> > > 32 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6237)
>> > > 32 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6242)
>> > > 33 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6246)
>> > > 33 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6248)
>> > > 33 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6250)
>> > > 33 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6251)
>> > > 33 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6254)
>> > > 33 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6255)
>> > > 34 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6139)
>> > > 34 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6141)
>> > > 34 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6264)
>> > > 34 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6266)
>> > > 34 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6271)
>> > > 35 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6142)
>> > > 35 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6143)
>> > > 35 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6277)
>> > > 35 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6309)
>> > > 35 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6413)
>> > > 35 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6414)
>> > > 35 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6419)
>> > > 35 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6420)
>> > > 36 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6144)
>> > > 36 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6146)
>> > > 36 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6147)
>> > > 36 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6310)
>> > > 36 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6326)
>> > > 36 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6329)
>> > > 36 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6330)
>> > > 40 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6159)
>> > > 40 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6160)
>> > > 40 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6188)
>> > > 41 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6440)
>> > > 41 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6480)
>> > > 41 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6483)
>> > > 43 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6190)
>> > > 44 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 5768)
>> > > 44 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 5772)
>> > > 46 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6191)
>> > > 46 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6218)
>> > > 46 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6219)
>> > > 46 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6225)
>> > > 46 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6244)
>> > > 46 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6245)
>> > > 48 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6129)
>> > > 49 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 5804)
>> > > 51 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 5877)
>> > > 53 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 5742)
>> > > 54 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 5757)
>> > > 66 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6232)
>> > > 66 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6233)
>> > > 66 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6234)
>> > > 66 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6259)
>> > > 66 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6260)
>> > > 66 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6278)
>> > > 79 is too many changed tests to stress test. Allowing this job to pass
>> > > without stress testing. (build 6481)
>> > > 103 is too many changed tests to stress test. Allowing this job to
>> pass
>> > > without stress testing. (build 6493)
>> > > 106 is too many changed tests to stress test. Allowing this job to
>> pass
>> > > without stress testing. (build 6206)
>> > > 106 is too many changed tests to stress test. Allowing this job to
>> pass
>> > > without stress testing. (build 6207)
>> > > 115 is too many changed tests to stress test. Allowing this job to
>> pass
>> > > without stress testing. (build 5769)
>> > > 118 is too many changed tests to stress test. Allowing this job to
>> pass
>> > > without stress testing. (build 6226)
>> > > 721 is too many changed tests to stress test. Allowing this job to
>> pass
>> > > without stress testing. (build 6197)
>> > > 722 is too many changed tests to stress test. Allowing this job to
>> pass
>> > > without stress testing. (build 6217)
>> > > 722 is too many changed tests to stress test. Allowing this job to
>> pass
>> > > without stress testing. (build 6221)
>> > >
>> > > Please note, sometimes the number of files reported seems to be way
>> > higher
>> > > than it should be. For example
>> > https://github.com/apache/geode/pull/4691
>> > > <https://github.com/apache/geode/pull/4691> shows exactly 1 test file
>> > > touched, but
>> > >
>> >
>> https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/6278
>> > > <
>> > >
>> >
>> https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/6278
>> > >
>> > > thinks it touched 66 test files.
>> > >
>> > > There’s currently no good data to predict how long StressNew will
>> take,
>> > it
>> > > just depends on the mix of tests touched. I am aware of at least 4
>> cases
>> > > (with less than 25 files) where the timeout was hit. In two of those
>> > cases
>> > > we re-ran with a longer timeout, and in two cases the PR author split
>> up
>> > > the PR into half a dozen smaller PRs to get around it.
>> > >
>> > >
>> > > > On Mar 1, 2020, at 7:52 PM, Anthony Baker <ab...@pivotal.io>
>> wrote:
>> > > >
>> > > > What percentage of PR’s are currently subject to the 25 test file
>> rule?
>> > > How many would be subject to the concourse timeout?
>> > > >
>> > > > I’d like to understand the scope of the impact for this change.
>> > > >
>> > > > Anthony
>> > > >
>> > > >
>> > > >> On Mar 1, 2020, at 8:58 AM, Owen Nichols <on...@pivotal.io>
>> wrote:
>> > > >>
>> > > >> Impossible, no. Inconvenient, perhaps, but a small price to pay for
>> > > being
>> > > >> able to trust that green means green.
>> > > >>
>> > > >> With or without this proposed change, if anyone is having trouble
>> > > getting
>> > > >> their PR to pass StressNew, please bring it up on the dev list and
>> we
>> > > can
>> > > >> discuss the appropriate solution on a case-by-case basis (e.g.
>> > > increasing
>> > > >> timeout, fixing the logic that identifies changed test files,
>> > splitting
>> > > >> into multiple PRs, authorizing an override, etc).
>> > > >>
>> > > >> On Sun, Mar 1, 2020 at 8:56 AM Dan Smith <ds...@pivotal.io>
>> wrote:
>> > > >>
>> > > >>> Won't this make it impossible to merge refactoring changes that
>> touch
>> > > a lot
>> > > >>> of tests?
>> > > >>>
>> > > >>> -Dan
>> > > >>>
>> > > >>> On Sat, Feb 29, 2020, 12:37 PM Robert Houghton <
>> rhoughton@pivotal.io
>> > >
>> > > >>> wrote:
>> > > >>>
>> > > >>>> Yes, as it should
>> > > >>>>
>> > > >>>> On Sat, Feb 29, 2020, 12:25 Dan Smith <ds...@pivotal.io> wrote:
>> > > >>>>
>> > > >>>>> Doesn't the build fail when concourse times out?
>> > > >>>>
>> > > >>>>
>> > > >>>
>> > > >
>> > >
>> > >
>> >
>>
>
Re: [PROPOSAL] eliminate file count loophole in PR StressNewTest
Posted by Donal Evans <do...@pivotal.io>.
With fairly apt timing, we've recently had a PR merged (
https://github.com/apache/geode/pull/4745) that introduced a test that has
resulted in fairly consistently flaky failures in the pipeline (JIRA
ticket: https://issues.apache.org/jira/projects/GEODE/issues/GEODE-7843).
The PR was quite large and touched/created a lot of tests, so StressNewTest
never ran on it:
https://concourse.apachegeode-ci.info/builds/136784#L5e3ac3fc:2. Given how
frequently the test is failing in the pipeline (11 failures on various
IntegrationTest jobs over the past 6 commits), it seems very likely that
had StressNewTest been allowed to run, this issue would have been caught at
the PR stage and could have been remedied then, instead of resulting in a
nearly constantly red pipeline.
I've already given my +1 to this proposal, but this situation has prompted
me to make it a *+1*.
On Tue, Mar 3, 2020 at 2:08 PM Anilkumar Gingade <ag...@pivotal.io>
wrote:
> The stress test is to identify the flaky-ness within the test; and assuming
> any changes to the test may have introduced the flaky-ness.
> It's about paying the cost upfront or later when the test is determined to
> be flaky.
> If 25+ tests have been changed in a PR, the cost of running stress test for
> those; and gating the PR for so long.
> Knowing how much pain it causes to fix a flaky test after a certain/long
> duration of time; I am +1 for doing this change.
>
> On Tue, Mar 3, 2020 at 10:06 AM Dan Smith <ds...@pivotal.io> wrote:
>
> > What's the current timeout for StressNewTest? Maybe if we just up the
> > threshold to 100 tests or so and up the timeout to match we can catch
> > pretty much all PRs.
> >
> > I'm not sure why the job is flagging more tests than it should. It looks
> > like at some point @rhoughon changed it to read the merge base from some
> > file created by concourse as an optimization [1] - I suspect maybe that
> > file is inaccurate?
> >
> > I originally wrote this job. It's definitely not a panacea, it will only
> > catch a new flaky test if
> > - the test is really flaky (likely to fail more than 1/50 times)
> > - the change actually happened in the test file itself, and not the
> > product or some other test file.
> >
> > [1]
> >
> >
> https://github.com/apache/geode/commit/4c06ba4625e69d44a5165aa9f2fccddfc064de87
> >
> > -Dan
> >
> > On Sun, Mar 1, 2020 at 9:00 PM Owen Nichols <on...@pivotal.io> wrote:
> >
> > > We don’t tend to look too closely at successful PR checks to see
> whether
> > > they actually checked anything at all.
> > >
> > > One example I found is
> > >
> >
> https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/5957
> > > <
> > >
> >
> https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/5957
> > > >:
> > > 32 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing.
> > >
> > > Here are 92 more examples (url’s omitted for brevity — use the example
> > > above as a template and just replace the last 4 digits):
> > > 26 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6243)
> > > 26 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6249)
> > > 26 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6402)
> > > 27 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6262)
> > > 27 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6430)
> > > 27 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6439)
> > > 27 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6449)
> > > 27 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6454)
> > > 27 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6458)
> > > 27 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6459)
> > > 28 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6224)
> > > 28 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6441)
> > > 28 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6448)
> > > 28 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6452)
> > > 29 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6102)
> > > 29 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6177)
> > > 30 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 5939)
> > > 30 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 5940)
> > > 30 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 5949)
> > > 30 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6473)
> > > 31 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 5953)
> > > 31 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6187)
> > > 31 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6470)
> > > 31 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6471)
> > > 31 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6474)
> > > 31 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6475)
> > > 32 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 5958)
> > > 32 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6173)
> > > 32 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6236)
> > > 32 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6237)
> > > 32 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6242)
> > > 33 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6246)
> > > 33 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6248)
> > > 33 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6250)
> > > 33 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6251)
> > > 33 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6254)
> > > 33 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6255)
> > > 34 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6139)
> > > 34 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6141)
> > > 34 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6264)
> > > 34 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6266)
> > > 34 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6271)
> > > 35 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6142)
> > > 35 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6143)
> > > 35 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6277)
> > > 35 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6309)
> > > 35 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6413)
> > > 35 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6414)
> > > 35 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6419)
> > > 35 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6420)
> > > 36 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6144)
> > > 36 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6146)
> > > 36 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6147)
> > > 36 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6310)
> > > 36 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6326)
> > > 36 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6329)
> > > 36 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6330)
> > > 40 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6159)
> > > 40 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6160)
> > > 40 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6188)
> > > 41 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6440)
> > > 41 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6480)
> > > 41 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6483)
> > > 43 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6190)
> > > 44 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 5768)
> > > 44 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 5772)
> > > 46 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6191)
> > > 46 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6218)
> > > 46 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6219)
> > > 46 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6225)
> > > 46 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6244)
> > > 46 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6245)
> > > 48 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6129)
> > > 49 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 5804)
> > > 51 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 5877)
> > > 53 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 5742)
> > > 54 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 5757)
> > > 66 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6232)
> > > 66 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6233)
> > > 66 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6234)
> > > 66 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6259)
> > > 66 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6260)
> > > 66 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6278)
> > > 79 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6481)
> > > 103 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6493)
> > > 106 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6206)
> > > 106 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6207)
> > > 115 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 5769)
> > > 118 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6226)
> > > 721 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6197)
> > > 722 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6217)
> > > 722 is too many changed tests to stress test. Allowing this job to pass
> > > without stress testing. (build 6221)
> > >
> > > Please note, sometimes the number of files reported seems to be way
> > higher
> > > than it should be. For example
> > https://github.com/apache/geode/pull/4691
> > > <https://github.com/apache/geode/pull/4691> shows exactly 1 test file
> > > touched, but
> > >
> >
> https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/6278
> > > <
> > >
> >
> https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/6278
> > >
> > > thinks it touched 66 test files.
> > >
> > > There’s currently no good data to predict how long StressNew will take,
> > it
> > > just depends on the mix of tests touched. I am aware of at least 4
> cases
> > > (with less than 25 files) where the timeout was hit. In two of those
> > cases
> > > we re-ran with a longer timeout, and in two cases the PR author split
> up
> > > the PR into half a dozen smaller PRs to get around it.
> > >
> > >
> > > > On Mar 1, 2020, at 7:52 PM, Anthony Baker <ab...@pivotal.io> wrote:
> > > >
> > > > What percentage of PR’s are currently subject to the 25 test file
> rule?
> > > How many would be subject to the concourse timeout?
> > > >
> > > > I’d like to understand the scope of the impact for this change.
> > > >
> > > > Anthony
> > > >
> > > >
> > > >> On Mar 1, 2020, at 8:58 AM, Owen Nichols <on...@pivotal.io>
> wrote:
> > > >>
> > > >> Impossible, no. Inconvenient, perhaps, but a small price to pay for
> > > being
> > > >> able to trust that green means green.
> > > >>
> > > >> With or without this proposed change, if anyone is having trouble
> > > getting
> > > >> their PR to pass StressNew, please bring it up on the dev list and
> we
> > > can
> > > >> discuss the appropriate solution on a case-by-case basis (e.g.
> > > increasing
> > > >> timeout, fixing the logic that identifies changed test files,
> > splitting
> > > >> into multiple PRs, authorizing an override, etc).
> > > >>
> > > >> On Sun, Mar 1, 2020 at 8:56 AM Dan Smith <ds...@pivotal.io> wrote:
> > > >>
> > > >>> Won't this make it impossible to merge refactoring changes that
> touch
> > > a lot
> > > >>> of tests?
> > > >>>
> > > >>> -Dan
> > > >>>
> > > >>> On Sat, Feb 29, 2020, 12:37 PM Robert Houghton <
> rhoughton@pivotal.io
> > >
> > > >>> wrote:
> > > >>>
> > > >>>> Yes, as it should
> > > >>>>
> > > >>>> On Sat, Feb 29, 2020, 12:25 Dan Smith <ds...@pivotal.io> wrote:
> > > >>>>
> > > >>>>> Doesn't the build fail when concourse times out?
> > > >>>>
> > > >>>>
> > > >>>
> > > >
> > >
> > >
> >
>
Re: [PROPOSAL] eliminate file count loophole in PR StressNewTest
Posted by Anilkumar Gingade <ag...@pivotal.io>.
The stress test is to identify the flaky-ness within the test; and assuming
any changes to the test may have introduced the flaky-ness.
It's about paying the cost upfront or later when the test is determined to
be flaky.
If 25+ tests have been changed in a PR, the cost of running stress test for
those; and gating the PR for so long.
Knowing how much pain it causes to fix a flaky test after a certain/long
duration of time; I am +1 for doing this change.
On Tue, Mar 3, 2020 at 10:06 AM Dan Smith <ds...@pivotal.io> wrote:
> What's the current timeout for StressNewTest? Maybe if we just up the
> threshold to 100 tests or so and up the timeout to match we can catch
> pretty much all PRs.
>
> I'm not sure why the job is flagging more tests than it should. It looks
> like at some point @rhoughon changed it to read the merge base from some
> file created by concourse as an optimization [1] - I suspect maybe that
> file is inaccurate?
>
> I originally wrote this job. It's definitely not a panacea, it will only
> catch a new flaky test if
> - the test is really flaky (likely to fail more than 1/50 times)
> - the change actually happened in the test file itself, and not the
> product or some other test file.
>
> [1]
>
> https://github.com/apache/geode/commit/4c06ba4625e69d44a5165aa9f2fccddfc064de87
>
> -Dan
>
> On Sun, Mar 1, 2020 at 9:00 PM Owen Nichols <on...@pivotal.io> wrote:
>
> > We don’t tend to look too closely at successful PR checks to see whether
> > they actually checked anything at all.
> >
> > One example I found is
> >
> https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/5957
> > <
> >
> https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/5957
> > >:
> > 32 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing.
> >
> > Here are 92 more examples (url’s omitted for brevity — use the example
> > above as a template and just replace the last 4 digits):
> > 26 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6243)
> > 26 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6249)
> > 26 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6402)
> > 27 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6262)
> > 27 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6430)
> > 27 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6439)
> > 27 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6449)
> > 27 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6454)
> > 27 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6458)
> > 27 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6459)
> > 28 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6224)
> > 28 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6441)
> > 28 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6448)
> > 28 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6452)
> > 29 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6102)
> > 29 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6177)
> > 30 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 5939)
> > 30 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 5940)
> > 30 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 5949)
> > 30 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6473)
> > 31 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 5953)
> > 31 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6187)
> > 31 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6470)
> > 31 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6471)
> > 31 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6474)
> > 31 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6475)
> > 32 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 5958)
> > 32 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6173)
> > 32 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6236)
> > 32 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6237)
> > 32 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6242)
> > 33 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6246)
> > 33 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6248)
> > 33 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6250)
> > 33 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6251)
> > 33 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6254)
> > 33 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6255)
> > 34 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6139)
> > 34 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6141)
> > 34 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6264)
> > 34 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6266)
> > 34 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6271)
> > 35 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6142)
> > 35 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6143)
> > 35 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6277)
> > 35 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6309)
> > 35 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6413)
> > 35 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6414)
> > 35 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6419)
> > 35 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6420)
> > 36 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6144)
> > 36 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6146)
> > 36 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6147)
> > 36 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6310)
> > 36 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6326)
> > 36 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6329)
> > 36 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6330)
> > 40 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6159)
> > 40 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6160)
> > 40 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6188)
> > 41 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6440)
> > 41 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6480)
> > 41 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6483)
> > 43 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6190)
> > 44 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 5768)
> > 44 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 5772)
> > 46 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6191)
> > 46 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6218)
> > 46 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6219)
> > 46 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6225)
> > 46 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6244)
> > 46 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6245)
> > 48 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6129)
> > 49 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 5804)
> > 51 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 5877)
> > 53 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 5742)
> > 54 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 5757)
> > 66 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6232)
> > 66 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6233)
> > 66 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6234)
> > 66 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6259)
> > 66 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6260)
> > 66 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6278)
> > 79 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6481)
> > 103 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6493)
> > 106 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6206)
> > 106 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6207)
> > 115 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 5769)
> > 118 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6226)
> > 721 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6197)
> > 722 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6217)
> > 722 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6221)
> >
> > Please note, sometimes the number of files reported seems to be way
> higher
> > than it should be. For example
> https://github.com/apache/geode/pull/4691
> > <https://github.com/apache/geode/pull/4691> shows exactly 1 test file
> > touched, but
> >
> https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/6278
> > <
> >
> https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/6278
> >
> > thinks it touched 66 test files.
> >
> > There’s currently no good data to predict how long StressNew will take,
> it
> > just depends on the mix of tests touched. I am aware of at least 4 cases
> > (with less than 25 files) where the timeout was hit. In two of those
> cases
> > we re-ran with a longer timeout, and in two cases the PR author split up
> > the PR into half a dozen smaller PRs to get around it.
> >
> >
> > > On Mar 1, 2020, at 7:52 PM, Anthony Baker <ab...@pivotal.io> wrote:
> > >
> > > What percentage of PR’s are currently subject to the 25 test file rule?
> > How many would be subject to the concourse timeout?
> > >
> > > I’d like to understand the scope of the impact for this change.
> > >
> > > Anthony
> > >
> > >
> > >> On Mar 1, 2020, at 8:58 AM, Owen Nichols <on...@pivotal.io> wrote:
> > >>
> > >> Impossible, no. Inconvenient, perhaps, but a small price to pay for
> > being
> > >> able to trust that green means green.
> > >>
> > >> With or without this proposed change, if anyone is having trouble
> > getting
> > >> their PR to pass StressNew, please bring it up on the dev list and we
> > can
> > >> discuss the appropriate solution on a case-by-case basis (e.g.
> > increasing
> > >> timeout, fixing the logic that identifies changed test files,
> splitting
> > >> into multiple PRs, authorizing an override, etc).
> > >>
> > >> On Sun, Mar 1, 2020 at 8:56 AM Dan Smith <ds...@pivotal.io> wrote:
> > >>
> > >>> Won't this make it impossible to merge refactoring changes that touch
> > a lot
> > >>> of tests?
> > >>>
> > >>> -Dan
> > >>>
> > >>> On Sat, Feb 29, 2020, 12:37 PM Robert Houghton <rhoughton@pivotal.io
> >
> > >>> wrote:
> > >>>
> > >>>> Yes, as it should
> > >>>>
> > >>>> On Sat, Feb 29, 2020, 12:25 Dan Smith <ds...@pivotal.io> wrote:
> > >>>>
> > >>>>> Doesn't the build fail when concourse times out?
> > >>>>
> > >>>>
> > >>>
> > >
> >
> >
>
Re: [PROPOSAL] eliminate file count loophole in PR StressNewTest
Posted by Dan Smith <ds...@pivotal.io>.
On Tue, Mar 10, 2020 at 10:51 AM Robert Houghton <rh...@pivotal.io>
wrote:
> @dsmith
> The file change for determining the merge base was due to changing the
> Concourse resource. The file encodes the merge-base SHA, regardless of
> depth of the git checkout.
>
> Makes sense. But maybe the file is inaccurate? Seems like the most likely
reason why this job is flagging too many tests. Is there any way to recover
that file from the job owen pointed out -
https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/6278?
Or maybe we just need to add more logging to this job to figure out why
it's flagging too many tests, including this file.
-Dan
Re: [PROPOSAL] eliminate file count loophole in PR StressNewTest
Posted by Robert Houghton <rh...@pivotal.io>.
@dsmith
The file change for determining the merge base was due to changing the
Concourse resource. The file encodes the merge-base SHA, regardless of
depth of the git checkout.
On Tue, Mar 3, 2020 at 10:06 AM Dan Smith <ds...@pivotal.io> wrote:
> What's the current timeout for StressNewTest? Maybe if we just up the
> threshold to 100 tests or so and up the timeout to match we can catch
> pretty much all PRs.
>
> I'm not sure why the job is flagging more tests than it should. It looks
> like at some point @rhoughon changed it to read the merge base from some
> file created by concourse as an optimization [1] - I suspect maybe that
> file is inaccurate?
>
> I originally wrote this job. It's definitely not a panacea, it will only
> catch a new flaky test if
> - the test is really flaky (likely to fail more than 1/50 times)
> - the change actually happened in the test file itself, and not the
> product or some other test file.
>
> [1]
>
> https://github.com/apache/geode/commit/4c06ba4625e69d44a5165aa9f2fccddfc064de87
>
> -Dan
>
> On Sun, Mar 1, 2020 at 9:00 PM Owen Nichols <on...@pivotal.io> wrote:
>
> > We don’t tend to look too closely at successful PR checks to see whether
> > they actually checked anything at all.
> >
> > One example I found is
> >
> https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/5957
> > <
> >
> https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/5957
> > >:
> > 32 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing.
> >
> > Here are 92 more examples (url’s omitted for brevity — use the example
> > above as a template and just replace the last 4 digits):
> > 26 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6243)
> > 26 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6249)
> > 26 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6402)
> > 27 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6262)
> > 27 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6430)
> > 27 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6439)
> > 27 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6449)
> > 27 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6454)
> > 27 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6458)
> > 27 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6459)
> > 28 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6224)
> > 28 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6441)
> > 28 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6448)
> > 28 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6452)
> > 29 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6102)
> > 29 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6177)
> > 30 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 5939)
> > 30 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 5940)
> > 30 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 5949)
> > 30 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6473)
> > 31 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 5953)
> > 31 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6187)
> > 31 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6470)
> > 31 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6471)
> > 31 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6474)
> > 31 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6475)
> > 32 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 5958)
> > 32 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6173)
> > 32 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6236)
> > 32 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6237)
> > 32 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6242)
> > 33 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6246)
> > 33 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6248)
> > 33 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6250)
> > 33 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6251)
> > 33 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6254)
> > 33 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6255)
> > 34 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6139)
> > 34 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6141)
> > 34 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6264)
> > 34 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6266)
> > 34 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6271)
> > 35 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6142)
> > 35 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6143)
> > 35 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6277)
> > 35 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6309)
> > 35 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6413)
> > 35 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6414)
> > 35 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6419)
> > 35 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6420)
> > 36 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6144)
> > 36 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6146)
> > 36 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6147)
> > 36 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6310)
> > 36 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6326)
> > 36 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6329)
> > 36 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6330)
> > 40 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6159)
> > 40 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6160)
> > 40 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6188)
> > 41 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6440)
> > 41 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6480)
> > 41 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6483)
> > 43 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6190)
> > 44 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 5768)
> > 44 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 5772)
> > 46 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6191)
> > 46 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6218)
> > 46 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6219)
> > 46 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6225)
> > 46 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6244)
> > 46 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6245)
> > 48 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6129)
> > 49 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 5804)
> > 51 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 5877)
> > 53 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 5742)
> > 54 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 5757)
> > 66 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6232)
> > 66 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6233)
> > 66 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6234)
> > 66 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6259)
> > 66 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6260)
> > 66 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6278)
> > 79 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6481)
> > 103 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6493)
> > 106 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6206)
> > 106 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6207)
> > 115 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 5769)
> > 118 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6226)
> > 721 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6197)
> > 722 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6217)
> > 722 is too many changed tests to stress test. Allowing this job to pass
> > without stress testing. (build 6221)
> >
> > Please note, sometimes the number of files reported seems to be way
> higher
> > than it should be. For example
> https://github.com/apache/geode/pull/4691
> > <https://github.com/apache/geode/pull/4691> shows exactly 1 test file
> > touched, but
> >
> https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/6278
> > <
> >
> https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/6278
> >
> > thinks it touched 66 test files.
> >
> > There’s currently no good data to predict how long StressNew will take,
> it
> > just depends on the mix of tests touched. I am aware of at least 4 cases
> > (with less than 25 files) where the timeout was hit. In two of those
> cases
> > we re-ran with a longer timeout, and in two cases the PR author split up
> > the PR into half a dozen smaller PRs to get around it.
> >
> >
> > > On Mar 1, 2020, at 7:52 PM, Anthony Baker <ab...@pivotal.io> wrote:
> > >
> > > What percentage of PR’s are currently subject to the 25 test file rule?
> > How many would be subject to the concourse timeout?
> > >
> > > I’d like to understand the scope of the impact for this change.
> > >
> > > Anthony
> > >
> > >
> > >> On Mar 1, 2020, at 8:58 AM, Owen Nichols <on...@pivotal.io> wrote:
> > >>
> > >> Impossible, no. Inconvenient, perhaps, but a small price to pay for
> > being
> > >> able to trust that green means green.
> > >>
> > >> With or without this proposed change, if anyone is having trouble
> > getting
> > >> their PR to pass StressNew, please bring it up on the dev list and we
> > can
> > >> discuss the appropriate solution on a case-by-case basis (e.g.
> > increasing
> > >> timeout, fixing the logic that identifies changed test files,
> splitting
> > >> into multiple PRs, authorizing an override, etc).
> > >>
> > >> On Sun, Mar 1, 2020 at 8:56 AM Dan Smith <ds...@pivotal.io> wrote:
> > >>
> > >>> Won't this make it impossible to merge refactoring changes that touch
> > a lot
> > >>> of tests?
> > >>>
> > >>> -Dan
> > >>>
> > >>> On Sat, Feb 29, 2020, 12:37 PM Robert Houghton <rhoughton@pivotal.io
> >
> > >>> wrote:
> > >>>
> > >>>> Yes, as it should
> > >>>>
> > >>>> On Sat, Feb 29, 2020, 12:25 Dan Smith <ds...@pivotal.io> wrote:
> > >>>>
> > >>>>> Doesn't the build fail when concourse times out?
> > >>>>
> > >>>>
> > >>>
> > >
> >
> >
>
Re: [PROPOSAL] eliminate file count loophole in PR StressNewTest
Posted by Dan Smith <ds...@pivotal.io>.
What's the current timeout for StressNewTest? Maybe if we just up the
threshold to 100 tests or so and up the timeout to match we can catch
pretty much all PRs.
I'm not sure why the job is flagging more tests than it should. It looks
like at some point @rhoughon changed it to read the merge base from some
file created by concourse as an optimization [1] - I suspect maybe that
file is inaccurate?
I originally wrote this job. It's definitely not a panacea, it will only
catch a new flaky test if
- the test is really flaky (likely to fail more than 1/50 times)
- the change actually happened in the test file itself, and not the
product or some other test file.
[1]
https://github.com/apache/geode/commit/4c06ba4625e69d44a5165aa9f2fccddfc064de87
-Dan
On Sun, Mar 1, 2020 at 9:00 PM Owen Nichols <on...@pivotal.io> wrote:
> We don’t tend to look too closely at successful PR checks to see whether
> they actually checked anything at all.
>
> One example I found is
> https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/5957
> <
> https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/5957
> >:
> 32 is too many changed tests to stress test. Allowing this job to pass
> without stress testing.
>
> Here are 92 more examples (url’s omitted for brevity — use the example
> above as a template and just replace the last 4 digits):
> 26 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6243)
> 26 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6249)
> 26 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6402)
> 27 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6262)
> 27 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6430)
> 27 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6439)
> 27 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6449)
> 27 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6454)
> 27 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6458)
> 27 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6459)
> 28 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6224)
> 28 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6441)
> 28 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6448)
> 28 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6452)
> 29 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6102)
> 29 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6177)
> 30 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 5939)
> 30 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 5940)
> 30 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 5949)
> 30 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6473)
> 31 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 5953)
> 31 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6187)
> 31 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6470)
> 31 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6471)
> 31 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6474)
> 31 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6475)
> 32 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 5958)
> 32 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6173)
> 32 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6236)
> 32 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6237)
> 32 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6242)
> 33 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6246)
> 33 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6248)
> 33 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6250)
> 33 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6251)
> 33 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6254)
> 33 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6255)
> 34 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6139)
> 34 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6141)
> 34 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6264)
> 34 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6266)
> 34 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6271)
> 35 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6142)
> 35 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6143)
> 35 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6277)
> 35 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6309)
> 35 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6413)
> 35 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6414)
> 35 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6419)
> 35 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6420)
> 36 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6144)
> 36 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6146)
> 36 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6147)
> 36 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6310)
> 36 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6326)
> 36 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6329)
> 36 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6330)
> 40 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6159)
> 40 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6160)
> 40 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6188)
> 41 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6440)
> 41 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6480)
> 41 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6483)
> 43 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6190)
> 44 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 5768)
> 44 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 5772)
> 46 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6191)
> 46 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6218)
> 46 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6219)
> 46 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6225)
> 46 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6244)
> 46 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6245)
> 48 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6129)
> 49 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 5804)
> 51 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 5877)
> 53 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 5742)
> 54 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 5757)
> 66 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6232)
> 66 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6233)
> 66 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6234)
> 66 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6259)
> 66 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6260)
> 66 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6278)
> 79 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6481)
> 103 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6493)
> 106 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6206)
> 106 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6207)
> 115 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 5769)
> 118 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6226)
> 721 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6197)
> 722 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6217)
> 722 is too many changed tests to stress test. Allowing this job to pass
> without stress testing. (build 6221)
>
> Please note, sometimes the number of files reported seems to be way higher
> than it should be. For example https://github.com/apache/geode/pull/4691
> <https://github.com/apache/geode/pull/4691> shows exactly 1 test file
> touched, but
> https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/6278
> <
> https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/6278>
> thinks it touched 66 test files.
>
> There’s currently no good data to predict how long StressNew will take, it
> just depends on the mix of tests touched. I am aware of at least 4 cases
> (with less than 25 files) where the timeout was hit. In two of those cases
> we re-ran with a longer timeout, and in two cases the PR author split up
> the PR into half a dozen smaller PRs to get around it.
>
>
> > On Mar 1, 2020, at 7:52 PM, Anthony Baker <ab...@pivotal.io> wrote:
> >
> > What percentage of PR’s are currently subject to the 25 test file rule?
> How many would be subject to the concourse timeout?
> >
> > I’d like to understand the scope of the impact for this change.
> >
> > Anthony
> >
> >
> >> On Mar 1, 2020, at 8:58 AM, Owen Nichols <on...@pivotal.io> wrote:
> >>
> >> Impossible, no. Inconvenient, perhaps, but a small price to pay for
> being
> >> able to trust that green means green.
> >>
> >> With or without this proposed change, if anyone is having trouble
> getting
> >> their PR to pass StressNew, please bring it up on the dev list and we
> can
> >> discuss the appropriate solution on a case-by-case basis (e.g.
> increasing
> >> timeout, fixing the logic that identifies changed test files, splitting
> >> into multiple PRs, authorizing an override, etc).
> >>
> >> On Sun, Mar 1, 2020 at 8:56 AM Dan Smith <ds...@pivotal.io> wrote:
> >>
> >>> Won't this make it impossible to merge refactoring changes that touch
> a lot
> >>> of tests?
> >>>
> >>> -Dan
> >>>
> >>> On Sat, Feb 29, 2020, 12:37 PM Robert Houghton <rh...@pivotal.io>
> >>> wrote:
> >>>
> >>>> Yes, as it should
> >>>>
> >>>> On Sat, Feb 29, 2020, 12:25 Dan Smith <ds...@pivotal.io> wrote:
> >>>>
> >>>>> Doesn't the build fail when concourse times out?
> >>>>
> >>>>
> >>>
> >
>
>
Re: [PROPOSAL] eliminate file count loophole in PR StressNewTest
Posted by Owen Nichols <on...@pivotal.io>.
We don’t tend to look too closely at successful PR checks to see whether they actually checked anything at all.
One example I found is https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/5957 <https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/5957>:
32 is too many changed tests to stress test. Allowing this job to pass without stress testing.
Here are 92 more examples (url’s omitted for brevity — use the example above as a template and just replace the last 4 digits):
26 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6243)
26 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6249)
26 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6402)
27 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6262)
27 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6430)
27 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6439)
27 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6449)
27 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6454)
27 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6458)
27 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6459)
28 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6224)
28 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6441)
28 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6448)
28 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6452)
29 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6102)
29 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6177)
30 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 5939)
30 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 5940)
30 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 5949)
30 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6473)
31 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 5953)
31 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6187)
31 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6470)
31 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6471)
31 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6474)
31 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6475)
32 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 5958)
32 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6173)
32 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6236)
32 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6237)
32 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6242)
33 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6246)
33 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6248)
33 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6250)
33 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6251)
33 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6254)
33 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6255)
34 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6139)
34 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6141)
34 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6264)
34 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6266)
34 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6271)
35 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6142)
35 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6143)
35 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6277)
35 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6309)
35 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6413)
35 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6414)
35 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6419)
35 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6420)
36 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6144)
36 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6146)
36 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6147)
36 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6310)
36 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6326)
36 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6329)
36 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6330)
40 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6159)
40 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6160)
40 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6188)
41 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6440)
41 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6480)
41 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6483)
43 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6190)
44 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 5768)
44 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 5772)
46 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6191)
46 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6218)
46 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6219)
46 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6225)
46 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6244)
46 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6245)
48 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6129)
49 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 5804)
51 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 5877)
53 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 5742)
54 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 5757)
66 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6232)
66 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6233)
66 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6234)
66 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6259)
66 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6260)
66 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6278)
79 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6481)
103 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6493)
106 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6206)
106 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6207)
115 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 5769)
118 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6226)
721 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6197)
722 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6217)
722 is too many changed tests to stress test. Allowing this job to pass without stress testing. (build 6221)
Please note, sometimes the number of files reported seems to be way higher than it should be. For example https://github.com/apache/geode/pull/4691 <https://github.com/apache/geode/pull/4691> shows exactly 1 test file touched, but https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/6278 <https://concourse.apachegeode-ci.info/teams/main/pipelines/apache-develop-pr/jobs/StressNewTestOpenJDK11/builds/6278> thinks it touched 66 test files.
There’s currently no good data to predict how long StressNew will take, it just depends on the mix of tests touched. I am aware of at least 4 cases (with less than 25 files) where the timeout was hit. In two of those cases we re-ran with a longer timeout, and in two cases the PR author split up the PR into half a dozen smaller PRs to get around it.
> On Mar 1, 2020, at 7:52 PM, Anthony Baker <ab...@pivotal.io> wrote:
>
> What percentage of PR’s are currently subject to the 25 test file rule? How many would be subject to the concourse timeout?
>
> I’d like to understand the scope of the impact for this change.
>
> Anthony
>
>
>> On Mar 1, 2020, at 8:58 AM, Owen Nichols <on...@pivotal.io> wrote:
>>
>> Impossible, no. Inconvenient, perhaps, but a small price to pay for being
>> able to trust that green means green.
>>
>> With or without this proposed change, if anyone is having trouble getting
>> their PR to pass StressNew, please bring it up on the dev list and we can
>> discuss the appropriate solution on a case-by-case basis (e.g. increasing
>> timeout, fixing the logic that identifies changed test files, splitting
>> into multiple PRs, authorizing an override, etc).
>>
>> On Sun, Mar 1, 2020 at 8:56 AM Dan Smith <ds...@pivotal.io> wrote:
>>
>>> Won't this make it impossible to merge refactoring changes that touch a lot
>>> of tests?
>>>
>>> -Dan
>>>
>>> On Sat, Feb 29, 2020, 12:37 PM Robert Houghton <rh...@pivotal.io>
>>> wrote:
>>>
>>>> Yes, as it should
>>>>
>>>> On Sat, Feb 29, 2020, 12:25 Dan Smith <ds...@pivotal.io> wrote:
>>>>
>>>>> Doesn't the build fail when concourse times out?
>>>>
>>>>
>>>
>
Re: [PROPOSAL] eliminate file count loophole in PR StressNewTest
Posted by Anthony Baker <ab...@pivotal.io>.
What percentage of PR’s are currently subject to the 25 test file rule? How many would be subject to the concourse timeout?
I’d like to understand the scope of the impact for this change.
Anthony
> On Mar 1, 2020, at 8:58 AM, Owen Nichols <on...@pivotal.io> wrote:
>
> Impossible, no. Inconvenient, perhaps, but a small price to pay for being
> able to trust that green means green.
>
> With or without this proposed change, if anyone is having trouble getting
> their PR to pass StressNew, please bring it up on the dev list and we can
> discuss the appropriate solution on a case-by-case basis (e.g. increasing
> timeout, fixing the logic that identifies changed test files, splitting
> into multiple PRs, authorizing an override, etc).
>
> On Sun, Mar 1, 2020 at 8:56 AM Dan Smith <ds...@pivotal.io> wrote:
>
>> Won't this make it impossible to merge refactoring changes that touch a lot
>> of tests?
>>
>> -Dan
>>
>> On Sat, Feb 29, 2020, 12:37 PM Robert Houghton <rh...@pivotal.io>
>> wrote:
>>
>>> Yes, as it should
>>>
>>> On Sat, Feb 29, 2020, 12:25 Dan Smith <ds...@pivotal.io> wrote:
>>>
>>>> Doesn't the build fail when concourse times out?
>>>
>>>
>>
Re: [PROPOSAL] eliminate file count loophole in PR StressNewTest
Posted by Owen Nichols <on...@pivotal.io>.
Impossible, no. Inconvenient, perhaps, but a small price to pay for being
able to trust that green means green.
With or without this proposed change, if anyone is having trouble getting
their PR to pass StressNew, please bring it up on the dev list and we can
discuss the appropriate solution on a case-by-case basis (e.g. increasing
timeout, fixing the logic that identifies changed test files, splitting
into multiple PRs, authorizing an override, etc).
On Sun, Mar 1, 2020 at 8:56 AM Dan Smith <ds...@pivotal.io> wrote:
> Won't this make it impossible to merge refactoring changes that touch a lot
> of tests?
>
> -Dan
>
> On Sat, Feb 29, 2020, 12:37 PM Robert Houghton <rh...@pivotal.io>
> wrote:
>
> > Yes, as it should
> >
> > On Sat, Feb 29, 2020, 12:25 Dan Smith <ds...@pivotal.io> wrote:
> >
> > > Doesn't the build fail when concourse times out?
> >
> >
>