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 <onich...@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 <doev...@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 <doev...@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 <aging...@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 <dsm...@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 <onich...@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 <aba...@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 <onich...@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 <dsm...@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 < > >>> rhough...@pivotal.io > >>>>> > >>>>>>>> wrote: > >>>>>>>> > >>>>>>>>> Yes, as it should > >>>>>>>>> > >>>>>>>>> On Sat, Feb 29, 2020, 12:25 Dan Smith <dsm...@pivotal.io> wrote: > >>>>>>>>> > >>>>>>>>>> Doesn't the build fail when concourse times out? > >>>>>>>>> > >>>>>>>>> > >>>>>>>> > >>>>>> > >>>>> > >>>>> > >>>> > >>> > >> > >
