Skip to content
Snippets Groups Projects
Commit 09601cc4 authored by Damon Kohler's avatar Damon Kohler
Browse files

Fixes bug in GraphName where a trailing slash would be added if an empty name...

Fixes bug in GraphName where a trailing slash would be added if an empty name was joined to a non-empty name.
Fixes flaky tests in MasterRegistrationManagerImplTest.
parent 9e205334
Branches
Tags
No related merge requests found
...@@ -17,14 +17,13 @@ ...@@ -17,14 +17,13 @@
package org.ros.internal.node.server.master; package org.ros.internal.node.server.master;
import com.google.common.base.Preconditions; import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Sets; import com.google.common.collect.Sets;
import org.ros.master.client.TopicSystemState; import org.ros.master.client.TopicSystemState;
import org.ros.namespace.GraphName; import org.ros.namespace.GraphName;
import org.ros.node.topic.Subscriber; import org.ros.node.topic.Subscriber;
import java.util.List;
import java.util.Set; import java.util.Set;
/** /**
...@@ -117,8 +116,8 @@ public class TopicRegistrationInfo { ...@@ -117,8 +116,8 @@ public class TopicRegistrationInfo {
* *
* @return an immutable list of publishers * @return an immutable list of publishers
*/ */
public List<NodeRegistrationInfo> getPublishers() { public Set<NodeRegistrationInfo> getPublishers() {
return ImmutableList.copyOf(publishers); return ImmutableSet.copyOf(publishers);
} }
/** /**
...@@ -153,8 +152,8 @@ public class TopicRegistrationInfo { ...@@ -153,8 +152,8 @@ public class TopicRegistrationInfo {
* *
* @return an immutable list of publishers * @return an immutable list of publishers
*/ */
public List<NodeRegistrationInfo> getSubscribers() { public Set<NodeRegistrationInfo> getSubscribers() {
return ImmutableList.copyOf(subscribers); return ImmutableSet.copyOf(subscribers);
} }
/** /**
......
...@@ -10,7 +10,6 @@ import static org.junit.Assert.assertTrue; ...@@ -10,7 +10,6 @@ import static org.junit.Assert.assertTrue;
import static org.mockito.Mockito.mock; import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verify;
import com.google.common.collect.Lists;
import com.google.common.collect.Sets; import com.google.common.collect.Sets;
import org.junit.Before; import org.junit.Before;
...@@ -64,7 +63,7 @@ public class MasterRegistrationManagerImplTest { ...@@ -64,7 +63,7 @@ public class MasterRegistrationManagerImplTest {
// Make sure only publisher in the topic is the topic we got for the node // Make sure only publisher in the topic is the topic we got for the node
// name // name
assertEquals(Lists.newArrayList(node), topic.getPublishers()); assertEquals(Sets.newHashSet(node), topic.getPublishers());
// No attempt for node shutdown // No attempt for node shutdown
verify(registrationListener, Mockito.never()).onNodeReplacement(node); verify(registrationListener, Mockito.never()).onNodeReplacement(node);
...@@ -97,7 +96,7 @@ public class MasterRegistrationManagerImplTest { ...@@ -97,7 +96,7 @@ public class MasterRegistrationManagerImplTest {
// Make sure only subscriber in the topic is the topic we got for the node // Make sure only subscriber in the topic is the topic we got for the node
// name // name
assertEquals(Lists.newArrayList(node), topic.getSubscribers()); assertEquals(Sets.newHashSet(node), topic.getSubscribers());
// No attempt for node shutdown // No attempt for node shutdown
verify(registrationListener, Mockito.never()).onNodeReplacement(node); verify(registrationListener, Mockito.never()).onNodeReplacement(node);
...@@ -383,7 +382,7 @@ public class MasterRegistrationManagerImplTest { ...@@ -383,7 +382,7 @@ public class MasterRegistrationManagerImplTest {
assertEquals(Sets.newHashSet(topicPublisher2), node2.getPublishers()); assertEquals(Sets.newHashSet(topicPublisher2), node2.getPublishers());
// Both publishers in topic // Both publishers in topic
assertEquals(Lists.newArrayList(node1, node2), topicPublisher1.getPublishers()); assertEquals(Sets.newHashSet(node1, node2), topicPublisher1.getPublishers());
// No attempt for node shutdown // No attempt for node shutdown
verify(registrationListener, Mockito.never()).onNodeReplacement(node1); verify(registrationListener, Mockito.never()).onNodeReplacement(node1);
...@@ -497,7 +496,7 @@ public class MasterRegistrationManagerImplTest { ...@@ -497,7 +496,7 @@ public class MasterRegistrationManagerImplTest {
// and the node will show this published topic. // and the node will show this published topic.
assertTrue(topic1.getPublishers().isEmpty()); assertTrue(topic1.getPublishers().isEmpty());
assertEquals(Sets.newHashSet(topic2), node.getPublishers()); assertEquals(Sets.newHashSet(topic2), node.getPublishers());
assertEquals(Lists.newArrayList(node), topic2.getPublishers()); assertEquals(Sets.newHashSet(node), topic2.getPublishers());
} }
/** /**
...@@ -559,7 +558,7 @@ public class MasterRegistrationManagerImplTest { ...@@ -559,7 +558,7 @@ public class MasterRegistrationManagerImplTest {
// and the node will show this subscribed topic. // and the node will show this subscribed topic.
assertTrue(topic1.getSubscribers().isEmpty()); assertTrue(topic1.getSubscribers().isEmpty());
assertEquals(Sets.newHashSet(topic2), node.getSubscribers()); assertEquals(Sets.newHashSet(topic2), node.getSubscribers());
assertEquals(Lists.newArrayList(node), topic2.getSubscribers()); assertEquals(Sets.newHashSet(node), topic2.getSubscribers());
} }
/** /**
......
...@@ -271,6 +271,9 @@ public class GraphName { ...@@ -271,6 +271,9 @@ public class GraphName {
if (isRoot()) { if (isRoot()) {
return other.toGlobal(); return other.toGlobal();
} }
if (other.isEmpty()) {
return this;
}
return new GraphName(toString() + SEPARATOR + other.toString()); return new GraphName(toString() + SEPARATOR + other.toString());
} }
......
...@@ -175,6 +175,7 @@ public class GraphNameTest { ...@@ -175,6 +175,7 @@ public class GraphNameTest {
public void testJoin() { public void testJoin() {
assertEquals(GraphName.of("/bar"), GraphName.of("/").join(GraphName.of("bar"))); assertEquals(GraphName.of("/bar"), GraphName.of("/").join(GraphName.of("bar")));
assertEquals(GraphName.of("bar"), GraphName.of("").join(GraphName.of("bar"))); assertEquals(GraphName.of("bar"), GraphName.of("").join(GraphName.of("bar")));
assertEquals(GraphName.of("bar"), GraphName.of("bar").join(GraphName.of("")));
assertEquals(GraphName.of("foo/bar"), GraphName.of("foo").join(GraphName.of("bar"))); assertEquals(GraphName.of("foo/bar"), GraphName.of("foo").join(GraphName.of("bar")));
assertEquals(GraphName.of("/foo/bar"), GraphName.of("/foo").join(GraphName.of("bar"))); assertEquals(GraphName.of("/foo/bar"), GraphName.of("/foo").join(GraphName.of("bar")));
assertEquals(GraphName.of("/bar"), GraphName.of("/foo").join(GraphName.of("/bar"))); assertEquals(GraphName.of("/bar"), GraphName.of("/foo").join(GraphName.of("/bar")));
...@@ -201,5 +202,4 @@ public class GraphNameTest { ...@@ -201,5 +202,4 @@ public class GraphNameTest {
assertTrue(latch.await(1, TimeUnit.SECONDS)); assertTrue(latch.await(1, TimeUnit.SECONDS));
assertEquals(sampleSize, anonymousGraphNames.size()); assertEquals(sampleSize, anonymousGraphNames.size());
} }
} }
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Please register or to comment