-1 to making StressNew not required +1 to eliminating the current loophole — StressNew should never give a free pass.
Any time your PR is having trouble passing StressNew, please bring it up on the dev list. We can review on a case-by-case basis and decide whether to try increasing the timeout, changing the repeat count, refactoring the PR, or as an absolute last resort requesting authorization for an override (for example, a change to spotless rules might touch a huge number of files but carry no risk). One bug we should fix is that StressNew sometimes counts more files touched than really were, especially if you had many commits or merges or rebases on your PR branch. Possible workarounds there include squashing and/or creating a new PR and/or splitting into multiple PRs. I’ve spent some time trying to reproduce why files are mis-counted, with no success, but perhaps someone cleverer with git could provide a fix. Another issue is that StressNew is only in the PR pipeline, not the main develop pipeline. This feels like an asymmetry where PRs must pass a “higher” standard. We should consider adding some form of StressNew to the main pipeline as well (maybe compare to the previous SHA that passed?). The original motivation for the 25-file limit was an attempt to limit how long StressNew might run for. Since concourse already applies a timeout, that check is unnecessary. However, a compromise solution might be to use the number of files changed to dial back the number of repeats, e.g. stay with 50 repeats if fewer than 25 files changed, otherwise compute 1250 / <#-files-changed> and do only that many repeats (e.g. if 50 files changed, run all tests 25x instead of 50x). While StressNew is intended to catch new flaky tests, it can also catch poorly-designed tests that fail just by running twice in the same VM. This may be a sign that the test does not clean up properly and could be polluting other tests in unexpected ways? It might be useful to run a “StressNew” with repeats=2 over a much broader scope, maybe even all tests, at least once in a while? > On Feb 28, 2020, at 9:51 AM, Mark Hanson <mhan...@pivotal.io> wrote: > > Hi All, > > Proposal: Force StressNewTest to fail a change with 25 or more files rather > than pass it without running it. > > Currently, the StressNewTest job in the pipeline will just pass a job that > has more than 25 files changed. It will be marked as green with no work done. > There are reasons, relating to run time being too long to be tracked by > concourse, so we just let it through as a pass. I think this is a bad signal. > I think that this should automatically be a failure in the short term. As a > result, I also think it should not be required. It is a bad signal, and I > think that by making it a fail, this will at least not give us a false sense > of security. I understand that this opens the flood gates so to speak, but I > don’t think as a community it is a big problem because we can still say that > you should not merge if the StressNewTest fails because of your test. > > I personally find the false sense of security more troubling than anything. > Hence the reason, I would like this to be > > Let me know what you think.. > > Thanks, > Mark
