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
