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? > > > >>>> > > > >>>> > > > >>> > > > > > > > > > > > > >
