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.mashen...@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 <a...@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 <somefire...@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 dpav...@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.mashen...@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" < > a...@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 <a...@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 < > somefire...@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 <dpav...@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 <a...@apache.org>: > > > > > >> > > > > > > >> > > Dmitrii, > > > > > >> > > > > > > > >> > > Could you please explain the reason of explicit set of 100+ > > > > > >> > > NoOpFailureHandlers? > > > > > >> > > > > > > > >> > > > > > > > >> > > вт, 4 дек. 2018 г. в 19:12, Dmitrii Ryabov < > > > somefire...@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 >
