We could keep it in one JVM but make it optional for merging the PR? On Mon, Dec 30, 2019 at 10:06 AM Jinmei Liao <jil...@pivotal.io> wrote:
> Having the stressTest reuse the JVMs is close to running the tests in my > IDEA repeatedly for N times or running a package of tests together in my > IDEA. There was a time that I couldn't run a group of tests together in my > IDEA until I had to fix the problem for the stressTest. Keeping them > running in the same JVM definitely helped me finding more static state > leaks. > > On Mon, Dec 30, 2019 at 9:59 AM Dan Smith <dsm...@pivotal.io> wrote: > > > Regarding the StressTest job - how about we switch that have a new JVM > for > > each test? That's how DistributedTest and IntegrationTest normally run. > We > > let StressTest reuse the JVMs because it would be faster and find > problems > > related to static state left behind, but I think in practice people are > > finding new and difficult issues with existing tests that only are due to > > reusing the JVM. > > > > -Dan > > > > On Mon, Dec 30, 2019 at 9:54 AM Kirk Lund <kl...@apache.org> wrote: > > > > > 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 > > > > > > > > > > > > -- > Cheers > > Jinmei >
