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 > > >>>> > > >>>> > > >>> > > > > >
