Hi Maxim, Thank you.
To my mind stopAllGrids call should be kept in afterTestsStop(). If developer forgot to call super(), he will see exception from beforeTestsStart() added by you. If you think patch is ready to be reviewed, please change JIRA status to "Patch Available". Optionally you can create Upsource review. It would be easier for multiple reviewers to handle this patch. This test framework is used by all Igniters so Upsource would be good option (https://reviews.ignite.apache.org/ ). Sincerely, Dmitriy Pavlov чт, 22 февр. 2018 г. в 13:44, Maxim Muzafarov <maxmu...@gmail.com>: > Hi all, > > I've made some changes based on our previous discusstions, please see PR > [1]: > 1) Remove duplicated code for stopGrid (by index and by name); > 2) Add new method that thows exception if not all grids haven't stopped > correctly; > 3) Change tests that have been affected by this changes; > > Also, I have some thoughts for clarification: > 1) beforeTestsStart() - I expect here in subtests that grids are not > started yet. Am I right? > 2) I think we should call stopAllGrids in tearDown method. So, if in some > cases we'll override afterTestsStop in subclasses and forgot to call > super() it won't lead us to exception. > > [1] https://github.com/apache/ignite/pull/3542 > [2] https://ci.ignite.apache.org/viewModification.html?modId=717275 > [3] https://issues.apache.org/jira/browse/IGNITE-6842 > > > ср, 7 февр. 2018 г. в 18:28, Maxim Muzafarov <maxmu...@gmail.com>: > > > Thank you all, > > > > I'll add this comment's for JIRA ticket, if you don't mind. > > > > ср, 7 февр. 2018 г. в 15:16, Dmitry Pavlov <dpavlov....@gmail.com>: > > > >> Yes, this solution allows to cover both cases: > >> a) not stopped node from previous test and > >> b) allows to remove useless code that stops Ignite nodes from each test. > >> > >> ср, 7 февр. 2018 г. в 15:13, Anton Vinogradov <avinogra...@gridgain.com > >: > >> > >> > Maxim, > >> > > >> > We discussed with Dima privately, and decided > >> > > >> > 1) We have to assert that there is no alive nodes at > GridAbstractTest's > >> > beforeTestsStarted > >> > 2) We have to kill all alive nodes (without force) at > GridAbstractTest's > >> > afterTestsStopped > >> > 3) In case of any exceptions at #2 we have to see test error > >> > 4) We can get rid of all useless stopAllGrids at GridAbstractTest's > >> > subclasses. > >> > > >> > > >> > > >> > On Wed, Feb 7, 2018 at 2:32 PM, Dmitry Pavlov <dpavlov....@gmail.com> > >> > wrote: > >> > > >> > > > Let's just add stopAllGrids(flase) to GridAbstractTest 's > >> > > afterTestsStopped > >> > > method body. > >> > > > >> > > Can't agree with it becase implicit silent shutdown of nodes from > test > >> > > framework may cause a lot of bugs introduced to Ignite. > >> > > > >> > > I think stop+test fail should occur. > >> > > In that case author of incorrect test or not consistent Ignite > >> shutdown > >> > > will see problem. > >> > > > >> > > 'Fail fast' if always better than hidding real problem under > automatic > >> > > framework feature. > >> > > > >> > > ср, 7 февр. 2018 г. в 14:05, Anton Vinogradov < > >> avinogra...@gridgain.com > >> > >: > >> > > > >> > > > Hi all, > >> > > > > >> > > > > I've made some research about this problem and i think that in > >> > general > >> > > we > >> > > > > should move stopAllGrids method in GridAbstractTest class to > >> > > > > afterTestsStopped method with some changes. Am I right? > >> > > > Let's just add stopAllGrids(flase) to GridAbstractTest 's > >> > > > afterTestsStopped method > >> > > > body. > >> > > > > >> > > > > I have a question about stopAllGrids(boolean cancel) this > "cancel" > >> > > > That's just a flag means "do not wait for any operations finish" > >> > > > See no reason to set it true in that case. > >> > > > > >> > > > > If you delegate closing to afterTestsStopped this will affect > only > >> > > > > last test (method). > >> > > > The idea is to stop all nodes at last test's method finish. > >> > > > > >> > > > > Nodes that survive between tests can affect successive > >> > > > tests. > >> > > > Thats ok. we have a lot tests where nodes reusable between test's > >> > > methods. > >> > > > > >> > > > > >> > > > On Wed, Feb 7, 2018 at 1:20 PM, Dmitry Pavlov < > >> dpavlov....@gmail.com> > >> > > > wrote: > >> > > > > >> > > > > Hi Igniters, > >> > > > > > >> > > > > IMO nodes that survive between tests is not general practice and > >> I'm > >> > > not > >> > > > > sure is a best practice. I suggest to mark such tests with some > >> > method > >> > > > > overriden from AbstractTest. > >> > > > > > >> > > > > About cancel flag, please note stopAllGrids(boolean cancel) > >> > > cancel=false > >> > > > > allows to wait of checkpoint ends in case persistence enabled. > >> > > > > > >> > > > > I still suggest to avoid stopping any nodes by test, but > validate > >> not > >> > > > > stopped node exist and fail test instead of siltent implicit > >> actions. > >> > > > > > >> > > > > Sincerely, > >> > > > > Dmitriy Pavlov > >> > > > > > >> > > > > ср, 7 февр. 2018 г. в 13:04, Andrey Kuznetsov < > stku...@gmail.com > >> >: > >> > > > > > >> > > > > > Hi Maxim, > >> > > > > > > >> > > > > > Regarding your first question, the use of afterTestsStopped is > >> not > >> > > > enough > >> > > > > > to stop all nodes, since each individual test (method) can > start > >> > > custom > >> > > > > set > >> > > > > > of notes during its operation, and this very test should stop > >> all > >> > > those > >> > > > > > nodes. If you delegate closing to afterTestsStopped this will > >> > affect > >> > > > only > >> > > > > > last test (method). Nodes that survive between tests can > affect > >> > > > > successive > >> > > > > > tests. > >> > > > > > > >> > > > > > 2018-02-07 1:10 GMT+03:00 Maxim Muzafarov <maxmu...@gmail.com > >: > >> > > > > > > >> > > > > > > Hello, > >> > > > > > > > >> > > > > > > I've made some research about this problem and i think that > in > >> > > > general > >> > > > > we > >> > > > > > > should move stopAllGrids method in GridAbstractTest class to > >> > > > > > > afterTestsStopped method with some changes. Am I right? > >> > > > > > > > >> > > > > > > Also, I have a question about stopAllGrids(boolean cancel) > >> this > >> > > > > "cancel" > >> > > > > > > argument. Why in some cases we should interrupt ComputeJob > >> and in > >> > > > some > >> > > > > > > cases shouldn't? For example here: > >> > > > > > > IgniteBaselineAffinityTopologyActivationTest#afterTest > >> > > > > > > we call method stopAllGrids(false) this way. Why not "true" > >> > > argument > >> > > > > > > instead? > >> > > > > > > > >> > > > > > > > >> > > > > > > -- > >> > > > > > Best regards, > >> > > > > > Andrey Kuznetsov. > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > >> > > >
