From b5daae9d77f532f971de112ad32784d715c92e67 Mon Sep 17 00:00:00 2001 From: Dhrupad Bhardwaj Date: Fri, 30 Mar 2018 17:38:26 -0400 Subject: [PATCH 1/5] Unittests for inherited callbacks and configuration validator in updater.h --- src/store.h | 1 + src/updater.h | 2 ++ test/Makefile | 5 ++- test/updater_unittest.cc | 77 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 84 insertions(+), 1 deletion(-) create mode 100644 test/updater_unittest.cc diff --git a/src/store.h b/src/store.h index b21008db..61b7067e 100644 --- a/src/store.h +++ b/src/store.h @@ -93,6 +93,7 @@ class MetadataStore { private: friend class MetadataReporter; friend class MetadataStoreTest; + friend class UpdaterTest; std::map GetMetadataMap() const; void PurgeDeletedEntries(); diff --git a/src/updater.h b/src/updater.h index 01de5d5d..40df0923 100644 --- a/src/updater.h +++ b/src/updater.h @@ -115,6 +115,8 @@ class PollingMetadataUpdater : public MetadataUpdater { ~PollingMetadataUpdater(); protected: + friend class UpdaterTest; + bool ValidateConfiguration() const; void StartUpdater(); void StopUpdater(); diff --git a/test/Makefile b/test/Makefile index 30f3b9df..51bf89d3 100644 --- a/test/Makefile +++ b/test/Makefile @@ -49,7 +49,8 @@ TESTS=\ kubernetes_unittest \ resource_unittest \ store_unittest \ - time_unittest + time_unittest \ + updater_unittest GTEST_LIB=gtest_lib.a @@ -119,5 +120,7 @@ store_unittest: store_unittest.o $(SRC_DIR)/store.o $(SRC_DIR)/resource.o $(SRC_ $(CXX) $(LDFLAGS) $^ $(LDLIBS) -o $@ time_unittest: time_unittest.o $(SRC_DIR)/time.o $(CXX) $(LDFLAGS) $^ $(LDLIBS) -o $@ +updater_unittest: updater_unittest.o $(SRC_DIR)/updater.o $(SRC_DIR)/resource.o $(SRC_DIR)/store.o $(SRC_DIR)/configuration.o $(SRC_DIR)/logging.o $(SRC_DIR)/time.o $(SRC_DIR)/json.o + $(CXX) $(LDFLAGS) $^ $(LDLIBS) -o $@ .PHONY: all test clean purge diff --git a/test/updater_unittest.cc b/test/updater_unittest.cc new file mode 100644 index 00000000..5109745f --- /dev/null +++ b/test/updater_unittest.cc @@ -0,0 +1,77 @@ +#include "../src/configuration.h" +#include "../src/resource.h" +#include "../src/store.h" +#include "../src/updater.h" +#include "gtest/gtest.h" + +#include +#include + +namespace google { + +class UpdaterTest : public ::testing::Test { + protected: + // query_metadata function not needed to test callbacks. + UpdaterTest() : config(), store(config), updater( + config, &store, "Test", 60.0, nullptr) {} + + std::map GetMetadataMap() const { + return store.GetMetadataMap(); + } + + bool ValidateConfiguration() const { + return updater.ValidateConfiguration(); + } + + void UpdateMetadataCallback(MetadataUpdater::ResourceMetadata& result) { + updater.UpdateMetadataCallback(std::move(result)); + } + + void UpdateResourceCallback(MetadataUpdater::ResourceMetadata& result) { + updater.UpdateResourceCallback(std::move(result)); + } + + Configuration config; + MetadataStore store; + PollingMetadataUpdater updater; +}; + +namespace { + +TEST_F(UpdaterTest, ValidateConfiguration) { + EXPECT_TRUE(ValidateConfiguration()); +} + +TEST_F(UpdaterTest, UpdateMetadataCallbackTest) { + MetadataStore::Metadata m( + "test-version", + false, + std::chrono::system_clock::now(), + std::chrono::system_clock::now(), + json::object({{"f", json::string("test")}})); + MonitoredResource resource("test_resource", {}); + MetadataUpdater::ResourceMetadata metadata( + std::vector({"", "test-prefix"}), + resource, std::move(m)); + UpdateMetadataCallback(metadata); + const auto metadata_map = GetMetadataMap(); + EXPECT_EQ(1, metadata_map.size()); + EXPECT_EQ("test-version", metadata_map.at(resource).version); + EXPECT_EQ("{\"f\":\"test\"}", metadata_map.at(resource).metadata->ToString()); +} + +TEST_F(UpdaterTest, UpdateResourceCallbackTest) { + MetadataUpdater::ResourceMetadata metadata( + std::vector({"", "test-prefix"}), + MonitoredResource("test_resource", {}), + MetadataStore::Metadata::IGNORED() + ); + UpdateResourceCallback(metadata); + EXPECT_EQ(MonitoredResource("test_resource", {}), store.LookupResource("")); + EXPECT_EQ(MonitoredResource("test_resource", {}), + store.LookupResource("test-prefix")); +} + +} // namespace + +} // google From 766345268cc1d9b4c9938748a58545db3939af08 Mon Sep 17 00:00:00 2001 From: Dhrupad Bhardwaj Date: Sat, 31 Mar 2018 12:34:41 -0400 Subject: [PATCH 2/5] Address feedback --- test/updater_unittest.cc | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/test/updater_unittest.cc b/test/updater_unittest.cc index 5109745f..85d88867 100644 --- a/test/updater_unittest.cc +++ b/test/updater_unittest.cc @@ -23,12 +23,12 @@ class UpdaterTest : public ::testing::Test { return updater.ValidateConfiguration(); } - void UpdateMetadataCallback(MetadataUpdater::ResourceMetadata& result) { + void UpdateMetadataCallback(MetadataUpdater::ResourceMetadata&& result) { updater.UpdateMetadataCallback(std::move(result)); } - void UpdateResourceCallback(MetadataUpdater::ResourceMetadata& result) { - updater.UpdateResourceCallback(std::move(result)); + void UpdateResourceCallback(const MetadataUpdater::ResourceMetadata& result) { + updater.UpdateResourceCallback(result); } Configuration config; @@ -53,7 +53,7 @@ TEST_F(UpdaterTest, UpdateMetadataCallbackTest) { MetadataUpdater::ResourceMetadata metadata( std::vector({"", "test-prefix"}), resource, std::move(m)); - UpdateMetadataCallback(metadata); + UpdateMetadataCallback(std::move(metadata)); const auto metadata_map = GetMetadataMap(); EXPECT_EQ(1, metadata_map.size()); EXPECT_EQ("test-version", metadata_map.at(resource).version); @@ -67,7 +67,8 @@ TEST_F(UpdaterTest, UpdateResourceCallbackTest) { MetadataStore::Metadata::IGNORED() ); UpdateResourceCallback(metadata); - EXPECT_EQ(MonitoredResource("test_resource", {}), store.LookupResource("")); + EXPECT_EQ(MonitoredResource("test_resource", {}), + store.LookupResource("")); EXPECT_EQ(MonitoredResource("test_resource", {}), store.LookupResource("test-prefix")); } From d139dd886fc6d5019e662158febae49efce89350 Mon Sep 17 00:00:00 2001 From: Dhrupad Bhardwaj Date: Sat, 31 Mar 2018 14:07:19 -0400 Subject: [PATCH 3/5] Address feedback. Make GetMetadataMap in MetadataStore public --- src/store.h | 6 ++++-- src/updater.h | 2 ++ test/store_unittest.cc | 20 ++++++++------------ test/updater_unittest.cc | 40 +++++++++++++++++++++++----------------- 4 files changed, 37 insertions(+), 31 deletions(-) diff --git a/src/store.h b/src/store.h index 61b7067e..0fad6c15 100644 --- a/src/store.h +++ b/src/store.h @@ -74,6 +74,10 @@ class MetadataStore { MetadataStore(const Configuration& config); + // Returns a copy of the map containing the mapping from monitored + // resources to metadata associated with that resource. + std::map GetMetadataMap() const; + // Looks up the local resource map entry for a given resource id. // Throws an exception if the resource is not found. const MonitoredResource& LookupResource(const std::string& resource_id) const @@ -93,9 +97,7 @@ class MetadataStore { private: friend class MetadataReporter; friend class MetadataStoreTest; - friend class UpdaterTest; - std::map GetMetadataMap() const; void PurgeDeletedEntries(); const Configuration& config_; diff --git a/src/updater.h b/src/updater.h index 40df0923..8205f912 100644 --- a/src/updater.h +++ b/src/updater.h @@ -69,6 +69,8 @@ class MetadataUpdater { std::function&&)>; protected: + friend class UpdaterTest; + // Validates the relevant configuration and returns true if it's correct. // Returns a bool that represents if it's configured properly. virtual bool ValidateConfiguration() const { diff --git a/test/store_unittest.cc b/test/store_unittest.cc index 7f5cd846..c6917e2a 100644 --- a/test/store_unittest.cc +++ b/test/store_unittest.cc @@ -10,10 +10,6 @@ class MetadataStoreTest : public ::testing::Test { protected: MetadataStoreTest() : config(), store(config) {} - std::map GetMetadataMap() const { - return store.GetMetadataMap(); - } - void PurgeDeletedEntries() { store.PurgeDeletedEntries(); } @@ -82,14 +78,14 @@ TEST_F(MetadataStoreTest, ResourceToIdsAssociationCorrectlyUpdated) { } TEST_F(MetadataStoreTest, DefaultMetadataMapIsEmpty) { - const auto metadata_map = GetMetadataMap(); + const auto metadata_map = store.GetMetadataMap(); EXPECT_TRUE(metadata_map.empty()); } TEST_F(MetadataStoreTest, UpdateResourceDoesNotUpdateMetadata) { MonitoredResource resource("type", {}); store.UpdateResource({"id1"}, resource); - const auto metadata_map = GetMetadataMap(); + const auto metadata_map = store.GetMetadataMap(); EXPECT_TRUE(metadata_map.empty()); } @@ -102,7 +98,7 @@ TEST_F(MetadataStoreTest, UpdateMetadataChangesMetadataMap) { std::chrono::system_clock::now(), json::object({{"f", json::string("hello")}})); store.UpdateMetadata(resource, std::move(m)); - const auto metadata_map = GetMetadataMap(); + const auto metadata_map = store.GetMetadataMap(); EXPECT_EQ(1, metadata_map.size()); EXPECT_EQ("default-version", metadata_map.at(resource).version); } @@ -124,7 +120,7 @@ TEST_F(MetadataStoreTest, MultipleUpdateMetadataChangesMetadataMap) { json::object({{"f", json::string("hello")}})); store.UpdateMetadata(resource1, std::move(m1)); store.UpdateMetadata(resource2, std::move(m2)); - const auto metadata_map = GetMetadataMap(); + const auto metadata_map = store.GetMetadataMap(); EXPECT_EQ(2, metadata_map.size()); EXPECT_EQ("default-version1", metadata_map.at(resource1).version); EXPECT_EQ("default-version2", metadata_map.at(resource2).version); @@ -139,7 +135,7 @@ TEST_F(MetadataStoreTest, UpdateMetadataForResourceChangesMetadataEntry) { std::chrono::system_clock::now(), json::object({{"f", json::string("hello")}})); store.UpdateMetadata(resource, std::move(m1)); - const auto metadata_map_before = GetMetadataMap(); + const auto metadata_map_before = store.GetMetadataMap(); EXPECT_EQ(1, metadata_map_before.size()); EXPECT_EQ("default-version1", metadata_map_before.at(resource).version); MetadataStore::Metadata m2( @@ -149,7 +145,7 @@ TEST_F(MetadataStoreTest, UpdateMetadataForResourceChangesMetadataEntry) { std::chrono::system_clock::now(), json::object({{"f", json::string("hello")}})); store.UpdateMetadata(resource, std::move(m2)); - const auto metadata_map_after = GetMetadataMap(); + const auto metadata_map_after = store.GetMetadataMap(); EXPECT_EQ(1, metadata_map_after.size()); EXPECT_EQ("default-version2", metadata_map_after.at(resource).version); } @@ -171,12 +167,12 @@ TEST_F(MetadataStoreTest, PurgeDeletedEntriesDeletesCorrectMetadata) { json::object({{"f", json::string("hello")}})); store.UpdateMetadata(resource1, std::move(m1)); store.UpdateMetadata(resource2, std::move(m2)); - const auto metadata_map_before = GetMetadataMap(); + const auto metadata_map_before = store.GetMetadataMap(); EXPECT_EQ(2, metadata_map_before.size()); EXPECT_EQ("default-version1", metadata_map_before.at(resource1).version); EXPECT_EQ("default-version2", metadata_map_before.at(resource2).version); PurgeDeletedEntries(); - const auto metadata_map_after = GetMetadataMap(); + const auto metadata_map_after = store.GetMetadataMap(); EXPECT_EQ(1, metadata_map_after.size()); EXPECT_EQ("default-version1", metadata_map_after.at(resource1).version); EXPECT_THROW(metadata_map_after.at(resource2), std::out_of_range); diff --git a/test/updater_unittest.cc b/test/updater_unittest.cc index 85d88867..b5c1205f 100644 --- a/test/updater_unittest.cc +++ b/test/updater_unittest.cc @@ -12,34 +12,38 @@ namespace google { class UpdaterTest : public ::testing::Test { protected: // query_metadata function not needed to test callbacks. - UpdaterTest() : config(), store(config), updater( - config, &store, "Test", 60.0, nullptr) {} + UpdaterTest() : config(), store(config) {} - std::map GetMetadataMap() const { - return store.GetMetadataMap(); + static bool ValidateConfiguration(MetadataUpdater* updater) { + return updater->ValidateConfiguration(); } - bool ValidateConfiguration() const { - return updater.ValidateConfiguration(); + static void UpdateMetadataCallback( + MetadataUpdater* updater, + MetadataUpdater::ResourceMetadata&& result) { + updater->UpdateMetadataCallback(std::move(result)); } - void UpdateMetadataCallback(MetadataUpdater::ResourceMetadata&& result) { - updater.UpdateMetadataCallback(std::move(result)); - } - - void UpdateResourceCallback(const MetadataUpdater::ResourceMetadata& result) { - updater.UpdateResourceCallback(result); + static void UpdateResourceCallback( + MetadataUpdater* updater, + const MetadataUpdater::ResourceMetadata& result) { + updater->UpdateResourceCallback(result); } Configuration config; MetadataStore store; - PollingMetadataUpdater updater; }; namespace { TEST_F(UpdaterTest, ValidateConfiguration) { - EXPECT_TRUE(ValidateConfiguration()); + PollingMetadataUpdater updater(config, &store, "Test", 60, nullptr); + EXPECT_TRUE(ValidateConfiguration(&updater)); +} + +TEST_F(UpdaterTest, UpdaterWithInvalidPollingIntervalTest) { + PollingMetadataUpdater updater(config, &store, "BadUpdater", -1, nullptr); + EXPECT_FALSE(ValidateConfiguration(&updater)); } TEST_F(UpdaterTest, UpdateMetadataCallbackTest) { @@ -53,8 +57,9 @@ TEST_F(UpdaterTest, UpdateMetadataCallbackTest) { MetadataUpdater::ResourceMetadata metadata( std::vector({"", "test-prefix"}), resource, std::move(m)); - UpdateMetadataCallback(std::move(metadata)); - const auto metadata_map = GetMetadataMap(); + PollingMetadataUpdater updater(config, &store, "Test", 60, nullptr); + UpdateMetadataCallback(&updater, std::move(metadata)); + const auto metadata_map = store.GetMetadataMap(); EXPECT_EQ(1, metadata_map.size()); EXPECT_EQ("test-version", metadata_map.at(resource).version); EXPECT_EQ("{\"f\":\"test\"}", metadata_map.at(resource).metadata->ToString()); @@ -66,7 +71,8 @@ TEST_F(UpdaterTest, UpdateResourceCallbackTest) { MonitoredResource("test_resource", {}), MetadataStore::Metadata::IGNORED() ); - UpdateResourceCallback(metadata); + PollingMetadataUpdater updater(config, &store, "Test", 60, nullptr); + UpdateResourceCallback(&updater, metadata); EXPECT_EQ(MonitoredResource("test_resource", {}), store.LookupResource("")); EXPECT_EQ(MonitoredResource("test_resource", {}), From 4842e7fc85d9e402c98b878faf2eb8f210f8216e Mon Sep 17 00:00:00 2001 From: Dhrupad Bhardwaj Date: Sat, 31 Mar 2018 14:20:20 -0400 Subject: [PATCH 4/5] Address feedback --- src/store.h | 4 ++-- test/updater_unittest.cc | 13 +++++++++---- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/src/store.h b/src/store.h index 0fad6c15..a5e0650e 100644 --- a/src/store.h +++ b/src/store.h @@ -74,8 +74,8 @@ class MetadataStore { MetadataStore(const Configuration& config); - // Returns a copy of the map containing the mapping from monitored - // resources to metadata associated with that resource. + // Returns a copy of the mapping from monitored resources to metadata + // associated with that resource. std::map GetMetadataMap() const; // Looks up the local resource map entry for a given resource id. diff --git a/test/updater_unittest.cc b/test/updater_unittest.cc index b5c1205f..6dd53404 100644 --- a/test/updater_unittest.cc +++ b/test/updater_unittest.cc @@ -36,17 +36,22 @@ class UpdaterTest : public ::testing::Test { namespace { -TEST_F(UpdaterTest, ValidateConfiguration) { +TEST_F(UpdaterTest, OneMinutePollingIntervalIsValid) { PollingMetadataUpdater updater(config, &store, "Test", 60, nullptr); EXPECT_TRUE(ValidateConfiguration(&updater)); } -TEST_F(UpdaterTest, UpdaterWithInvalidPollingIntervalTest) { +TEST_F(UpdaterTest, ZeroSecondPollingIntervalIsValid) { + PollingMetadataUpdater updater(config, &store, "Test", 0, nullptr); + EXPECT_TRUE(ValidateConfiguration(&updater)); +} + +TEST_F(UpdaterTest, NegativePollingIntervalIsInvalid) { PollingMetadataUpdater updater(config, &store, "BadUpdater", -1, nullptr); EXPECT_FALSE(ValidateConfiguration(&updater)); } -TEST_F(UpdaterTest, UpdateMetadataCallbackTest) { +TEST_F(UpdaterTest, UpdateMetadataCallback) { MetadataStore::Metadata m( "test-version", false, @@ -65,7 +70,7 @@ TEST_F(UpdaterTest, UpdateMetadataCallbackTest) { EXPECT_EQ("{\"f\":\"test\"}", metadata_map.at(resource).metadata->ToString()); } -TEST_F(UpdaterTest, UpdateResourceCallbackTest) { +TEST_F(UpdaterTest, UpdateResourceCallback) { MetadataUpdater::ResourceMetadata metadata( std::vector({"", "test-prefix"}), MonitoredResource("test_resource", {}), From 53d36c57d8ba515e74b11084f0092240c5dd0c9a Mon Sep 17 00:00:00 2001 From: Dhrupad Bhardwaj Date: Sat, 31 Mar 2018 16:04:48 -0400 Subject: [PATCH 5/5] Address nit --- src/store.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/store.h b/src/store.h index a5e0650e..5b1c25ce 100644 --- a/src/store.h +++ b/src/store.h @@ -74,7 +74,7 @@ class MetadataStore { MetadataStore(const Configuration& config); - // Returns a copy of the mapping from monitored resources to metadata + // Returns a copy of the mapping from a monitored resource to the metadata // associated with that resource. std::map GetMetadataMap() const;