Code duplicates in ssh tests

[Ivan Fedotov]

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.

[hidden email]

[Dmitriy 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, Иван Федотов <[hidden email]>:

> 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.
>
> [hidden email]
>

[Ivan Fedotov]

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 <[hidden email]>:

> 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, Иван Федотов <[hidden email]>:
>
> > 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.
> >
> > [hidden email]
> >
>

--
Ivan Fedotov.

[hidden email]

[Dmitriy Pavlov]

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, Иван Федотов <[hidden email]>:

> 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 <[hidden email]>:
>
> > 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, Иван Федотов <[hidden email]>:
> >
> > > 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.
> > >
> > > [hidden email]
> > >
> >
>
>
>
> --
> Ivan Fedotov.
>
> [hidden email]
>