some fixups for mirror of remote slobrok map:

* require symmetrical register/unregister calls
* modernize a bit
* add unit test for ServiceMapMirror
* use common mock listener in unit tests
* add an explicit shutdown() when removing a RemoteSlobrok instance,
  and perform clear() after canceling all RPC requests and tasks

3 files changed, 143 insertions, 70 deletions

diff --git a/slobrok/src/tests/service_map_mirror/CMakeLists.txt b/slobrok/src/tests/service_map_mirror/CMakeLists.txt
new file mode 100644
index 00000000000..2af34702068
--- /dev/null
+++ b/slobrok/src/tests/service_map_mirror/CMakeLists.txt
@@ -0,0 +1,9 @@
+# Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+vespa_add_executable(slobrok_service_map_mirror_test_app TEST
+    SOURCES
+    service_map_mirror_test.cpp
+    DEPENDS
+    slobrok_slobrokserver
+    GTest::GTest
+)
+vespa_add_test(NAME slobrok_service_map_mirror_test_app COMMAND slobrok_service_map_mirror_test_app)
diff --git a/slobrok/src/tests/service_map_mirror/service_map_mirror_test.cpp b/slobrok/src/tests/service_map_mirror/service_map_mirror_test.cpp
new file mode 100644
index 00000000000..b1d6c3fb1c1
--- /dev/null
+++ b/slobrok/src/tests/service_map_mirror/service_map_mirror_test.cpp
@@ -0,0 +1,101 @@
+// Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
+
+#include <vespa/slobrok/server/mock_map_listener.h>
+#include <vespa/slobrok/server/service_map_mirror.h>
+#include <vespa/vespalib/gtest/gtest.h>
+#include <vespa/vespalib/util/stringfmt.h>
+#include <map>
+
+using namespace vespalib;
+using namespace slobrok;
+using vespalib::make_string_short::fmt;
+
+using Map = std::map<vespalib::string, vespalib::string>;
+
+Map dump(const ServiceMapMirror &history) {
+    Map result;
+    for (const auto & entry : history.allMappings()) {
+        result[entry.name] = entry.spec;
+    }
+    return result;
+}
+
+
+void addTo(ServiceMapMirror &target, const ServiceMapping &mapping) {
+    auto cur = target.currentGeneration();
+    std::vector<vespalib::string> removes = {};
+    ServiceMappingList updates = { mapping };
+    auto nxt = cur;
+    nxt.add();
+    MapDiff diff{cur, removes, updates, nxt};
+    target.apply(diff);
+}
+
+void removeFrom(ServiceMapMirror &target, const ServiceMapping &mapping) {
+    auto cur = target.currentGeneration();
+    std::vector<vespalib::string> removes = { mapping.name };
+    ServiceMappingList updates = { };
+    auto nxt = cur;
+    nxt.add();
+    MapDiff diff{cur, removes, updates, nxt};
+    target.apply(diff);
+}
+
+TEST(ServiceMapMirrorTest, empty_inspection) {
+    ServiceMapMirror mirror;
+    auto bar = dump(mirror);
+    EXPECT_TRUE(bar.empty());
+
+    MockMapListener observer;
+    mirror.registerListener(observer);
+    mirror.unregisterListener(observer);
+    EXPECT_EQ(observer.last_event, MockEvent::NONE);
+}
+
+TEST(ServiceMapMirrorTest, full_inspection) {
+    ServiceMapMirror mirror;
+    MockMapListener observer;
+    mirror.registerListener(observer);
+    for (int i = 0; i < 1984; ++i) {
+        EXPECT_EQ(mirror.currentGeneration(), GenCnt(i));
+        auto name = fmt("key/%d/name", i);
+        auto spec = fmt("tcp/host%d.domain.tld:19099", 10000+i);
+        ServiceMapping toAdd{name, spec};
+        addTo(mirror, toAdd);
+        EXPECT_EQ(observer.last_event, MockEvent::ADD);
+        EXPECT_EQ(observer.last_add, toAdd);
+    }
+    EXPECT_EQ(mirror.currentGeneration(), GenCnt(1984));
+    ServiceMapping toRemove{"key/666/name", "tcp/host10666.domain.tld:19099"};
+    removeFrom(mirror, toRemove);
+    EXPECT_EQ(observer.last_event, MockEvent::REMOVE);
+    EXPECT_EQ(observer.last_remove, toRemove);
+    EXPECT_EQ(mirror.currentGeneration(), GenCnt(1985));
+
+    ServiceMapping oldMapping{"key/1969/name", "tcp/host11969.domain.tld:19099"};
+    ServiceMapping toUpdate{"key/1969/name", "tcp/woodstock:19069"};
+    addTo(mirror, toUpdate);
+    EXPECT_EQ(observer.last_event, MockEvent::UPDATE);
+    EXPECT_EQ(observer.last_remove, oldMapping);
+    EXPECT_EQ(observer.last_add, toUpdate);
+    EXPECT_EQ(mirror.currentGeneration(), GenCnt(1986));
+
+    auto map = dump(mirror);
+    EXPECT_FALSE(map.contains("foo"));
+    EXPECT_TRUE(map.contains("key/0/name"));
+    EXPECT_FALSE(map.contains("key/666/name"));
+    EXPECT_TRUE(map.contains("key/1983/name"));
+    EXPECT_FALSE(map.contains("key/1984/name"));
+    EXPECT_TRUE(map.contains("key/1969/name"));
+    EXPECT_EQ(map["key/0/name"], "tcp/host10000.domain.tld:19099");
+    EXPECT_EQ(map["key/123/name"], "tcp/host10123.domain.tld:19099");
+    EXPECT_EQ(map["key/1983/name"], "tcp/host11983.domain.tld:19099");
+    EXPECT_EQ(map["key/1969/name"], "tcp/woodstock:19069");
+    EXPECT_EQ(map.size(), 1983ul);
+
+    mirror.unregisterListener(observer);
+}
+
+
+GTEST_MAIN_RUN_ALL_TESTS()
+
diff --git a/slobrok/src/tests/union_service_map/union_service_map_test.cpp b/slobrok/src/tests/union_service_map/union_service_map_test.cpp
index 5f1f70fb9fb..30db95324bb 100644
--- a/slobrok/src/tests/union_service_map/union_service_map_test.cpp
+++ b/slobrok/src/tests/union_service_map/union_service_map_test.cpp
@@ -1,5 +1,6 @@
 // Copyright Verizon Media. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
 
+#include <vespa/slobrok/server/mock_map_listener.h>
 #include <vespa/slobrok/server/union_service_map.h>
 #include <vespa/vespalib/gtest/gtest.h>
 #include <vespa/vespalib/util/stringfmt.h>
@@ -8,70 +9,32 @@ using namespace vespalib;
 using namespace slobrok;
 using vespalib::make_string_short::fmt;
 
-enum class Event { NONE, ADD, REMOVE, UPDATE };
-
-struct MapObserver : public MapListener {
-    MapObserver();
-    virtual ~MapObserver();
-    void add(const ServiceMapping &mapping) override;
-    void remove(const ServiceMapping &mapping) override;
-    void update(const ServiceMapping &old_mapping,
-                const ServiceMapping &new_mapping) override;
-
-    Event last_event = Event::NONE;
-    ServiceMapping last_add = {{}, {}};
-    ServiceMapping last_remove = {{}, {}};
-
-    void clear() { last_event = Event::NONE; }
-};
-
-MapObserver::MapObserver() = default;
-MapObserver::~MapObserver() = default;
-
-void MapObserver::add(const ServiceMapping &mapping) {
-    last_event = Event::ADD;
-    last_add = mapping;
-}
-
-void MapObserver::remove(const ServiceMapping &mapping) {
-    last_event = Event::REMOVE;
-    last_remove = mapping;
-}
-
-void MapObserver::update(const ServiceMapping &old_mapping,
-                         const ServiceMapping &new_mapping)
-{
-    last_event = Event::UPDATE;
-    last_remove = old_mapping;
-    last_add = new_mapping;
-}
-
 TEST(UnionServiceMapTest, forwards_simple_requests) {
     ProxyMapSource source;
     UnionServiceMap unionizer;
-    MapObserver observer;
+    MockMapListener observer;
     unionizer.registerListener(observer);
     source.registerListener(unionizer);
 
-    EXPECT_EQ(observer.last_event, Event::NONE);
+    EXPECT_EQ(observer.last_event, MockEvent::NONE);
 
     ServiceMapping one{"foo/1", "bar/1"};
     source.add(one);
-    EXPECT_EQ(observer.last_event, Event::ADD);
+    EXPECT_EQ(observer.last_event, MockEvent::ADD);
     EXPECT_EQ(observer.last_add, one);
     ServiceMapping two{"foo/2", "bar/2"};
     source.add(two);
-    EXPECT_EQ(observer.last_event, Event::ADD);
+    EXPECT_EQ(observer.last_event, MockEvent::ADD);
     EXPECT_EQ(observer.last_add, two);
 
     source.remove(one);
-    EXPECT_EQ(observer.last_event, Event::REMOVE);
+    EXPECT_EQ(observer.last_event, MockEvent::REMOVE);
     EXPECT_EQ(observer.last_remove, one);
 
     ServiceMapping two_q{"foo/2", "qux/2"};
     source.update(two, two_q);
     // update implemented ass remove+add:
-    EXPECT_EQ(observer.last_event, Event::ADD);
+    EXPECT_EQ(observer.last_event, MockEvent::ADD);
     EXPECT_EQ(observer.last_remove, two);
     EXPECT_EQ(observer.last_add, two_q);
 }
@@ -81,47 +44,47 @@ TEST(UnionServiceMapTest, handles_refcount) {
     ProxyMapSource source2;
     ProxyMapSource source3;
     UnionServiceMap unionizer;
-    MapObserver observer;
+    MockMapListener observer;
     unionizer.registerListener(observer);
     source1.registerListener(unionizer);
     source2.registerListener(unionizer);
     source3.registerListener(unionizer);
 
-    EXPECT_EQ(observer.last_event, Event::NONE);
+    EXPECT_EQ(observer.last_event, MockEvent::NONE);
     ServiceMapping one{"foo/1", "bar/1"};
     source1.add(one);
-    EXPECT_EQ(observer.last_event, Event::ADD);
+    EXPECT_EQ(observer.last_event, MockEvent::ADD);
     EXPECT_EQ(observer.last_add, one);
     observer.clear();
-    EXPECT_EQ(observer.last_event, Event::NONE);
+    EXPECT_EQ(observer.last_event, MockEvent::NONE);
     source2.add(one);
-    EXPECT_EQ(observer.last_event, Event::NONE);
+    EXPECT_EQ(observer.last_event, MockEvent::NONE);
     source3.add(one);
-    EXPECT_EQ(observer.last_event, Event::NONE);
+    EXPECT_EQ(observer.last_event, MockEvent::NONE);
     ServiceMapping two{"foo/2", "bar/2"};
     source1.add(two);
-    EXPECT_EQ(observer.last_event, Event::ADD);
+    EXPECT_EQ(observer.last_event, MockEvent::ADD);
     EXPECT_EQ(observer.last_add, two);
     observer.clear();
-    EXPECT_EQ(observer.last_event, Event::NONE);
+    EXPECT_EQ(observer.last_event, MockEvent::NONE);
     source2.add(two);
-    EXPECT_EQ(observer.last_event, Event::NONE);
+    EXPECT_EQ(observer.last_event, MockEvent::NONE);
 
     source1.remove(one);
-    EXPECT_EQ(observer.last_event, Event::NONE);
+    EXPECT_EQ(observer.last_event, MockEvent::NONE);
     source2.remove(one);
-    EXPECT_EQ(observer.last_event, Event::NONE);
+    EXPECT_EQ(observer.last_event, MockEvent::NONE);
 
     source1.remove(two);
-    EXPECT_EQ(observer.last_event, Event::NONE);
+    EXPECT_EQ(observer.last_event, MockEvent::NONE);
     source2.remove(two);
-    EXPECT_EQ(observer.last_event, Event::REMOVE);
+    EXPECT_EQ(observer.last_event, MockEvent::REMOVE);
     EXPECT_EQ(observer.last_remove, two);
 
     observer.clear();
-    EXPECT_EQ(observer.last_event, Event::NONE);
+    EXPECT_EQ(observer.last_event, MockEvent::NONE);
     source3.remove(one);
-    EXPECT_EQ(observer.last_event, Event::REMOVE);
+    EXPECT_EQ(observer.last_event, MockEvent::REMOVE);
     EXPECT_EQ(observer.last_remove, one);
 }
 
@@ -130,48 +93,48 @@ TEST(UnionServiceMapTest, handles_conflicts) {
     ProxyMapSource source2;
     ProxyMapSource source3;
     UnionServiceMap unionizer;
-    MapObserver observer;
+    MockMapListener observer;
     unionizer.registerListener(observer);
     source1.registerListener(unionizer);
     source2.registerListener(unionizer);
     source3.registerListener(unionizer);
 
-    EXPECT_EQ(observer.last_event, Event::NONE);
+    EXPECT_EQ(observer.last_event, MockEvent::NONE);
     ServiceMapping one{"foo/1", "bar/1"};
     source1.add(one);
-    EXPECT_EQ(observer.last_event, Event::ADD);
+    EXPECT_EQ(observer.last_event, MockEvent::ADD);
     EXPECT_EQ(observer.last_add, one);
     observer.clear();
     source2.add(one);
-    EXPECT_EQ(observer.last_event, Event::NONE);
+    EXPECT_EQ(observer.last_event, MockEvent::NONE);
 
     ServiceMapping two{"foo/2", "bar/2"};
     source1.add(two);
-    EXPECT_EQ(observer.last_event, Event::ADD);
+    EXPECT_EQ(observer.last_event, MockEvent::ADD);
     EXPECT_EQ(observer.last_add, two);
     observer.clear();
     source2.add(two);
-    EXPECT_EQ(observer.last_event, Event::NONE);
+    EXPECT_EQ(observer.last_event, MockEvent::NONE);
 
     ServiceMapping one_q{"foo/1", "qux/1"};
     source3.add(one_q);
-    EXPECT_EQ(observer.last_event, Event::REMOVE);
+    EXPECT_EQ(observer.last_event, MockEvent::REMOVE);
     EXPECT_EQ(observer.last_remove, one);
 
     ServiceMapping two_q{"foo/2", "qux/2"};
     source3.add(two_q);
-    EXPECT_EQ(observer.last_event, Event::REMOVE);
+    EXPECT_EQ(observer.last_event, MockEvent::REMOVE);
     EXPECT_EQ(observer.last_remove, two);
 
     source3.remove(one_q);
-    EXPECT_EQ(observer.last_event, Event::ADD);
+    EXPECT_EQ(observer.last_event, MockEvent::ADD);
     EXPECT_EQ(observer.last_add, one);
 
     observer.clear();
     source1.remove(two);
-    EXPECT_EQ(observer.last_event, Event::NONE);
+    EXPECT_EQ(observer.last_event, MockEvent::NONE);
     source2.remove(two);
-    EXPECT_EQ(observer.last_event, Event::ADD);
+    EXPECT_EQ(observer.last_event, MockEvent::ADD);
     EXPECT_EQ(observer.last_add, two_q);
 }