Re: Code duplicates in ssh tests
Ok, I agree here, that we can remove one test. Feel free to create an issue and PR if nobody else mind. Let us wait at least until Mon 23 Jul before merge. пт, 20 июл. 2018 г. в 17:57, Иван Федотов : > Hi, Dmitry. > > I thought about elements order, but if we go deeper in > ignite.cluster().stopNodes() method, we can see that in ClusterIgniteImpl > [1] all nodes id will be collected in HashSet in forNodesIds method [2]. > > So I think that in this case it's not important what use initially, HashSet > or ArrayList. > > [1] > > https://github.com/apache/ignite/blob/master/modules/core/src/main/java/org/apache/ignite/internal/cluster/IgniteClusterImpl.java#L250 > [2] > > https://github.com/apache/ignite/blob/master/modules/core/src/main/java/org/apache/ignite/internal/cluster/ClusterGroupAdapter.java#L454 > > > 2018-07-20 16:52 GMT+03:00 Dmitry Pavlov : > > > Hi Ivan, > > > > I can suppose that it is related to elements order. Is it reasonable to > > keep 2 tests with 1 common mehod? One test will call this method with > > HashSet, and other with ArrayList. > > > > Sincerely, > > Dmitriy Pavlov > > > > пт, 20 июл. 2018 г. в 15:01, Иван Федотов : > > > > > Hi, Igniters! > > > > > > I’m working on ssh module and found some code duplicates in > > > IgniteProjectionStartStopRestartSelfTest. > > > > > > 1. Tests testRestartNodesByIds and testRestartNodesByIdsC are fully > > > duplicate themself [1]. I tried to found what differences should they > > have > > > and looked at similar tests: testStopNodesByIds and testStopNodesByIdsC > > > [2]. It relates to the second point. > > > > > > 2. The only difference is that in testStopNodesByIds we stop nodes by > > > passing HashSet of Ids and in testStopNodesByIdsC we stop by passing > > > ArrayList of Ids. In my opinion it does not matter, because stopNodes > > > methods have Collection as argument and we can pass to it both HashSet > > and > > > ArrayList. So, I think that code in these tests are also duplicate each > > > other. > > > > > > What do you think? Can we remove one of these tests in both cases? > > > > > > > > > [1] > > > > > > https://github.com/apache/ignite/blob/master/modules/ > > ssh/src/test/java/org/apache/ignite/internal/ > > IgniteProjectionStartStopRestartSelfTest.java#L878 > > > > > > [2] > > > > > > https://github.com/apache/ignite/blob/master/modules/ > > ssh/src/test/java/org/apache/ignite/internal/ > > IgniteProjectionStartStopRestartSelfTest.java#L659 > > > > > > > > > -- > > > Ivan Fedotov. > > > > > > ivanan...@gmail.com > > > > > > > > > -- > Ivan Fedotov. > > ivanan...@gmail.com >
Re: Code duplicates in ssh tests
Hi, Dmitry. I thought about elements order, but if we go deeper in ignite.cluster().stopNodes() method, we can see that in ClusterIgniteImpl [1] all nodes id will be collected in HashSet in forNodesIds method [2]. So I think that in this case it's not important what use initially, HashSet or ArrayList. [1] https://github.com/apache/ignite/blob/master/modules/core/src/main/java/org/apache/ignite/internal/cluster/IgniteClusterImpl.java#L250 [2] https://github.com/apache/ignite/blob/master/modules/core/src/main/java/org/apache/ignite/internal/cluster/ClusterGroupAdapter.java#L454 2018-07-20 16:52 GMT+03:00 Dmitry Pavlov : > Hi Ivan, > > I can suppose that it is related to elements order. Is it reasonable to > keep 2 tests with 1 common mehod? One test will call this method with > HashSet, and other with ArrayList. > > Sincerely, > Dmitriy Pavlov > > пт, 20 июл. 2018 г. в 15:01, Иван Федотов : > > > Hi, Igniters! > > > > I’m working on ssh module and found some code duplicates in > > IgniteProjectionStartStopRestartSelfTest. > > > > 1. Tests testRestartNodesByIds and testRestartNodesByIdsC are fully > > duplicate themself [1]. I tried to found what differences should they > have > > and looked at similar tests: testStopNodesByIds and testStopNodesByIdsC > > [2]. It relates to the second point. > > > > 2. The only difference is that in testStopNodesByIds we stop nodes by > > passing HashSet of Ids and in testStopNodesByIdsC we stop by passing > > ArrayList of Ids. In my opinion it does not matter, because stopNodes > > methods have Collection as argument and we can pass to it both HashSet > and > > ArrayList. So, I think that code in these tests are also duplicate each > > other. > > > > What do you think? Can we remove one of these tests in both cases? > > > > > > [1] > > > > https://github.com/apache/ignite/blob/master/modules/ > ssh/src/test/java/org/apache/ignite/internal/ > IgniteProjectionStartStopRestartSelfTest.java#L878 > > > > [2] > > > > https://github.com/apache/ignite/blob/master/modules/ > ssh/src/test/java/org/apache/ignite/internal/ > IgniteProjectionStartStopRestartSelfTest.java#L659 > > > > > > -- > > Ivan Fedotov. > > > > ivanan...@gmail.com > > > -- Ivan Fedotov. ivanan...@gmail.com
Re: Code duplicates in ssh tests
Hi Ivan, I can suppose that it is related to elements order. Is it reasonable to keep 2 tests with 1 common mehod? One test will call this method with HashSet, and other with ArrayList. Sincerely, Dmitriy Pavlov пт, 20 июл. 2018 г. в 15:01, Иван Федотов : > Hi, Igniters! > > I’m working on ssh module and found some code duplicates in > IgniteProjectionStartStopRestartSelfTest. > > 1. Tests testRestartNodesByIds and testRestartNodesByIdsC are fully > duplicate themself [1]. I tried to found what differences should they have > and looked at similar tests: testStopNodesByIds and testStopNodesByIdsC > [2]. It relates to the second point. > > 2. The only difference is that in testStopNodesByIds we stop nodes by > passing HashSet of Ids and in testStopNodesByIdsC we stop by passing > ArrayList of Ids. In my opinion it does not matter, because stopNodes > methods have Collection as argument and we can pass to it both HashSet and > ArrayList. So, I think that code in these tests are also duplicate each > other. > > What do you think? Can we remove one of these tests in both cases? > > > [1] > > https://github.com/apache/ignite/blob/master/modules/ssh/src/test/java/org/apache/ignite/internal/IgniteProjectionStartStopRestartSelfTest.java#L878 > > [2] > > https://github.com/apache/ignite/blob/master/modules/ssh/src/test/java/org/apache/ignite/internal/IgniteProjectionStartStopRestartSelfTest.java#L659 > > > -- > Ivan Fedotov. > > ivanan...@gmail.com >
Code duplicates in ssh tests
Hi, Igniters! I’m working on ssh module and found some code duplicates in IgniteProjectionStartStopRestartSelfTest. 1. Tests testRestartNodesByIds and testRestartNodesByIdsC are fully duplicate themself [1]. I tried to found what differences should they have and looked at similar tests: testStopNodesByIds and testStopNodesByIdsC [2]. It relates to the second point. 2. The only difference is that in testStopNodesByIds we stop nodes by passing HashSet of Ids and in testStopNodesByIdsC we stop by passing ArrayList of Ids. In my opinion it does not matter, because stopNodes methods have Collection as argument and we can pass to it both HashSet and ArrayList. So, I think that code in these tests are also duplicate each other. What do you think? Can we remove one of these tests in both cases? [1] https://github.com/apache/ignite/blob/master/modules/ssh/src/test/java/org/apache/ignite/internal/IgniteProjectionStartStopRestartSelfTest.java#L878 [2] https://github.com/apache/ignite/blob/master/modules/ssh/src/test/java/org/apache/ignite/internal/IgniteProjectionStartStopRestartSelfTest.java#L659 -- Ivan Fedotov. ivanan...@gmail.com
