Alright, so basically it seems like people are not ok with the not requiring stressnewtest approach. So I guess that means that we need to identify -1s willing to help resolve this problem…
Who would like to help? Thanks, Mark > On Feb 28, 2020, at 11:12 AM, Ernest Burghardt <eburgha...@pivotal.io> wrote: > > -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 >> >>
