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 11:03:37 GMT
Dmitriy,

That's an incorrect idea to ask me to provide PR or to fix these test
properly since I'm not an author or reviewer.

But, I, as a community member, ask you to explain what problems the fix
fixes.
In case you're not able to provide the explanation I will rollback the
changes.

That's not acceptable to merge fix of unknown problems. At least, such "100
times copy-paste fix".
Please provide the explanation of the problem we're fixing for each test
group.

P.s. My goal is not to rollback something, but to prevent merge without
understanding what it fixes.

On Wed, Dec 5, 2018 at 1:40 PM Dmitriy Pavlov <dpavlov@apache.org> wrote:

> 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