Skip to content
This repository was archived by the owner on Aug 19, 2019. It is now read-only.

Unit tests for MetadataStore #101

Merged
igorpeshansky merged 9 commits into from
Mar 30, 2018
Merged

Unit tests for MetadataStore #101

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
24d8201
Unittests for MetadataStore and Metadata struct
dhrupadb Mar 28, 2018
b7dd868
Unit test for PurgeDeletedEntries
dhrupadb Mar 28, 2018
0f276c6
Address feedback
dhrupadb Mar 28, 2018
a68a995
Address more feedback
dhrupadb Mar 28, 2018
c02df84
Address feedback
dhrupadb Mar 29, 2018
fba23d2
Address feedback
dhrupadb Mar 29, 2018
6797d98
Address feedback
dhrupadb Mar 29, 2018
1d173e5
Address feedback, revert bad changes
dhrupadb Mar 29, 2018
87eceaa
Make the clone of ignored metadata explicit; remove redundant check.
igorpeshansky Mar 30, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/store.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ class MetadataStore {

Metadata Clone() const {
if (ignore) {
return {};
return IGNORED();
}
return {version, is_deleted, created_at, collected_at, metadata->Clone()};
}
Expand Down Expand Up @@ -92,6 +92,7 @@ class MetadataStore {

private:
friend class MetadataReporter;
friend class MetadataStoreTest;

std::map<MonitoredResource, Metadata> GetMetadataMap() const;
void PurgeDeletedEntries();
Expand Down
3 changes: 3 additions & 0 deletions test/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ TESTS=\
health_checker_unittest \
json_unittest \
resource_unittest \
store_unittest \
time_unittest

GTEST_LIB=gtest_lib.a
Expand Down Expand Up @@ -85,6 +86,8 @@ configuration_unittest: $(GTEST_LIB) $(YAML_CPP_LIBS) configuration_unittest.o $
$(CXX) $(LDFLAGS) $^ $(LDLIBS) -o $@
resource_unittest: $(GTEST_LIB) resource_unittest.o $(SRC_DIR)/resource.o $(SRC_DIR)/json.o
$(CXX) $(LDFLAGS) $^ $(LDLIBS) -o $@
store_unittest: $(GTEST_LIB) store_unittest.o $(SRC_DIR)/store.o $(SRC_DIR)/resource.o $(SRC_DIR)/json.o $(SRC_DIR)/logging.o $(SRC_DIR)/time.o $(SRC_DIR)/configuration.o
$(CXX) $(LDFLAGS) $^ $(LDLIBS) -o $@
time_unittest: $(GTEST_LIB) time_unittest.o $(SRC_DIR)/time.o
$(CXX) $(LDFLAGS) $^ $(LDLIBS) -o $@
health_checker_unittest: $(GTEST_LIB) health_checker_unittest.o $(SRC_DIR)/health_checker.o $(SRC_DIR)/configuration.o
Expand Down
229 changes: 229 additions & 0 deletions test/store_unittest.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,229 @@
#include "../src/configuration.h"
#include "../src/resource.h"
#include "../src/store.h"
#include "../src/time.h"
#include "gtest/gtest.h"

namespace google {

class MetadataStoreTest : public ::testing::Test {
protected:
MetadataStoreTest() : config(), store(config) {}

std::map<MonitoredResource, MetadataStore::Metadata> GetMetadataMap() const {
return store.GetMetadataMap();
}

void PurgeDeletedEntries() {
store.PurgeDeletedEntries();
}

Copy link
Contributor

@igorpeshansky igorpeshansky Mar 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for a blank line here.

Copy link
Contributor Author

@dhrupadb dhrupadb Mar 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Configuration config;
Copy link
Contributor

@bmoyles0117 bmoyles0117 Mar 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we actually need a local copy of the config? I'm only seeing it being used in the constructor.

Copy link
Contributor Author

@dhrupadb dhrupadb Mar 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Good catch.

Copy link
Contributor

@igorpeshansky igorpeshansky Mar 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, we do need a local copy of the config, to make sure it gets properly cleaned up. The store does not own the config reference, so the new code leaks. In the previous version, the MetadataStoreTest object owned the Configuration object, and cleanup happened automatically. Can we please revert this change?

Copy link
Contributor Author

@dhrupadb dhrupadb Mar 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

MetadataStore store;
};

TEST_F(MetadataStoreTest, ResourceWithOneIdCorrectlyStored) {
MonitoredResource resource("type", {});
store.UpdateResource({"id"}, resource);
EXPECT_EQ(resource, store.LookupResource("id"));
}

TEST_F(MetadataStoreTest, EmptyStoreLookupThrowsError) {
EXPECT_THROW(store.LookupResource("missing_id"), std::out_of_range);
}

TEST_F(MetadataStoreTest, ResourceLookupFailuresAreIndependent) {
MonitoredResource resource("type", {});
store.UpdateResource({"id"}, resource);
EXPECT_THROW(store.LookupResource("missing_id"), std::out_of_range);
EXPECT_EQ(resource, store.LookupResource("id"));
}

TEST_F(MetadataStoreTest, MultipleResourcesWithSingleIdsCorrectlyStored) {
MonitoredResource resource1("type1", {});
MonitoredResource resource2("type2", {});
store.UpdateResource({"id1"}, resource1);
store.UpdateResource({"id2"}, resource2);
EXPECT_EQ(resource2, store.LookupResource("id2"));
EXPECT_EQ(resource1, store.LookupResource("id1"));
}

TEST_F(MetadataStoreTest, SingleResourceWithMultipleIdsCorrectlyStored) {
MonitoredResource resource("type", {});
store.UpdateResource({"id1", "id2", "id3"}, resource);
EXPECT_EQ(resource, store.LookupResource("id3"));
EXPECT_EQ(resource, store.LookupResource("id1"));
EXPECT_EQ(resource, store.LookupResource("id2"));
}

TEST_F(MetadataStoreTest, MultipleResourcesAndIdsCorrectlyStored) {
MonitoredResource resource1("type1", {});
MonitoredResource resource2("type2", {});
store.UpdateResource({"id1", "id2", "id3"}, resource1);
store.UpdateResource({"id-a", "id-b", "id-c"}, resource2);
EXPECT_EQ(resource1, store.LookupResource("id1"));
EXPECT_EQ(resource1, store.LookupResource("id2"));
EXPECT_EQ(resource1, store.LookupResource("id3"));
EXPECT_EQ(resource2, store.LookupResource("id-a"));
EXPECT_EQ(resource2, store.LookupResource("id-b"));
EXPECT_EQ(resource2, store.LookupResource("id-c"));
}

TEST_F(MetadataStoreTest, ResourceToIdsAssociationCorrectlyUpdated) {
MonitoredResource resource("type", {});
store.UpdateResource({"id1", "id2"}, resource);
EXPECT_EQ(resource, store.LookupResource("id1"));
EXPECT_EQ(resource, store.LookupResource("id2"));
store.UpdateResource({"id-a", "id-b"}, resource);
EXPECT_EQ(resource, store.LookupResource("id1"));
EXPECT_EQ(resource, store.LookupResource("id2"));
EXPECT_EQ(resource, store.LookupResource("id-a"));
EXPECT_EQ(resource, store.LookupResource("id-b"));
}

TEST_F(MetadataStoreTest, DefaultMetadataMapIsEmpty) {
const auto metadata_map = GetMetadataMap();
EXPECT_TRUE(metadata_map.empty());
}

TEST_F(MetadataStoreTest, UpdateResourceDoesNotUpdateMetadata) {
MonitoredResource resource("type", {});
store.UpdateResource({"id1"}, resource);
const auto metadata_map = GetMetadataMap();
EXPECT_TRUE(metadata_map.empty());
}

TEST_F(MetadataStoreTest, UpdateMetadataChangesMetadataMap) {
MonitoredResource resource("type", {});
MetadataStore::Metadata m(
"default-version",
false,
std::chrono::system_clock::now(),
std::chrono::system_clock::now(),
json::object({{"f", json::string("hello")}}));
store.UpdateMetadata(resource, std::move(m));
const auto metadata_map = GetMetadataMap();
EXPECT_EQ(1, metadata_map.size());
EXPECT_EQ("default-version", metadata_map.at(resource).version);
}

TEST_F(MetadataStoreTest, MultipleUpdateMetadataChangesMetadataMap) {
MonitoredResource resource1("type1", {});
MonitoredResource resource2("type2", {});
MetadataStore::Metadata m1(
"default-version1",
false,
std::chrono::system_clock::now(),
std::chrono::system_clock::now(),
json::object({{"f", json::string("hello")}}));
MetadataStore::Metadata m2(
"default-version2",
false,
std::chrono::system_clock::now(),
std::chrono::system_clock::now(),
json::object({{"f", json::string("hello")}}));
store.UpdateMetadata(resource1, std::move(m1));
store.UpdateMetadata(resource2, std::move(m2));
const auto metadata_map = 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);
}

TEST_F(MetadataStoreTest, UpdateMetadataForResourceChangesMetadataEntry) {
MonitoredResource resource("type", {});
MetadataStore::Metadata m1(
"default-version1",
false,
std::chrono::system_clock::now(),
std::chrono::system_clock::now(),
json::object({{"f", json::string("hello")}}));
store.UpdateMetadata(resource, std::move(m1));
const auto metadata_map_before = GetMetadataMap();
EXPECT_EQ(1, metadata_map_before.size());
EXPECT_EQ("default-version1", metadata_map_before.at(resource).version);
MetadataStore::Metadata m2(
"default-version2",
false,
std::chrono::system_clock::now(),
std::chrono::system_clock::now(),
json::object({{"f", json::string("hello")}}));
store.UpdateMetadata(resource, std::move(m2));
const auto metadata_map_after = GetMetadataMap();
EXPECT_EQ(1, metadata_map_after.size());
EXPECT_EQ("default-version2", metadata_map_after.at(resource).version);
}

TEST_F(MetadataStoreTest, PurgeDeletedEntriesDeletesCorrectMetadata) {
MonitoredResource resource1("type1", {});
MonitoredResource resource2("type2", {});
MetadataStore::Metadata m1(
"default-version1",
false,
std::chrono::system_clock::now(),
std::chrono::system_clock::now(),
json::object({{"f", json::string("hello")}}));
MetadataStore::Metadata m2(
"default-version2",
true,
std::chrono::system_clock::now(),
std::chrono::system_clock::now(),
json::object({{"f", json::string("hello")}}));
store.UpdateMetadata(resource1, std::move(m1));
store.UpdateMetadata(resource2, std::move(m2));
const auto metadata_map_before = 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();
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);
}

TEST(MetadataTest, MetadataCorrectlyConstructed) {
MetadataStore::Metadata m(
"default-version",
false,
time::rfc3339::FromString("2018-03-03T01:23:45.678901234Z"),
time::rfc3339::FromString("2018-03-03T01:32:45.678901234Z"),
json::object({{"f", json::string("hello")}}));
EXPECT_FALSE(m.ignore);
EXPECT_EQ("default-version", m.version);
EXPECT_FALSE(m.is_deleted);
EXPECT_EQ(time::rfc3339::FromString("2018-03-03T01:23:45.678901234Z"),
m.created_at);
EXPECT_EQ(time::rfc3339::FromString("2018-03-03T01:32:45.678901234Z"),
m.collected_at);
EXPECT_EQ("{\"f\":\"hello\"}", m.metadata->ToString());
}

TEST(MetadataTest, MetadataCorrectlyCloned) {
MetadataStore::Metadata m(
"default-version",
false,
time::rfc3339::FromString("2018-03-03T01:23:45.678901234Z"),
time::rfc3339::FromString("2018-03-03T01:32:45.678901234Z"),
json::object({{"f", json::string("hello")}}));
MetadataStore::Metadata m_clone = m.Clone();
EXPECT_FALSE(m_clone.ignore);
EXPECT_EQ(m.version, m_clone.version);
EXPECT_FALSE(m_clone.is_deleted);
EXPECT_EQ(m.created_at, m_clone.created_at);
EXPECT_EQ(m.collected_at, m_clone.collected_at);
EXPECT_EQ(m.metadata->ToString(), m_clone.metadata->ToString());
}

TEST(MetadataTest, IgnoredMetadataCorrectlyCreated) {
MetadataStore::Metadata m = MetadataStore::Metadata::IGNORED();
EXPECT_TRUE(m.ignore);
}

TEST(MetadataTest, IgnoredMetadataCorrectlyCloned) {
MetadataStore::Metadata m = MetadataStore::Metadata::IGNORED();
MetadataStore::Metadata m_clone = m.Clone();
Copy link
Contributor

@bmoyles0117 bmoyles0117 Mar 29, 2018 •
edited by igorpeshansky
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might seem like overkill, but it would be nice to add EXPECT_TRUE(m.ignore);, as I believe m.Clone() could potentially mutate the object, and thus copy from there and have both be false. If it's not possible that the value could be mutated, ignore :)

Copy link
Contributor Author

@dhrupadb dhrupadb Mar 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I was on the same train but @igorpeshansky 's point is that we should test that "cloning" an IGNORED metadata type works. As a result if .ignore before and after is the same the test worked, independent of the actual value. As is it is impossible to mutate the type of the ignore field because it can only be set via the default constructor.

Copy link
Contributor

@igorpeshansky igorpeshansky Mar 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dhrupadb, weren't you testing m_clone.ignore? Bryan suggests testing m.ignore. However, Clone() is declared const, so it shouldn't mutate any publicly visible state.

Copy link
Contributor Author

@dhrupadb dhrupadb Mar 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Added a check for m.

Copy link
Contributor

@igorpeshansky igorpeshansky Mar 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No point in checking m, really, unless you don't trust const.
One thing that would help is changing the implementation of Clone() to use IGNORED() directly rather than the empty constructor/initializer list (effectively inlining the IGNORED() implementation). I've taken the liberty of pushing that change into your branch, hope that's ok.

Copy link
Contributor Author

@dhrupadb dhrupadb Mar 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep LGTM. Thanks!

EXPECT_EQ(m.ignore, m_clone.ignore);
}

} // namespace