+1 On Fri, Mar 6, 2020 at 6:33 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? > > > > >>>> > > > > >>>> > > > > >>> > > > > > > > > > > > > > > > > > > >
