You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ignite.apache.org by Иван Федотов <iv...@gmail.com> on 2018/07/20 12:00:48 UTC
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.
ivanan639@gmail.com
Re: Code duplicates in ssh tests
Posted by Dmitry Pavlov <dp...@gmail.com>.
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, Иван Федотов <iv...@gmail.com>:
> 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 <dp...@gmail.com>:
>
> > 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, Иван Федотов <iv...@gmail.com>:
> >
> > > 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.
> > >
> > > ivanan639@gmail.com
> > >
> >
>
>
>
> --
> Ivan Fedotov.
>
> ivanan639@gmail.com
>
Re: Code duplicates in ssh tests
Posted by Иван Федотов <iv...@gmail.com>.
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 <dp...@gmail.com>:
> 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, Иван Федотов <iv...@gmail.com>:
>
> > 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.
> >
> > ivanan639@gmail.com
> >
>
--
Ivan Fedotov.
ivanan639@gmail.com
Re: Code duplicates in ssh tests
Posted by Dmitry Pavlov <dp...@gmail.com>.
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, Иван Федотов <iv...@gmail.com>:
> 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.
>
> ivanan639@gmail.com
>