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. >> > > > > > >> > > > > >> > > > >> > > >> > >> >
