Here's my take on stresstest. It's currently providing two purposes: 1) Prevents addition of new flaky tests
Some new flakiness does slip through. I can write a new test that passes 50-100 times consistently and thus gets by stresstest, but then fails once or twice in CI or dunitrunner when I run it 1000+ times. So it's not perfect, but this purpose does seem valuable and I think most of us don't find this part of it frustrating. 2) Forces fixing existing flaky tests that your PR has touched I think this is what is causing the frustration. If I change the name of a method which then changes 3 old tests, I'm forced to ensure that those 3 tests now pass stresstest. This is good and bad. First, it forces us to pay down some technical debt right now to get the PR in. But, it makes it so frustrating and painful that I'm worried people will be so turned off by fixing technical debt because of this that they won't do anything else to improve the code base. Personally, I'd prefer to disable stresstest for pre-existing tests (although, what do you do about a PR that actually edits an old test and makes it flaky when it wasn't before?), and try to figure out what we can do to encourage the community to pay down real technical debt in Geode. For real technical debt paydown, I'm talking about (in more-or-less this order): a) accept the fact that yes Geode has a huge amount of tech debt -- if enough of us remain in denial of this, we'll never make any progress and that's not the kind of project I want to remain on long term b) start passing in all dependencies into constructors c) write Unit Test for every class and NEVER approve a PR that doesn't add Unit Test coverage for any code that is touched, d) remove static code (except the most simple util methods that aren't specific to Geode) and static variables including singletons (a static variable is NOT a constant) e) use interfaces for dependencies so that one class does not directly depend on another impl class f) NEVER allow bi-directional dependencies -- fix any that exist #a-f would be a GREAT start. After that we can worry about breaking up god classes or other bigger changes, but the above is pretty much the prerequisite for any real paying down of tech debt. On Mon, Dec 30, 2019 at 9:04 AM Bruce Schuchardt <bschucha...@pivotal.io> wrote: > I feel that we should keep it but that we need to look into what's > causing the frustration with the stresstest job. That seems to be the > thing causing the most grief. People make a small change to some test, > such as changing an import statement, and then find that it fails in > stresstest. > > I also feel that we're in a learning phase and should have another > discussion about this in mid-2020. > > > On 12/27/19 1:47 PM, Owen Nichols wrote: > > In October we agreed to require at least 1 reviewer and 4 passing PR > checks before a PR can be merged. Now that we’re tried it for a few > months, do we like it? > > > > I saw some strong opinions on the dev list recently: > > > >> Changes to the infrastructure to flat out prevent things that should be > self policing is annoying. This PR review lock we have had already cost us > valuable time waiting for PR pipelines to pass that have no relevance to > the commit, like CI work. I hate to see process enforced that keeps us from > getting work done when necessary. > > > > and > > > >> I think we're getting more and more bureaucratic in our process and > that it stifles productivity. I was recently forced to spend three days > fixing tests in which I had changed an import statement before they would > pass stress testing. I'm glad the tests now pass reliably but I was very > frustrated by the process. > > > > Just wondering if others feel the same way. Is it time to make some > changes? > > > > -Owen >
