ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dmitriy Pavlov <dpav...@apache.org>
Subject Re: Default failure handler was changed for tests
Date Wed, 05 Dec 2018 10:40:35 GMT
Anton, please provide PR to demo your idea. Code speaks louder than words
sometimes.

No reason to revert a contribution if someone has an idea, which is not
clear for others.

Again, we should discuss not Dmitrii contribution, but the initial
selection of no-op.

If you will do a test failure fixes later and you will set new handler
StopNode+FailTest as the only option - ok for me.

ср, 5 дек. 2018 г. в 13:35, Anton Vinogradov <av@apache.org>:

> Dmitriy,
>
> As I said before, these changes allow tests to be successful in case of
> unexpected failures.
> That's not acceptable.
>
> As a reviewer, you have to be ready to provide arguments why these tests
> have to be fixed this way and what was the problem, in case you merged such
> changes.
> That's unacceptable to hide issues instead of fix.
>
> Now, I ask you, as a reviewer, to provide the explanation.
> What problem and at what test we solved by no-op handler.
>
> And I'm going to rollback changes in case arguments will not be provided.
>
> On Wed, Dec 5, 2018 at 1:10 PM Dmitriy Pavlov <dpavlov@apache.org> wrote:
>
> > I will not do any rollback because changes make tests better. Please pay
> > attention that no-op became default long time ago. Please discuss this
> > selection with authors of the previous commit. New commit changes
> > NoOp->FailTest+stopNode.
> >
> > Please provide a PR to demonstrate your idea how to transfer and handle
> > exceptions. I believe it will not work because the fail handler is
> > activated from any pool inside a node.
> >
> > ср, 5 дек. 2018 г. в 13:05, Anton Vinogradov <av@apache.org>:
> >
> > > Dmitriy,
> > >
> > > >> Which code block will do a throw?
> > > Depends on the test.
> > >
> > > Looks like we make the *bad *test even *worse*.
> > >
> > > That's not a correct fix.
> > > In case you expect failure you have to check this expectation inside
> the
> > > special handler.
> > >
> > > I'd like to ask you to rollback these changes and replace them with
> > correct
> > > fixes.
> > >
> > > On Wed, Dec 5, 2018 at 12:39 PM Andrey Mashenkov <
> > > andrey.mashenkov@gmail.com>
> > > wrote:
> > >
> > > > Hi,
> > > >
> > > > Dmitri,
> > > >
> > > > The meaningful failure handler as a default one looks reasonable.
> > > > Thanks a lot.
> > > >
> > > > But what is the reason to fallback to noop for 100+ test?
> > > > Does it means these test become failed after changing default failure
> > > > handler?
> > > > If so, let's create a ticket (may be umbrella) to investigate and fix
> > > this.
> > > >
> > > > I see 100+ touched files in PR and some of them are abstract classes,
> > so,
> > > > we have much more affected tests.
> > > > Seems, most of failover test doesn't expects if any critical internal
> > > issue
> > > > occur and there is no need to fallback to noop.
> > > > Other test should set custom failure handler to detect expected
> > failures
> > > or
> > > > if grid hanging simulation is needed (to keep hanged grid under
> > control).
> > > >
> > > > On Wed, Dec 5, 2018 at 12:16 PM Anton Vinogradov <av@apache.org>
> > wrote:
> > > >
> > > > > Dmitrii,
> > > > >
> > > > > No-op means "hide any problem", so, we lose the guarantees.
> > > > > Could you please share some examples where "no-op" better than
> > "strict
> > > > > try-catch with a check"?
> > > > >
> > > > > On Wed, Dec 5, 2018 at 11:37 AM Dmitrii Ryabov <
> > somefireone@gmail.com>
> > > > > wrote:
> > > > >
> > > > > > Anton, I think wrapping every disconnecting node with try-catch
> > will
> > > be
> > > > > > less readable than no-op handler.
> > > > > >
> > > > > > ср, 5 дек. 2018 г., 9:26 Dmitriy Pavlov dpavlov@apache.org:
> > > > > >
> > > > > > > Folks let me remind you that Dmitry changed default of
ALL
> tests
> > > from
> > > > > > noop
> > > > > > > to a meaningful handler. So we should start every message
here
> > from
> > > > > > saying
> > > > > > > thank you to Dmitry.
> > > > > > >
> > > > > > > Please review remaining tests and remove noop where possible.
> > > > > > >
> > > > > > > вт, 4 дек. 2018 г., 23:48 Andrey Mashenkov <
> > > > andrey.mashenkov@gmail.com
> > > > > >:
> > > > > > >
> > > > > > > > Hi all,
> > > > > > > >
> > > > > > > > Really, why noop?
> > > > > > > >
> > > > > > > > If you expect failure handler should be triggered,
you can
> > > override
> > > > > > > default
> > > > > > > > one and rise some flag, which can be checked in test.
> > > > > > > > This will make test clearer.
> > > > > > > >
> > > > > > > > With noop, you'll get previous unwanted  behavior,
that you
> are
> > > > > trying
> > > > > > to
> > > > > > > > improve, isnt'it?
> > > > > > > >
> > > > > > > > 4 дек. 2018 г. 23:25 пользователь
"Anton Vinogradov" <
> > > > av@apache.org>
> > > > > > > > написал:
> > > > > > > >
> > > > > > > > And you have to check the reason of failure inside
the
> > try-catch
> > > > > block,
> > > > > > > of
> > > > > > > > course.
> > > > > > > > In case found not equals to expected then test should
rethrow
> > the
> > > > > > > > exception.
> > > > > > > >
> > > > > > > >
> > > > > > > > вт, 4 дек. 2018 г. в 23:21, Anton Vinogradov
<av@apache.org
> >:
> > > > > > > >
> > > > > > > >
> > > > > > > > > Dmitrii,
> > > > > > > > >
> > > > > > > > > The solution is not clear to me.
> > > > > > > > > In case you expect the failure then a correct
case is to
> wrap
> > > it
> > > > > with
> > > > > > > > > try-catch block instead of no-op failure handler
usage.
> > > > > > > > >
> > > > > > > > > вт, 4 дек. 2018 г. в 21:41, Dmitrii Ryabov
<
> > > > somefireone@gmail.com
> > > > > >:
> > > > > > > > >
> > > > > > > > >> Anton,
> > > > > > > > >>
> > > > > > > > >> Tests in these classes check fail cases when
we expect
> > > critical
> > > > > > > > >> failure like node stop or exception thrown.
Such tests
> > trigger
> > > > > > failure
> > > > > > > > >> handler and it fails test when everything
goes as it
> should
> > > go.
> > > > > > That's
> > > > > > > > >> why we need no-op handler here.
> > > > > > > > >> вт, 4 дек. 2018 г. в 20:06, Dmitriy
Pavlov <
> > > dpavlov@apache.org
> > > > >:
> > > > > > > > >> >
> > > > > > > > >> > Hi Igniters,
> > > > > > > > >> >
> > > > > > > > >> > BTW, if you find in any of your tests
it does't need an
> > old
> > > > > value
> > > > > > of
> > > > > > > > >> > handler (=NoOp), feel free to remove
it.
> > > > > > > > >> >
> > > > > > > > >> > Sincerely,
> > > > > > > > >> > Dmitriy Pavlov
> > > > > > > > >> >
> > > > > > > > >> > вт, 4 дек. 2018 г. в 20:02, Anton
Vinogradov <
> > av@apache.org
> > > >:
> > > > > > > > >> >
> > > > > > > > >> > > Dmitrii,
> > > > > > > > >> > >
> > > > > > > > >> > > Could you please explain the reason
of explicit set of
> > > 100+
> > > > > > > > >> > > NoOpFailureHandlers?
> > > > > > > > >> > >
> > > > > > > > >> > >
> > > > > > > > >> > > вт, 4 дек. 2018 г. в 19:12,
Dmitrii Ryabov <
> > > > > > somefireone@gmail.com
> > > > > > > >:
> > > > > > > > >> > >
> > > > > > > > >> > > > Hello, Igniters!
> > > > > > > > >> > > >
> > > > > > > > >> > > > Today the test framework's
default no-op failure
> > handler
> > > > was
> > > > > > > > >> changed to
> > > > > > > > >> > > the
> > > > > > > > >> > > > handler, which stops the node
and fails the test.
> > > > > > > > >> > > >
> > > > > > > > >> > > > Over 100 tests kept no-op
failure handler by
> overrided
> > > > > > > > >> > > > `getFailureHandler()` method.
> > > > > > > > >> > > >
> > > > > > > > >> > > > If you'll found a problem
or something unexpected -
> > > write
> > > > > here
> > > > > > > or
> > > > > > > > >> in the
> > > > > > > > >> > > > ticket [1].
> > > > > > > > >> > > >
> > > > > > > > >> > > > [1]
> https://issues.apache.org/jira/browse/IGNITE-8227
> > > > > > > > >> > > >
> > > > > > > > >> > >
> > > > > > > > >>
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > > >
> > > > --
> > > > Best regards,
> > > > Andrey V. Mashenkov
> > > >
> > >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message