Re: [DISCUSS] Standardize use of awaitility to a higher timeout

[Dan Smith <ds...@pivotal.io>]

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 <pr...@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 <mh...@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 <jb...@pivotal.io>
> >> wrote:
> >> > >
> >> > > +1 what Dan said.
> >> > >
> >> > >> On Jul 11, 2018, at 11:16 AM, Dan Smith <ds...@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 <ud...@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
> >> > >>>>
> >> > >>>>
> >> > >>>
> >> >
> >> >
> >>
> >
> >
>