Added test for ComputePodAssociations #109

ACEmilG · 2018-03-30T18:05:32Z

No description provided.

igorpeshansky · 2018-03-30T18:32:30Z

test/kubernetes_unittest.cc

  }
+
+  json::value ComputePodAssociations(const json::Object* pod,
+				     const KubernetesReader& reader) {


Indentation. Want to switch your editor config to expand tabs to spaces? Also below.

Fixed all over

igorpeshansky · 2018-03-30T18:33:53Z

test/kubernetes_unittest.cc

+    "InstanceId: TestID\n"
+  ));
+  Environment environment(config);
+  KubernetesReader reader(config, nullptr);  // Don't need HealthChecker.


The statement in the comment is kind of obvious from you setting it to nullptr, but if you choose to keep it, please update the other place where you do the same.

It should already be commented in all other locations.

This seems to be the only place, FWIW.

igorpeshansky · 2018-03-30T18:34:47Z

test/kubernetes_unittest.cc

+  Environment environment(config);
+  KubernetesReader reader(config, nullptr);  // Don't need HealthChecker.
+  const std::string encoded_ref = boost::algorithm::join(
+      std::vector<std::string>{api_version, kind, uid1}, "/");


Let's avoid this kind of logic in tests. Anything more complicated than "the same string goes in and out" is probably not worth the readability hit. Just use literal strings.

Agreed, done.

igorpeshansky · 2018-03-30T18:35:31Z

test/kubernetes_unittest.cc

+    return reader.ComputePodAssociations(pod);
+  }
+
+  void InsertIntoOwners(const std::string& key, json::value& value,


UpdateOwnersCache.

igorpeshansky · 2018-03-30T18:36:11Z

test/kubernetes_unittest.cc

+  const std::string encoded_ref = boost::algorithm::join(
+      std::vector<std::string>{api_version, kind, uid1}, "/");
+  json::value controller = json::object({
+          {"controller", json::boolean(true)},


2-space indent, please.

igorpeshansky · 2018-03-30T18:36:36Z

test/kubernetes_unittest.cc

+    })},
+  });
+
+  auto associations = ComputePodAssociations(pod->As<json::Object>(), reader);


const auto

igorpeshansky · 2018-03-30T18:37:21Z

test/kubernetes_unittest.cc

+
+  auto associations = ComputePodAssociations(pod->As<json::Object>(), reader);
+
+  EXPECT_EQ(associations->ToString(), json::object({


I would suggest pulling this json::object out into an expected_associations local.

igorpeshansky · 2018-03-30T18:38:11Z

test/kubernetes_unittest.cc

+
+  auto associations = ComputePodAssociations(pod->As<json::Object>(), reader);
+
+  EXPECT_EQ(associations->ToString(), json::object({


Let's stick with the EXPECT_EQ(expected, actual) parameter order throughout.

igorpeshansky

One minor formatting nit.

igorpeshansky · 2018-03-30T19:31:25Z

test/kubernetes_unittest.cc

+    "InstanceId: TestID\n"
+  ));
+  Environment environment(config);
+  KubernetesReader reader(config, nullptr);  // Don't need HealthChecker.


This seems to be the only place, FWIW.

igorpeshansky · 2018-03-30T19:33:55Z

test/kubernetes_unittest.cc

+    })},
+    {"version", json::string("TestVersion")},
+  });
+  const auto associations = ComputePodAssociations(pod->As<json::Object>(), reader);


Let's stay within 80 characters. I'd break after the =, personally...

igorpeshansky

Minor nits.

igorpeshansky · 2018-03-30T19:54:13Z

test/kubernetes_unittest.cc

    return reader.GetNodeMetadata(node, collected_at, is_deleted);
  }
+
+  json::value ComputePodAssociations(const json::Object* pod,


Just noticed — let's make reader the first argument, both here and in UpdateOwnersCache.

igorpeshansky · 2018-03-30T19:54:35Z

test/kubernetes_unittest.cc

+    return reader.ComputePodAssociations(pod);
+  }
+
+  void UpdateOwnersCache(const std::string& key, json::value& value,


const json::value& value.

igorpeshansky · 2018-03-30T19:55:02Z

test/kubernetes_unittest.cc

 }
+
+TEST_F(KubernetesTest, ComputePodAssociations) {
+  const std::string api_version = "1.2.3";


Let's inline these as well and get rid of the local variables.

igorpeshansky · 2018-03-30T19:55:31Z

test/kubernetes_unittest.cc

+      {"infrastructureResource", json::object({
+        {"labels", json::object({
+          {"instance_id", json::string("TestID")},
+          {"zone", json::string("TestZone")}


Let's keep the trailing comma.

igorpeshansky

One more tiny cleanup.

igorpeshansky · 2018-03-30T20:13:14Z

test/kubernetes_unittest.cc

+  ));
+  Environment environment(config);
+  KubernetesReader reader(config, nullptr);  // Don't need HealthChecker.
+  const std::string encoded_ref = "1.2.3/TestKind/TestUID1";


Let's move this to just before the call to UpdateOwnersCache. Or, better yet, just inline the value into the call.

igorpeshansky · 2018-03-30T20:13:55Z

test/kubernetes_unittest.cc

+  Environment environment(config);
+  KubernetesReader reader(config, nullptr);  // Don't need HealthChecker.
+  const std::string encoded_ref = "1.2.3/TestKind/TestUID1";
+  json::value controller = json::object({


Let's just inline the json::object() call into the UpdateOwnersCache call.

Actually: we use controller in another location, and thus I think its better to keep as a variable.

Ah, you're right, but then let's use std::move(controller) instead of controller->Clone() below (no point in making gratuitous copies).

Just pushed a commit with a fix.

igorpeshansky · 2018-03-30T21:20:42Z

test/kubernetes_unittest.cc

+  Environment environment(config);
+  KubernetesReader reader(config, nullptr);  // Don't need HealthChecker.
+  const std::string encoded_ref = "1.2.3/TestKind/TestUID1";
+  json::value controller = json::object({


Ah, you're right, but then let's use std::move(controller) instead of controller->Clone() below (no point in making gratuitous copies).

supriyagarg

LGTM

igorpeshansky

LGTM

igorpeshansky · 2018-03-31T21:05:45Z

test/kubernetes_unittest.cc

+  Environment environment(config);
+  KubernetesReader reader(config, nullptr);  // Don't need HealthChecker.
+  const std::string encoded_ref = "1.2.3/TestKind/TestUID1";
+  json::value controller = json::object({


Just pushed a commit with a fix.

Added test for ComputePodAssociations

080d0eb