Unittests for InstanceMonitoredResource and InstanceResourceMetadata by dhrupadb · Pull Request #105 · Stackdriver/metadata-agent

dhrupadb · 2018-03-30T00:02:13Z

No description provided.

igorpeshansky · 2018-03-30T01:05:51Z

  void StopUpdater();

 private:
+  friend class InstanceTest;


Let's add a blank line between this and the first function.

igorpeshansky · 2018-03-30T01:09:43Z

+  Environment env(config);
+  MonitoredResource mr_actual =
+      InstanceReader::InstanceResource(env);
+  MonitoredResource mr_expected(


You can just call this mr and inline the call to InstanceReader::InstanceResource(env) into the EXPECT_EQ. Also below.

Just in-lined the MonitoredResource call and changed mr_actual to mr.

You might as well inline the call being tested, then, and get rid of the local.

igorpeshansky · 2018-03-30T01:10:19Z

+
+class InstanceTest : public ::testing::Test {
+ protected:
+  const MetadataStore::Metadata&& GetResourceMetadata(


All of these can be static.

igorpeshansky · 2018-03-30T01:11:54Z

+
+class InstanceTest : public ::testing::Test {
+ protected:
+  const MetadataStore::Metadata&& GetResourceMetadata(


How about:

const MetadataStore::Metadata& GetMetadata( const MetadataUpdater::ResourceMetadata& rm) { return rm->metadata; }

? You don't actually need the ownership, you just want to get a reference to compare with.

igorpeshansky · 2018-03-30T01:13:30Z

+    return std::move(rm->metadata);
+  }
+
+  MonitoredResource GetMonitoredResource(


Ditto here: the ResourceMetadata object already owns that value — just get a const reference to look at (and avoid a copy):

const MonitoredResource& GetMonitoredResource( const MetadataUpdater::ResourceMetadata& rm) { return rm->resource; }

Also in GetResourceIds.

I don't see a const reference being returned. Also in GetResourceIds.

igorpeshansky · 2018-03-30T01:14:50Z

+}
+
+TEST_F(InstanceTest, GetInstanceMonitoredResourceWithEmptyConfig) {
+  Environment env(*(new Configuration()));


This leaks, please don't do that. Just say:

Configuration config; Environment env(config);

igorpeshansky · 2018-03-30T01:16:21Z

+      "InstanceZone: us-east1-b\n"
+  ));
+  InstanceReader reader(config);
+  const auto& result = reader.MetadataQuery();


Oops, and here you have to go the other way and copy, otherwise you are storing a reference to a temporary. So:

const auto result = reader.MetadataQuery();

igorpeshansky · 2018-03-30T01:17:18Z

+      "gce_instance",
+      {{"instance_id", "1234567891011"}, {"zone", "us-east1-b"}});
+  std::string vinit[] = {"", "1234567891011"};
+  std::vector<std::string> ids_exp = {vinit, std::end(vinit)};


const std::vector<std:string> ids_expected{"", "1234567891011};

The parentheses were optional, BTW.
I also suggested renaming it to ids_expected, but now that you've inlined the monitored resource, might as well inline this too:

EXPECT_EQ(std::vector<std::string>{"", "1234567891011"}, GetResourceIds(rm));

Don't have an easy way to test this now, but it might even auto-convert to a vector:

EXPECT_EQ({"", "1234567891011"}, GetResourceIds(rm));

Done. Sorry I missed the rename. Also inline auto-convert doesn't work actually.

Auto-convert doesn't, but inlining should.

igorpeshansky · 2018-03-30T01:18:02Z

+  std::string vinit[] = {"", "1234567891011"};
+  std::vector<std::string> ids_exp = {vinit, std::end(vinit)};
+  EXPECT_EQ(1, result.size());
+  const MetadataUpdater::ResourceMetadata& rm = result.at(0);


result[0] should also work, and is clearer.

ACEmilG · 2018-03-30T13:24:35Z

+  MonitoredResource mr_actual =
+      InstanceReader::InstanceResource(env);
+  MonitoredResource mr_expected(
+      "gce_instance", {{"instance_id", ""}, {"zone", ""}});


I'm confused on this expectation. Reading the code, I think this will actually make an HTTP call to the metadata server. Is it possible to execute this test without mocks set up?

+1. Totally missed that. Let's remove this test for now and add it when we have mocks.

Shouldn't this error out then though? The fact that there's a default gce_instance in the config that this falls over to, that should be behavior we test. I intended this test originally as one that caught an error when we didn't get a valid response from the API.

igorpeshansky · 2018-03-30T13:55:16Z

+  MonitoredResource mr_actual =
+      InstanceReader::InstanceResource(env);
+  MonitoredResource mr_expected(
+      "gce_instance", {{"instance_id", ""}, {"zone", ""}});


+1. Totally missed that. Let's remove this test for now and add it when we have mocks.

dhrupadb

PTAL

dhrupadb · 2018-03-30T14:52:45Z

  void StopUpdater();

 private:
+  friend class InstanceTest;


dhrupadb · 2018-03-30T15:01:01Z

+
+class InstanceTest : public ::testing::Test {
+ protected:
+  const MetadataStore::Metadata&& GetResourceMetadata(


dhrupadb · 2018-03-30T15:01:05Z

+
+class InstanceTest : public ::testing::Test {
+ protected:
+  const MetadataStore::Metadata&& GetResourceMetadata(


dhrupadb · 2018-03-30T15:01:27Z

+    return std::move(rm->metadata);
+  }
+
+  MonitoredResource GetMonitoredResource(


dhrupadb · 2018-03-30T15:07:12Z

+  Environment env(config);
+  MonitoredResource mr_actual =
+      InstanceReader::InstanceResource(env);
+  MonitoredResource mr_expected(


Just in-lined the MonitoredResource call and changed mr_actual to mr.

dhrupadb · 2018-03-30T15:07:37Z

+}
+
+TEST_F(InstanceTest, GetInstanceMonitoredResourceWithEmptyConfig) {
+  Environment env(*(new Configuration()));


dhrupadb · 2018-03-30T15:08:10Z

+      "InstanceZone: us-east1-b\n"
+  ));
+  InstanceReader reader(config);
+  const auto& result = reader.MetadataQuery();


dhrupadb · 2018-03-30T15:09:16Z

+      "gce_instance",
+      {{"instance_id", "1234567891011"}, {"zone", "us-east1-b"}});
+  std::string vinit[] = {"", "1234567891011"};
+  std::vector<std::string> ids_exp = {vinit, std::end(vinit)};


dhrupadb · 2018-03-30T15:09:41Z

+  std::string vinit[] = {"", "1234567891011"};
+  std::vector<std::string> ids_exp = {vinit, std::end(vinit)};
+  EXPECT_EQ(1, result.size());
+  const MetadataUpdater::ResourceMetadata& rm = result.at(0);


dhrupadb · 2018-03-30T15:10:49Z

+  MonitoredResource mr_actual =
+      InstanceReader::InstanceResource(env);
+  MonitoredResource mr_expected(
+      "gce_instance", {{"instance_id", ""}, {"zone", ""}});


Shouldn't this error out then though? The fact that there's a default gce_instance in the config that this falls over to, that should be behavior we test. I intended this test originally as one that caught an error when we didn't get a valid response from the API.

igorpeshansky · 2018-03-30T15:46:08Z

+    return std::move(rm->metadata);
+  }
+
+  MonitoredResource GetMonitoredResource(


I don't see a const reference being returned. Also in GetResourceIds.

igorpeshansky · 2018-03-30T15:46:39Z

+  Environment env(config);
+  MonitoredResource mr_actual =
+      InstanceReader::InstanceResource(env);
+  MonitoredResource mr_expected(


You might as well inline the call being tested, then, and get rid of the local.

igorpeshansky · 2018-03-30T15:49:54Z

+      "gce_instance",
+      {{"instance_id", "1234567891011"}, {"zone", "us-east1-b"}});
+  std::string vinit[] = {"", "1234567891011"};
+  std::vector<std::string> ids_exp = {vinit, std::end(vinit)};


The parentheses were optional, BTW.
I also suggested renaming it to ids_expected, but now that you've inlined the monitored resource, might as well inline this too:

EXPECT_EQ(std::vector<std::string>{"", "1234567891011"}, GetResourceIds(rm));

Don't have an easy way to test this now, but it might even auto-convert to a vector:

EXPECT_EQ({"", "1234567891011"}, GetResourceIds(rm));

igorpeshansky · 2018-03-30T15:50:54Z

+  EXPECT_EQ(1, result.size());
+  const MetadataUpdater::ResourceMetadata& rm = result[0];
+  EXPECT_EQ(MonitoredResource("gce_instance", {
+      {"instance_id", "1234567891011"},


Let's do a 2-space indent here.

igorpeshansky · 2018-03-30T15:51:38Z

+      {"zone", "us-east1-b"}
+  }), GetMonitoredResource(rm));
+  EXPECT_EQ(ids_exp, GetResourceIds(rm));
+  EXPECT_EQ(MetadataStore::Metadata::IGNORED().ignore,


Eh? Just say EXPECT_TRUE(GetResourceMetadata(rm).ignore)...

dhrupadb

PTAL

dhrupadb · 2018-03-30T16:13:30Z

+      {"zone", "us-east1-b"}
+  }), GetMonitoredResource(rm));
+  EXPECT_EQ(ids_exp, GetResourceIds(rm));
+  EXPECT_EQ(MetadataStore::Metadata::IGNORED().ignore,


dhrupadb · 2018-03-30T16:13:34Z

+  EXPECT_EQ(1, result.size());
+  const MetadataUpdater::ResourceMetadata& rm = result[0];
+  EXPECT_EQ(MonitoredResource("gce_instance", {
+      {"instance_id", "1234567891011"},


dhrupadb · 2018-03-30T16:15:48Z

+    return std::move(rm->metadata);
+  }
+
+  MonitoredResource GetMonitoredResource(


dhrupadb · 2018-03-30T16:17:59Z

+      "gce_instance",
+      {{"instance_id", "1234567891011"}, {"zone", "us-east1-b"}});
+  std::string vinit[] = {"", "1234567891011"};
+  std::vector<std::string> ids_exp = {vinit, std::end(vinit)};


Done. Sorry I missed the rename. Also inline auto-convert doesn't work actually.

igorpeshansky · 2018-03-30T19:19:53Z

+                           std::move(other.metadata_)) {}
+
+    const MetadataStore::Metadata& metadata() const {
+      return metadata_;


You should be able to put the whole function on one line, saving some vertical space. These are accessors, they don't deserve it. 😄 No need to put blank lines between them either.

igorpeshansky · 2018-03-30T19:22:59Z

+    }
+
+   private:
+    friend class MetadataUpdater;


How about adding a comment like // Needs write access to metadata_.?

Let's make this a line comment instead (on the same line as the friend declaration).

igorpeshansky · 2018-03-30T19:23:41Z

+  ));
+  InstanceReader reader(config);
+  const auto result = reader.MetadataQuery();
+  const std::vector<std::string> ids_expected({"", "1234567891011"});


This should be right next to the EXPECT_EQ if you're not inlining it.

igorpeshansky · 2018-03-30T19:24:57Z

+      "gce_instance",
+      {{"instance_id", "1234567891011"}, {"zone", "us-east1-b"}});
+  std::string vinit[] = {"", "1234567891011"};
+  std::vector<std::string> ids_exp = {vinit, std::end(vinit)};


Auto-convert doesn't, but inlining should.

dhrupadb

PTAL

dhrupadb · 2018-03-30T19:32:20Z

+                           std::move(other.metadata_)) {}
+
+    const MetadataStore::Metadata& metadata() const {
+      return metadata_;


dhrupadb · 2018-03-30T19:32:54Z

+    }
+
+   private:
+    friend class MetadataUpdater;


dhrupadb · 2018-03-30T19:33:35Z

+  ));
+  InstanceReader reader(config);
+  const auto result = reader.MetadataQuery();
+  const std::vector<std::string> ids_expected({"", "1234567891011"});


dhrupadb · 2018-03-30T19:35:09Z

+  Environment env(config);
+  MonitoredResource mr_actual =
+      InstanceReader::InstanceResource(env);
+  MonitoredResource mr_expected(


dhrupadb · 2018-03-30T19:35:17Z

+      "gce_instance",
+      {{"instance_id", "1234567891011"}, {"zone", "us-east1-b"}});
+  std::string vinit[] = {"", "1234567891011"};
+  std::vector<std::string> ids_exp = {vinit, std::end(vinit)};


igorpeshansky · 2018-03-30T19:36:38Z

+    }
+
+   private:
+    friend class MetadataUpdater;


Let's make this a line comment instead (on the same line as the friend declaration).

igorpeshansky · 2018-03-30T19:38:45Z

  void UpdateMetadataCallback(ResourceMetadata&& result) {
-    store_->UpdateMetadata(result.resource, std::move(result.metadata));
+    store_->UpdateMetadata(result.resource_,
+                           std::move(result.metadata_));


Should fit on one line.

dhrupadb

PTAL

dhrupadb · 2018-03-30T19:41:08Z

+    }
+
+   private:
+    friend class MetadataUpdater;


dhrupadb · 2018-03-30T19:41:15Z

  void UpdateMetadataCallback(ResourceMetadata&& result) {
-    store_->UpdateMetadata(result.resource, std::move(result.metadata));
+    store_->UpdateMetadata(result.resource_,
+                           std::move(result.metadata_));


ACEmilG

LGTM

igorpeshansky

LGTM

dhrupadb · 2018-03-30T19:51:02Z

Rebased.

dhrupadb requested review from ACEmilG, bmoyles0117 and igorpeshansky March 30, 2018 00:02