-1 to limiting any tests... if there are issues with the tests let's fix that. we have too many commits coming in with little or no testing over new/changed code, so I can't see how removing any existing test coverage as a good idea
Best, EB On Fri, Feb 28, 2020 at 10:58 AM Mark Hanson <mhan...@pivotal.io> wrote: > Just to make sure we are clear, I am not suggesting that we disable > stressnewtest, but that we make it not required. It would still run and > provide feedback, but it would not give us an unwarranted green in my > approach. > > > On Feb 28, 2020, at 10:49 AM, Ju@N <jujora...@gmail.com> wrote: > > > > +1 to what Owen said, I don't think disabling *StressNewTest* is a > > good idea. > > > > On Fri, 28 Feb 2020 at 18:35, Owen Nichols <onich...@pivotal.io> wrote: > > > >> -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 > >> > >> > > > > -- > > Ju@N > >
