Unit tests for MetadataStore #101
Conversation
igorpeshansky
changed the title
src/store.h
Outdated
|
|
||
| private: | ||
| friend class MetadataReporter; | ||
| friend class MetadataAgentStoreTest; |
There was a problem hiding this comment.
Why MetadataAgentStoreTest and not MetadataStoreTest?
test/store_unittest.cc
Outdated
|
|
||
| class MetadataAgentStoreTest : public ::testing::Test { | ||
| protected: | ||
| Configuration *config; |
There was a problem hiding this comment.
Why don't we make this a unique_ptr? Then you don't have to worry about cleaning it up. Ditto with the store pointer.
test/store_unittest.cc
Outdated
| Configuration *config; | ||
| MetadataStore* store; | ||
|
|
||
| void SetupStore() { |
There was a problem hiding this comment.
This can be done in the constructor, can't it? Then they won't even need to be pointers -- just inline objects.
test/store_unittest.cc
Outdated
| MetadataStore* store; | ||
|
|
||
| void SetupStore() { | ||
| config = new Configuration(std::istringstream("")); |
There was a problem hiding this comment.
This is equivalent to new Configuration().
test/store_unittest.cc
Outdated
| time::rfc3339::FromString("2018-03-03T01:23:45.678901234Z"), | ||
| time::rfc3339::FromString("2018-03-03T01:32:45.678901234Z"), | ||
| json::Parser::FromString("{\"f\":\"hello\"}") | ||
| ); |
There was a problem hiding this comment.
This isn't a container -- just close the parenthesis on the above line. Same everywhere below.
test/store_unittest.cc
Outdated
| false, | ||
| time::rfc3339::FromString("2018-03-03T01:23:45.678901234Z"), | ||
| time::rfc3339::FromString("2018-03-03T01:32:45.678901234Z"), | ||
| json::Parser::FromString("{\"f\":\"hello\"}") |
There was a problem hiding this comment.
json::object({{"f": json::string("hello")}})
test/store_unittest.cc
Outdated
| MetadataStore::Metadata m( | ||
| "default-version", | ||
| false, | ||
| time::rfc3339::FromString("2018-03-03T01:23:45.678901234Z"), |
There was a problem hiding this comment.
Why not use std::chrono::system_clock::now()?
test/store_unittest.cc
Outdated
| m.created_at); | ||
| EXPECT_EQ(time::rfc3339::FromString("2018-03-03T01:32:45.678901234Z"), | ||
| m.collected_at); | ||
| const auto& obj = m.metadata->As<json::Object>(); |
There was a problem hiding this comment.
Careful -- the return value isn't a reference. But here auto doesn't buy you anything anyway -- just use const json::Object* obj.
| m.collected_at); | ||
| EXPECT_EQ("{}", m.metadata->ToString()); | ||
| EXPECT_FALSE(m.is_deleted); | ||
| EXPECT_TRUE(m.ignore); |
There was a problem hiding this comment.
This is the only field worth checking. The rest are implementation details.
| ); | ||
| MetadataStore::Metadata m_clone = m.Clone(); | ||
| EXPECT_EQ("default-version", m_clone.version); | ||
| EXPECT_FALSE(m_clone.is_deleted); |
There was a problem hiding this comment.
You'll want to check that the clone isn't ignored (EXPECT_FALSE(m_clone.ignore)).
There was a problem hiding this comment.
Oops, looks like you already had it and I missed it.
src/store.h
Outdated
|
|
||
| private: | ||
| friend class MetadataReporter; | ||
| friend class MetadataAgentStoreTest; |
test/store_unittest.cc
Outdated
| Configuration *config; | ||
| MetadataStore* store; | ||
|
|
||
| void SetupStore() { |
test/store_unittest.cc
Outdated
| MetadataStore* store; | ||
|
|
||
| void SetupStore() { | ||
| config = new Configuration(std::istringstream("")); |
test/store_unittest.cc
Outdated
|
|
||
| }; | ||
|
|
||
| TEST_F(MetadataAgentStoreTest, ResourceIDStore) { |
test/store_unittest.cc
Outdated
|
|
||
| TEST_F(MetadataAgentStoreTest, ResourceNotFoundStore) { | ||
| SetupStore(); | ||
| EXPECT_THROW(store->LookupResource("some_resource_id").type(), |
test/store_unittest.cc
Outdated
| false, | ||
| time::rfc3339::FromString("2018-03-03T01:23:45.678901234Z"), | ||
| time::rfc3339::FromString("2018-03-03T01:32:45.678901234Z"), | ||
| json::Parser::FromString("{\"f\":\"hello\"}") |
test/store_unittest.cc
Outdated
| m.created_at); | ||
| EXPECT_EQ(time::rfc3339::FromString("2018-03-03T01:32:45.678901234Z"), | ||
| m.collected_at); | ||
| const auto& obj = m.metadata->As<json::Object>(); |
| ); | ||
| MetadataStore::Metadata m_clone = m.Clone(); | ||
| EXPECT_EQ("default-version", m_clone.version); | ||
| EXPECT_FALSE(m_clone.is_deleted); |
| m.collected_at); | ||
| EXPECT_EQ("{}", m.metadata->ToString()); | ||
| EXPECT_FALSE(m.is_deleted); | ||
| EXPECT_TRUE(m.ignore); |
test/store_unittest.cc
Outdated
| SetupStore(); | ||
| MonitoredResource mr("some_resource", {}); | ||
| std::vector<std::string> resourceId(1, "id"); | ||
| store->UpdateResource(resourceId, mr); |
test/store_unittest.cc
Outdated
|
|
||
| MetadataStoreTest() : config(), store(config) {} | ||
|
|
||
| std::map<MonitoredResource, MetadataStore::Metadata> GetMetadataMap() { |
There was a problem hiding this comment.
[Optional] Mark this function as const.
test/store_unittest.cc
Outdated
| EXPECT_THROW(store.LookupResource("some_resource_id"), std::out_of_range); | ||
| } | ||
|
|
||
| TEST_F(MetadataStoreTest, ManyResources) { |
There was a problem hiding this comment.
I wonder if we could come up with some consistent naming scheme, e.g., OneResource above, MultipleResources here, OneResourceMultipleIds below, then MultipleResourcesMultipleIds, etc...
test/store_unittest.cc
Outdated
| TEST_F(MetadataStoreTest, ResourceID) { | ||
| MonitoredResource mr("some_resource", {}); | ||
| store.UpdateResource({"id"}, mr); | ||
| EXPECT_EQ("some_resource", store.LookupResource("id").type()); |
There was a problem hiding this comment.
[Optional] MonitoredResource has an equality operator defined. So you can just compare monitored resources for equality, e.g., EXPECT_EQ(mr, store.LookupResource("id"));.
test/store_unittest.cc
Outdated
| EXPECT_EQ("some_resource", store.LookupResource("id2").type()); | ||
| store.UpdateResource({"id-a", "id-b"}, mr); | ||
| EXPECT_EQ("some_resource", store.LookupResource("id-a").type()); | ||
| EXPECT_EQ("some_resource", store.LookupResource("id-b").type()); |
There was a problem hiding this comment.
We should check whether "id1" and "id2" still map to the resource. This will document the current behavior, and allow a discussion of whether that's the behavior we want.
test/store_unittest.cc
Outdated
| EXPECT_TRUE(metadata_map.empty()); | ||
| } | ||
|
|
||
| TEST_F(MetadataStoreTest, EmptyMetadataNonemptyResource) { |
There was a problem hiding this comment.
It's nice to name tests according to what they test and the expected outcome. How about UpdateResourceDoesNotUpdateMetadata, UpdateMetadataChangesMetadataMap, etc?
test/store_unittest.cc
Outdated
| store.UpdateMetadata(mr1, std::move(m1)); | ||
| store.UpdateMetadata(mr2, std::move(m2)); | ||
| const auto metadata_map_before = GetMetadataMap(); | ||
| EXPECT_FALSE(metadata_map_before.empty()); |
There was a problem hiding this comment.
You can check the exact size instead, here and below.
test/store_unittest.cc
Outdated
| EXPECT_THROW(metadata_map_after.at(mr2), std::out_of_range); | ||
| } | ||
|
|
||
| TEST(MetadataTest, ParseMetadata) { |
There was a problem hiding this comment.
Umm, what exactly is this testing? The fact that you can retrieve the fields?
The only thing that's worth testing here, IMO, is that a metadata that wasn't constructed via IGNORED() is not ignored (i.e., EXPECT_FALSE(m.ignore);). I'd even call this test ConstructedMetadataNotIgnored or something.
There was a problem hiding this comment.
Just checking that the default constructor works properly. Updated.
test/store_unittest.cc
Outdated
| time::rfc3339::FromString("2018-03-03T01:32:45.678901234Z"), | ||
| json::object({{"f", json::string("hello")}})); | ||
| MetadataStore::Metadata m_clone = m.Clone(); | ||
| EXPECT_EQ("default-version", m_clone.version); |
There was a problem hiding this comment.
How about EXPECT_EQ(m.version, m_clone.version);, etc?
There was a problem hiding this comment.
Done. for metadata it needs a deep comparison so left it as is.
There was a problem hiding this comment.
You can just compare the ToString values, i.e., EXPECT_EQ(m.metadata->ToString(), m_clone.metadata->ToString())...
test/store_unittest.cc
Outdated
|
|
||
| TEST(MetadataTest, TestIgnoredMetadata) { | ||
| MetadataStore::Metadata m = MetadataStore::Metadata::IGNORED(); | ||
| EXPECT_FALSE(m.is_deleted); |
There was a problem hiding this comment.
How is this relevant?
There was a problem hiding this comment.
Forgot to delete that. PTAL.
test/store_unittest.cc
Outdated
| json::object({{"f", json::string("hello")}})); | ||
| EXPECT_EQ("default-version", m.version); | ||
| EXPECT_FALSE(m.is_deleted); | ||
| EXPECT_FALSE(m.ignore); |
There was a problem hiding this comment.
Let's make sure to test this first, and then test the contents.
test/store_unittest.cc
Outdated
|
|
||
| MetadataStoreTest() : config(), store(config) {} | ||
|
|
||
| std::map<MonitoredResource, MetadataStore::Metadata> GetMetadataMap() { |
test/store_unittest.cc
Outdated
| TEST_F(MetadataStoreTest, ResourceID) { | ||
| MonitoredResource mr("some_resource", {}); | ||
| store.UpdateResource({"id"}, mr); | ||
| EXPECT_EQ("some_resource", store.LookupResource("id").type()); |
test/store_unittest.cc
Outdated
| EXPECT_EQ("some_resource", store.LookupResource("id2").type()); | ||
| store.UpdateResource({"id-a", "id-b"}, mr); | ||
| EXPECT_EQ("some_resource", store.LookupResource("id-a").type()); | ||
| EXPECT_EQ("some_resource", store.LookupResource("id-b").type()); |
test/store_unittest.cc
Outdated
| EXPECT_TRUE(metadata_map.empty()); | ||
| } | ||
|
|
||
| TEST_F(MetadataStoreTest, EmptyMetadataNonemptyResource) { |
test/store_unittest.cc
Outdated
| store.UpdateMetadata(mr1, std::move(m1)); | ||
| store.UpdateMetadata(mr2, std::move(m2)); | ||
| const auto metadata_map_before = GetMetadataMap(); | ||
| EXPECT_FALSE(metadata_map_before.empty()); |
test/store_unittest.cc
Outdated
| EXPECT_THROW(metadata_map_after.at(mr2), std::out_of_range); | ||
| } | ||
|
|
||
| TEST(MetadataTest, ParseMetadata) { |
There was a problem hiding this comment.
Just checking that the default constructor works properly. Updated.
test/store_unittest.cc
Outdated
| json::object({{"f", json::string("hello")}})); | ||
| EXPECT_EQ("default-version", m.version); | ||
| EXPECT_FALSE(m.is_deleted); | ||
| EXPECT_FALSE(m.ignore); |
test/store_unittest.cc
Outdated
| time::rfc3339::FromString("2018-03-03T01:32:45.678901234Z"), | ||
| json::object({{"f", json::string("hello")}})); | ||
| MetadataStore::Metadata m_clone = m.Clone(); | ||
| EXPECT_EQ("default-version", m_clone.version); |
There was a problem hiding this comment.
Done. for metadata it needs a deep comparison so left it as is.
test/store_unittest.cc
Outdated
|
|
||
| TEST(MetadataTest, TestIgnoredMetadata) { | ||
| MetadataStore::Metadata m = MetadataStore::Metadata::IGNORED(); | ||
| EXPECT_FALSE(m.is_deleted); |
There was a problem hiding this comment.
Forgot to delete that. PTAL.
test/store_unittest.cc
Outdated
| EXPECT_THROW(store.LookupResource("some_resource_id"), std::out_of_range); | ||
| } | ||
|
|
||
| TEST_F(MetadataStoreTest, ManyResources) { |
test/store_unittest.cc
Outdated
| EXPECT_EQ(mr, store.LookupResource("id")); | ||
| } | ||
|
|
||
| TEST_F(MetadataStoreTest, EmptyStoreThrowsError) { |
There was a problem hiding this comment.
Maybe UnknownResourceIdThrowsError?
There was a problem hiding this comment.
That's the next test. This was just making sure that an empty store threw an error.
There was a problem hiding this comment.
The next test actually checks something else. It checks that known keys return data, while unknown keys punt. As opposed to this one, where every key is effectively unknown.
The way it's named now makes it seem like an empty store should always throw an error, where what it's testing is that every lookup will throw an error if the store is empty.
You could call this EmptyStoreLookupThrowsError if you want.
test/store_unittest.cc
Outdated
| MonitoredResource mr("some_resource", {}); | ||
| store.UpdateResource({"id"}, mr); | ||
| EXPECT_EQ(mr, store.LookupResource("id")); | ||
| EXPECT_THROW(store.LookupResource("other_resource"), std::out_of_range); |
There was a problem hiding this comment.
How about switching them around, to ensure that it doesn't keep throwing?
There was a problem hiding this comment.
Done. You're saying that this guards against multiple throws?
There was a problem hiding this comment.
It guards against the possibility that once a lookup fails, any subsequent lookup will also throw. Not that this is likely to happen, but unit tests are supposed to be paranoid and guard against corner cases.
There was a problem hiding this comment.
Ah gotcha. Makes sense.
test/store_unittest.cc
Outdated
| }; | ||
|
|
||
| TEST_F(MetadataStoreTest, ResourceWithOneIdStoredCorrectly) { | ||
| MonitoredResource mr("some_resource", {}); |
There was a problem hiding this comment.
The first value is type, so how about using "type" instead of "some_resource"? Also below.
test/store_unittest.cc
Outdated
| } | ||
|
|
||
| TEST_F(MetadataStoreTest, EmptyStoreThrowsError) { | ||
| EXPECT_THROW(store.LookupResource("some_resource_id"), std::out_of_range); |
There was a problem hiding this comment.
s/"some_resource_id"/"unknown_id"/
There was a problem hiding this comment.
Let's use either missing_id or unknown_id consistently throughout.
test/store_unittest.cc
Outdated
| MonitoredResource mr("some_resource", {}); | ||
| store.UpdateResource({"id"}, mr); | ||
| EXPECT_EQ(mr, store.LookupResource("id")); | ||
| EXPECT_THROW(store.LookupResource("other_resource"), std::out_of_range); |
There was a problem hiding this comment.
s/"other_resource"/"unknown_id"/
test/store_unittest.cc
Outdated
| MetadataStore::Metadata m_clone = m.Clone(); | ||
| EXPECT_EQ(m.version, m_clone.version); | ||
| EXPECT_FALSE(m_clone.is_deleted); | ||
| EXPECT_FALSE(m_clone.ignore); |
There was a problem hiding this comment.
Let's check this first.
| json::object({{"f", json::string("hello")}})); | ||
| MetadataStore::Metadata m_clone = m.Clone(); | ||
| EXPECT_EQ(m.version, m_clone.version); | ||
| EXPECT_FALSE(m_clone.is_deleted); |
There was a problem hiding this comment.
Why not EXPECT_EQ(m.is_deleted, m_clone.is_deleted)?
There was a problem hiding this comment.
This means the type could change though. Are we ok with that?
There was a problem hiding this comment.
Huh? Doesn't the construction test check that?
There was a problem hiding this comment.
True. But in case the implementation changes we want to explicitly check the field.
test/store_unittest.cc
Outdated
| TEST(MetadataTest, IgnoredMetadataCorrectlyCloned) { | ||
| MetadataStore::Metadata m = MetadataStore::Metadata::IGNORED(); | ||
| MetadataStore::Metadata m_clone = m.Clone(); | ||
| EXPECT_TRUE(m_clone.ignore); |
There was a problem hiding this comment.
[Optional] EXPECT_EQ(m.ignore, m_clone.ignore). But may not be worth it with just one value.
There was a problem hiding this comment.
Same comment as above.
There was a problem hiding this comment.
Doesn't the construction test already check that?
There was a problem hiding this comment.
True. But in case the implementation changes we want to explicitly check the field.
| void PurgeDeletedEntries() { | ||
| store.PurgeDeletedEntries(); | ||
| } | ||
|
|
There was a problem hiding this comment.
No need for a blank line here.
test/store_unittest.cc
Outdated
|
|
||
| class MetadataStoreTest : public ::testing::Test { | ||
| protected: | ||
| Configuration config; |
There was a problem hiding this comment.
Data members follow function members (style guide).
test/store_unittest.cc
Outdated
|
|
||
| class MetadataStoreTest : public ::testing::Test { | ||
| protected: | ||
| Configuration config; |
| void PurgeDeletedEntries() { | ||
| store.PurgeDeletedEntries(); | ||
| } | ||
|
|
test/store_unittest.cc
Outdated
|
|
||
| }; | ||
|
|
||
| TEST_F(MetadataStoreTest, ResourceWithOneIdStoredCorrectly) { |
test/store_unittest.cc
Outdated
| }; | ||
|
|
||
| TEST_F(MetadataStoreTest, ResourceWithOneIdStoredCorrectly) { | ||
| MonitoredResource mr("some_resource", {}); |
test/store_unittest.cc
Outdated
| EXPECT_EQ(mr, store.LookupResource("id")); | ||
| } | ||
|
|
||
| TEST_F(MetadataStoreTest, EmptyStoreThrowsError) { |
test/store_unittest.cc
Outdated
| EXPECT_THROW(metadata_map_after.at(mr2), std::out_of_range); | ||
| } | ||
|
|
||
| TEST(MetadataTest, MetadataParsedCorrectly) { |
test/store_unittest.cc
Outdated
| EXPECT_EQ("hello", obj->Get<json::String>("f")); | ||
| } | ||
|
|
||
| TEST(MetadataTest, MetadataClonedCorrectly) { |
| json::object({{"f", json::string("hello")}})); | ||
| MetadataStore::Metadata m_clone = m.Clone(); | ||
| EXPECT_EQ(m.version, m_clone.version); | ||
| EXPECT_FALSE(m_clone.is_deleted); |
There was a problem hiding this comment.
This means the type could change though. Are we ok with that?
test/store_unittest.cc
Outdated
| MetadataStore::Metadata m_clone = m.Clone(); | ||
| EXPECT_EQ(m.version, m_clone.version); | ||
| EXPECT_FALSE(m_clone.is_deleted); | ||
| EXPECT_FALSE(m_clone.ignore); |
test/store_unittest.cc
Outdated
| TEST(MetadataTest, IgnoredMetadataCorrectlyCloned) { | ||
| MetadataStore::Metadata m = MetadataStore::Metadata::IGNORED(); | ||
| MetadataStore::Metadata m_clone = m.Clone(); | ||
| EXPECT_TRUE(m_clone.ignore); |
There was a problem hiding this comment.
Same comment as above.
test/store_unittest.cc
Outdated
| EXPECT_EQ(mr, store.LookupResource("id")); | ||
| } | ||
|
|
||
| TEST_F(MetadataStoreTest, EmptyStoreThrowsError) { |
test/store_unittest.cc
Outdated
| MonitoredResource mr("some_resource", {}); | ||
| store.UpdateResource({"id"}, mr); | ||
| EXPECT_EQ(mr, store.LookupResource("id")); | ||
| EXPECT_THROW(store.LookupResource("other_resource"), std::out_of_range); |
There was a problem hiding this comment.
It guards against the possibility that once a lookup fails, any subsequent lookup will also throw. Not that this is likely to happen, but unit tests are supposed to be paranoid and guard against corner cases.
test/store_unittest.cc
Outdated
| } | ||
|
|
||
| TEST_F(MetadataStoreTest, EmptyStoreThrowsError) { | ||
| EXPECT_THROW(store.LookupResource("some_resource_id"), std::out_of_range); |
There was a problem hiding this comment.
Let's use either missing_id or unknown_id consistently throughout.
test/store_unittest.cc
Outdated
|
|
||
| TEST_F(MetadataStoreTest, ResourceNotFoundThrowsError) { | ||
| MonitoredResource mr("some_resource", {}); | ||
| TEST_F(MetadataStoreTest, UnknownResourceIdThrowsError) { |
There was a problem hiding this comment.
Ah. See above. Let's name this one ResourceLookupFailsIndependently or something?
There was a problem hiding this comment.
This test actually conflates a few things, so let's decouple them. First, let's also add a separate test to just store a resource and return it successfully (KnownResourceIdFindsResource). Then, I'd name this test ResourceLookupFailuresAreIndependent, because it tests the interaction between lookup failures and lookup successes.
There was a problem hiding this comment.
Done. The first test ResourceWithOneIdCorrectlyStored is the same as KnownResourceIdFindsResource.
| json::object({{"f", json::string("hello")}})); | ||
| MetadataStore::Metadata m_clone = m.Clone(); | ||
| EXPECT_EQ(m.version, m_clone.version); | ||
| EXPECT_FALSE(m_clone.is_deleted); |
There was a problem hiding this comment.
Huh? Doesn't the construction test check that?
test/store_unittest.cc
Outdated
| time::rfc3339::FromString("2018-03-03T01:32:45.678901234Z"), | ||
| json::object({{"f", json::string("hello")}})); | ||
| MetadataStore::Metadata m_clone = m.Clone(); | ||
| EXPECT_EQ("default-version", m_clone.version); |
test/store_unittest.cc
Outdated
| m.created_at); | ||
| EXPECT_EQ(time::rfc3339::FromString("2018-03-03T01:32:45.678901234Z"), | ||
| m.collected_at); | ||
| const json::Object* obj = m.metadata->As<json::Object>(); |
There was a problem hiding this comment.
Replicating the comment from an earlier thread:
You can just compare the ToString values, i.e., EXPECT_EQ(m.metadata->ToString(), m_clone.metadata->ToString())...
test/store_unittest.cc
Outdated
| TEST(MetadataTest, IgnoredMetadataCorrectlyCloned) { | ||
| MetadataStore::Metadata m = MetadataStore::Metadata::IGNORED(); | ||
| MetadataStore::Metadata m_clone = m.Clone(); | ||
| EXPECT_TRUE(m_clone.ignore); |
There was a problem hiding this comment.
Doesn't the construction test already check that?
test/store_unittest.cc
Outdated
| EXPECT_EQ(mr, store.LookupResource("id")); | ||
| } | ||
|
|
||
| TEST_F(MetadataStoreTest, EmptyStoreThrowsError) { |
There was a problem hiding this comment.
That's the next test. This was just making sure that an empty store threw an error.
test/store_unittest.cc
Outdated
| m.created_at); | ||
| EXPECT_EQ(time::rfc3339::FromString("2018-03-03T01:32:45.678901234Z"), | ||
| m.collected_at); | ||
| const json::Object* obj = m.metadata->As<json::Object>(); |
test/store_unittest.cc
Outdated
| } | ||
|
|
||
| TEST_F(MetadataStoreTest, EmptyStoreThrowsError) { | ||
| EXPECT_THROW(store.LookupResource("some_resource_id"), std::out_of_range); |
test/store_unittest.cc
Outdated
| MonitoredResource mr("some_resource", {}); | ||
| store.UpdateResource({"id"}, mr); | ||
| EXPECT_EQ(mr, store.LookupResource("id")); | ||
| EXPECT_THROW(store.LookupResource("other_resource"), std::out_of_range); |
There was a problem hiding this comment.
Ah gotcha. Makes sense.
| json::object({{"f", json::string("hello")}})); | ||
| MetadataStore::Metadata m_clone = m.Clone(); | ||
| EXPECT_EQ(m.version, m_clone.version); | ||
| EXPECT_FALSE(m_clone.is_deleted); |
There was a problem hiding this comment.
True. But in case the implementation changes we want to explicitly check the field.
test/store_unittest.cc
Outdated
| TEST(MetadataTest, IgnoredMetadataCorrectlyCloned) { | ||
| MetadataStore::Metadata m = MetadataStore::Metadata::IGNORED(); | ||
| MetadataStore::Metadata m_clone = m.Clone(); | ||
| EXPECT_TRUE(m_clone.ignore); |
There was a problem hiding this comment.
True. But in case the implementation changes we want to explicitly check the field.
test/store_unittest.cc
Outdated
| time::rfc3339::FromString("2018-03-03T01:32:45.678901234Z"), | ||
| json::object({{"f", json::string("hello")}})); | ||
| MetadataStore::Metadata m_clone = m.Clone(); | ||
| EXPECT_EQ("default-version", m_clone.version); |
test/store_unittest.cc
Outdated
| EXPECT_EQ(mr, store.LookupResource("id")); | ||
| } | ||
|
|
||
| TEST_F(MetadataStoreTest, EmptyStoreThrowsError) { |
There was a problem hiding this comment.
The next test actually checks something else. It checks that known keys return data, while unknown keys punt. As opposed to this one, where every key is effectively unknown.
The way it's named now makes it seem like an empty store should always throw an error, where what it's testing is that every lookup will throw an error if the store is empty.
You could call this EmptyStoreLookupThrowsError if you want.
test/store_unittest.cc
Outdated
|
|
||
| TEST_F(MetadataStoreTest, ResourceNotFoundThrowsError) { | ||
| MonitoredResource mr("some_resource", {}); | ||
| TEST_F(MetadataStoreTest, UnknownResourceIdThrowsError) { |
There was a problem hiding this comment.
This test actually conflates a few things, so let's decouple them. First, let's also add a separate test to just store a resource and return it successfully (KnownResourceIdFindsResource). Then, I'd name this test ResourceLookupFailuresAreIndependent, because it tests the interaction between lookup failures and lookup successes.
test/store_unittest.cc
Outdated
| EXPECT_EQ(mr, store.LookupResource("id")); | ||
| } | ||
|
|
||
| TEST_F(MetadataStoreTest, EmptyStoreThrowsError) { |
| store.PurgeDeletedEntries(); | ||
| } | ||
|
|
||
| Configuration config; |
There was a problem hiding this comment.
Do we actually need a local copy of the config? I'm only seeing it being used in the constructor.
There was a problem hiding this comment.
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?
| TEST_F(MetadataStoreTest, MultipleUpdateMetadataChangesMetadataMap) { | ||
| MonitoredResource mr1("type1", {}); | ||
| MonitoredResource mr2("type2", {}); | ||
| MetadataStore::Metadata m1( |
There was a problem hiding this comment.
Observation, I got really confused when reading the comparisons, as I didn't pick up on mr1 and m1. Would it be worth using metadata1 and metadata2 to keep it incredibly obvious? I know it's not "consistent", but it would have helped when glancing through quickly.
There was a problem hiding this comment.
@bmoyles0117 you're right about the confusion, but I actually prefer m over metadata, because metadata.metadata reads really weird. Can we instead address it by renaming mr to resource? Sorry, @dhrupadb...
|
|
||
| TEST(MetadataTest, IgnoredMetadataCorrectlyCloned) { | ||
| MetadataStore::Metadata m = MetadataStore::Metadata::IGNORED(); | ||
| MetadataStore::Metadata m_clone = m.Clone(); |
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
Done. Added a check for m.
There was a problem hiding this comment.
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.
|
|
||
| TEST(MetadataTest, IgnoredMetadataCorrectlyCloned) { | ||
| MetadataStore::Metadata m = MetadataStore::Metadata::IGNORED(); | ||
| MetadataStore::Metadata m_clone = m.Clone(); |
There was a problem hiding this comment.
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.
| TEST_F(MetadataStoreTest, MultipleUpdateMetadataChangesMetadataMap) { | ||
| MonitoredResource mr1("type1", {}); | ||
| MonitoredResource mr2("type2", {}); | ||
| MetadataStore::Metadata m1( |
| store.PurgeDeletedEntries(); | ||
| } | ||
|
|
||
| Configuration config; |
|
|
||
| TEST(MetadataTest, IgnoredMetadataCorrectlyCloned) { | ||
| MetadataStore::Metadata m = MetadataStore::Metadata::IGNORED(); | ||
| MetadataStore::Metadata m_clone = m.Clone(); |
There was a problem hiding this comment.
@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.
| TEST_F(MetadataStoreTest, MultipleUpdateMetadataChangesMetadataMap) { | ||
| MonitoredResource mr1("type1", {}); | ||
| MonitoredResource mr2("type2", {}); | ||
| MetadataStore::Metadata m1( |
There was a problem hiding this comment.
@bmoyles0117 you're right about the confusion, but I actually prefer m over metadata, because metadata.metadata reads really weird. Can we instead address it by renaming mr to resource? Sorry, @dhrupadb...
| store.PurgeDeletedEntries(); | ||
| } | ||
|
|
||
| Configuration config; |
There was a problem hiding this comment.
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?
dhrupadb
force-pushed
the
dhrupadb-metadata-store-unittests
branch
from
78587f2 to
1d173e5
Compare
| store.PurgeDeletedEntries(); | ||
| } | ||
|
|
||
| Configuration config; |
| TEST_F(MetadataStoreTest, MultipleUpdateMetadataChangesMetadataMap) { | ||
| MonitoredResource mr1("type1", {}); | ||
| MonitoredResource mr2("type2", {}); | ||
| MetadataStore::Metadata m1( |
|
|
||
| TEST(MetadataTest, IgnoredMetadataCorrectlyCloned) { | ||
| MetadataStore::Metadata m = MetadataStore::Metadata::IGNORED(); | ||
| MetadataStore::Metadata m_clone = m.Clone(); |
There was a problem hiding this comment.
Done. Added a check for m.
|
|
||
| TEST(MetadataTest, IgnoredMetadataCorrectlyCloned) { | ||
| MetadataStore::Metadata m = MetadataStore::Metadata::IGNORED(); | ||
| MetadataStore::Metadata m_clone = m.Clone(); |
There was a problem hiding this comment.
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.
3 participants
No description provided.