ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Anton Vinogradov ...@apache.org>
Subject Re: Default failure handler was changed for tests
Date Wed, 05 Dec 2018 10:05:17 GMT
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