Following up on this thread - I finally got around to getting these changes merged in!
A couple of things to note: - If you use regular Awaitility, your code will now fail spotless. Use GeodeAwaitility instead. Now there is no need to set any timeouts, poll delays, etc in your individual tests. - If you want to run with a shorter timeout in your IDE, you can just set the system property GEODE_AWAITILITY_TIMEOUT_SECONDS to something smaller in your IDE. Eg in intellij you can go to Run->Edit Configurations -> Defaults -> Junit and add something like -DGEODE_AWAITILITY_TIMEOUT_SECONDS=30 -Dan On Thu, Jul 12, 2018 at 10:25 AM Kirk Lund <kl...@apache.org> wrote: > I typically use 2 MINUTES as the timeout for everything I've worked on in > integration or distributed tests. I believe that whatever the underlying > async action is will still complete well under 2 MINUTES even if a long GC > pause occurs during that window of time. 5 MINUTES is probably longer than > we need but at least it'll be ok on very slow hardware. > > On Thu, Jul 12, 2018 at 10:21 AM, Kirk Lund <kl...@apache.org> wrote: > > > I recommend keeping this Awaitility with default timeout separate from > any > > Rules or MemberVM. I want to be able to use Awaitility in integration > tests > > and distributed tests that don't use MemberVM or the StarterRules. So > from > > that POV, I'd prefer the GeodeAwaitility that Dan proposes. > > > > On Thu, Jul 12, 2018 at 9:48 AM, Patrick Rhomberg <prhomb...@pivotal.io> > > wrote: > > > >> +1 to a single source of member wait calls. > >> > >> We already have a standard set of await methods for DUnit member VMs, > >> located in the MemberVM class, and delegated to that class from the > >> MemberStarterRule. I have a PR outstanding [1] that improves those > >> methods, too. > >> > >> For those awaits that are common for VM members, that may be a more > >> natural > >> place for these methods to live. > >> > >> Imagination is Change. > >> ~Patrick > >> > >> [1] PR 2039, https://github.com/apache/geode/pull/2039, currently just > >> waiting for a precheckin after a merge with develop. > >> > >> On Wed, Jul 11, 2018 at 1:03 PM, Mark Hanson <mhan...@pivotal.io> > wrote: > >> > >> > After further discussion, based on GC variability +1 for what Dan > said. > >> > > >> > Thanks, > >> > Mark > >> > > >> > > On Jul 11, 2018, at 12:53 PM, Jacob Barrett <jbarr...@pivotal.io> > >> wrote: > >> > > > >> > > +1 what Dan said. > >> > > > >> > >> On Jul 11, 2018, at 11:16 AM, Dan Smith <dsm...@pivotal.io> wrote: > >> > >> > >> > >> Well, some of these tests are waiting for members to startup, etc. > If > >> > the > >> > >> machine they are running on is slow, that could take more than a > >> minute. > >> > >> > >> > >> The point here is that these are not tests of how long it takes do > a > >> > geode > >> > >> operation. That's what performance tests are for. These tests just > >> have > >> > an > >> > >> atMost because we want them to fail, rather than hang, if the > >> assertion > >> > is > >> > >> never satisfied. Because these tests should always pass in a > variety > >> of > >> > >> environments, we should set atMost to be something really large. > >> > >> > >> > >> Performance tests that have assertions about timing need to run on > >> known > >> > >> hardware, and generally need to assert things like 99.9% the > response > >> > time > >> > >> is within this window. That's not what this test suite is. > >> > >> > >> > >> -Dan > >> > >> > >> > >>> On Wed, Jul 11, 2018 at 10:05 AM, Udo Kohlmeyer <u...@apache.org> > >> > wrote: > >> > >>> > >> > >>> Hi there Dan, > >> > >>> > >> > >>> Whilst 5min seems to be a viable option, I believe that knowing > how > >> > long > >> > >>> an operation should take and reacting if it doesn't complete in > that > >> > time > >> > >>> is better than waiting a standard amount of time. I like the > faster > >> > >>> feedback option, rather than the standard timeout across the > board. > >> > Now, > >> > >>> that said, crazy timeouts like 1-10s are maybe a little low. > >> > >>> > >> > >>> Maybe 1min rather than 5min? With exception to disk recovery > tests, > >> > which > >> > >>> feasibly could take more than 1min. I cannot think of a single > >> > operation in > >> > >>> Geode, that should realistically should take more than 60s. > >> > >>> > >> > >>> --Udo > >> > >>> > >> > >>> > >> > >>> > >> > >>>> On 7/11/18 09:07, Dan Smith wrote: > >> > >>>> > >> > >>>> Hi all, > >> > >>>> > >> > >>>> We have a bunch of tests that are using awaitility. It seems like > >> > every > >> > >>>> tests is picking some random number of it's timeout, usually in > the > >> > range > >> > >>>> of 10-60 seconds. > >> > >>>> > >> > >>>> I'd like to change all of our tests to use a standard timeout > that > >> is > >> > much > >> > >>>> higher, to avoid worrying about whether our timeouts are to low. > >> So I > >> > >>>> propose introducing our own GeodeAwaitility class that sets a > >> timeout > >> > of 5 > >> > >>>> minutes and replacing all of our usage with that. > >> > >>>> > >> > >>>> -Dan > >> > >>>> > >> > >>>> > >> > >>> > >> > > >> > > >> > > > > >