From 09601cc4cd276e7287e172a723714948fb759847 Mon Sep 17 00:00:00 2001 From: Damon Kohler <damonkohler@google.com> Date: Tue, 24 Jul 2012 15:43:54 +0200 Subject: [PATCH] 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. --- .../node/server/master/TopicRegistrationInfo.java | 11 +++++------ .../master/MasterRegistrationManagerImplTest.java | 11 +++++------ .../src/main/java/org/ros/namespace/GraphName.java | 3 +++ .../test/java/org/ros/namespace/GraphNameTest.java | 2 +- 4 files changed, 14 insertions(+), 13 deletions(-) diff --git a/rosjava/src/main/java/org/ros/internal/node/server/master/TopicRegistrationInfo.java b/rosjava/src/main/java/org/ros/internal/node/server/master/TopicRegistrationInfo.java index 236dda78..57fe543d 100644 --- a/rosjava/src/main/java/org/ros/internal/node/server/master/TopicRegistrationInfo.java +++ b/rosjava/src/main/java/org/ros/internal/node/server/master/TopicRegistrationInfo.java @@ -17,14 +17,13 @@ package org.ros.internal.node.server.master; 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 org.ros.master.client.TopicSystemState; import org.ros.namespace.GraphName; import org.ros.node.topic.Subscriber; -import java.util.List; import java.util.Set; /** @@ -117,8 +116,8 @@ public class TopicRegistrationInfo { * * @return an immutable list of publishers */ - public List<NodeRegistrationInfo> getPublishers() { - return ImmutableList.copyOf(publishers); + public Set<NodeRegistrationInfo> getPublishers() { + return ImmutableSet.copyOf(publishers); } /** @@ -153,8 +152,8 @@ public class TopicRegistrationInfo { * * @return an immutable list of publishers */ - public List<NodeRegistrationInfo> getSubscribers() { - return ImmutableList.copyOf(subscribers); + public Set<NodeRegistrationInfo> getSubscribers() { + return ImmutableSet.copyOf(subscribers); } /** diff --git a/rosjava/src/test/java/org/ros/internal/node/server/master/MasterRegistrationManagerImplTest.java b/rosjava/src/test/java/org/ros/internal/node/server/master/MasterRegistrationManagerImplTest.java index 12d07f93..eff7334c 100644 --- a/rosjava/src/test/java/org/ros/internal/node/server/master/MasterRegistrationManagerImplTest.java +++ b/rosjava/src/test/java/org/ros/internal/node/server/master/MasterRegistrationManagerImplTest.java @@ -10,7 +10,6 @@ import static org.junit.Assert.assertTrue; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; -import com.google.common.collect.Lists; import com.google.common.collect.Sets; import org.junit.Before; @@ -64,7 +63,7 @@ public class MasterRegistrationManagerImplTest { // Make sure only publisher in the topic is the topic we got for the node // name - assertEquals(Lists.newArrayList(node), topic.getPublishers()); + assertEquals(Sets.newHashSet(node), topic.getPublishers()); // No attempt for node shutdown verify(registrationListener, Mockito.never()).onNodeReplacement(node); @@ -97,7 +96,7 @@ public class MasterRegistrationManagerImplTest { // Make sure only subscriber in the topic is the topic we got for the node // name - assertEquals(Lists.newArrayList(node), topic.getSubscribers()); + assertEquals(Sets.newHashSet(node), topic.getSubscribers()); // No attempt for node shutdown verify(registrationListener, Mockito.never()).onNodeReplacement(node); @@ -383,7 +382,7 @@ public class MasterRegistrationManagerImplTest { assertEquals(Sets.newHashSet(topicPublisher2), node2.getPublishers()); // Both publishers in topic - assertEquals(Lists.newArrayList(node1, node2), topicPublisher1.getPublishers()); + assertEquals(Sets.newHashSet(node1, node2), topicPublisher1.getPublishers()); // No attempt for node shutdown verify(registrationListener, Mockito.never()).onNodeReplacement(node1); @@ -497,7 +496,7 @@ public class MasterRegistrationManagerImplTest { // and the node will show this published topic. assertTrue(topic1.getPublishers().isEmpty()); 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 { // and the node will show this subscribed topic. assertTrue(topic1.getSubscribers().isEmpty()); assertEquals(Sets.newHashSet(topic2), node.getSubscribers()); - assertEquals(Lists.newArrayList(node), topic2.getSubscribers()); + assertEquals(Sets.newHashSet(node), topic2.getSubscribers()); } /** diff --git a/rosjava_bootstrap/src/main/java/org/ros/namespace/GraphName.java b/rosjava_bootstrap/src/main/java/org/ros/namespace/GraphName.java index 4892cb25..a58d32b3 100644 --- a/rosjava_bootstrap/src/main/java/org/ros/namespace/GraphName.java +++ b/rosjava_bootstrap/src/main/java/org/ros/namespace/GraphName.java @@ -271,6 +271,9 @@ public class GraphName { if (isRoot()) { return other.toGlobal(); } + if (other.isEmpty()) { + return this; + } return new GraphName(toString() + SEPARATOR + other.toString()); } diff --git a/rosjava_bootstrap/src/test/java/org/ros/namespace/GraphNameTest.java b/rosjava_bootstrap/src/test/java/org/ros/namespace/GraphNameTest.java index 4f7b99b2..ac5217a1 100644 --- a/rosjava_bootstrap/src/test/java/org/ros/namespace/GraphNameTest.java +++ b/rosjava_bootstrap/src/test/java/org/ros/namespace/GraphNameTest.java @@ -175,6 +175,7 @@ public class GraphNameTest { 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("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("/bar"), GraphName.of("/foo").join(GraphName.of("/bar"))); @@ -201,5 +202,4 @@ public class GraphNameTest { assertTrue(latch.await(1, TimeUnit.SECONDS)); assertEquals(sampleSize, anonymousGraphNames.size()); } - } -- GitLab