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 13:28:46 GMT
Anton, if you care enough here will you try to research a couple of these
tests? Or you are asking others to do things for you, aren't you?

I like idea from Andrew to create ticket and check these test to keep
moving towards 0....10 tests with noop. It is easy to locate these
overridden method now.

So threat this change as contributed mechanism for failing tests. Is it Ok
for you?

ср, 5 дек. 2018 г., 15:59 Anton Vinogradov <av@apache.org>:

> >> I didn't get. What is the problem in saving No-Op for several tests? Why
> >> should we keep No-Op for all?
> Several (less than 10) is ok to me with the proper explanation why tests
> fail and why no-op is a better choice.
>
> 100+++ copy-pasted no-op handlers are not ok!
>
> >> I don't ask you to re-do this change, I ask to demonstrate any better
> >> approach for tests which intentionally activate failure handler.
> You asking me to provide approach without explanation why tests fail
> without no-op handler?
> My approach is to rollback this fix, reopen the issue and make everything
> properly.
> Make a proper investigation first.
>
>
> Finally, let's stop this game.
> We have to discuss the reasons why tests fail.
> In case no-one checked "why" before the fix was merged we will be able to
> start doing this after rollback.
>
> On Wed, Dec 5, 2018 at 3:49 PM Eduard Shangareev <
> eduard.shangareev@gmail.com> wrote:
>
> > Guys,
> >
> > I didn't get. What is the problem in saving No-Op for several tests? Why
> > should we keep No-Op for all?
> >
> > On Wed, Dec 5, 2018 at 3:20 PM Павлухин Иван <vololo100@gmail.com>
> wrote:
> >
> > > Anton,
> > >
> > > Yes I meant that patch. And I would like to respell a name "massive
> > > no-op handler restore" to "use no-op failure handler only where it is
> > > assumed".
> > > ср, 5 дек. 2018 г. в 15:09, Dmitriy Pavlov <dpavlov@apache.org>:
> > > >
> > > > Dmitrii Ryabov explained these tests are perfectly ok to have
> failures
> > as
> > > > these tests do test failures.
> > > >
> > > > Anton, there is no reason to revert other's contributions because you
> > > know
> > > > how to do things better. A lot of people can do things better than
> me.
> > > > Should we revert everything I've contributed? I hope - no.
> > > >
> > > > If you can do things better, just commit further improvements. And I
> > will
> > > > be happy if you contribute some improvements later.
> > > >
> > > > If you would like to revert by veto, please justify your intent. If
> you
> > > > would discuss it with all community, please feel free to convince me
> > and
> > > > others.
> > > >
> > > > ср, 5 дек. 2018 г. в 14:53, Павлухин Иван <vololo100@gmail.com>:
> > > >
> > > > > Hi Anton,
> > > > >
> > > > > Could you please summarize what does aforementioned patch made
> really
> > > > > worse?
> > > > >
> > > > > As I see, the patch added a very good thing -- meaningful failure
> > > > > handler in tests. And I think it is really important. But was is
> the
> > > > > harm and does it overweight positive result? And why?
> > > > > ср, 5 дек. 2018 г. в 14:03, Anton Vinogradov <av@apache.org>:
> > > > > >
> > > > > > 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
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Best regards,
> > > > > Ivan Pavlukhin
> > > > >
> > >
> > >
> > >
> > > --
> > > Best regards,
> > > Ivan Pavlukhin
> > >
> >
>

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