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 <rhough...@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 <dsm...@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 <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? > > > >>>>>>>>> > > > >>>>>>>>> > > > >>>>>>>> > > > >>>>>> > > > >>>>> > > > >>>>> > > > >>>> > > > >>> > > > >> > > > > > > > > >
