Add test for GetNodeMetadata in kubernetes #102

ACEmilG · 2018-03-29T13:49:33Z

No description provided.

igorpeshansky · 2018-03-29T15:50:55Z

test/Makefile

@@ -63,6 +75,9 @@ init-submodules:
 $(YAML_CPP_LIBS):
 	cd $(SRC_DIR) && $(MAKE) $@

+$(CPP_NETLIB_LIBS):


Let's keep these in the same order as src/Makefile (i.e., move this above the $(YAML_CPP_LIBS) target).

igorpeshansky · 2018-03-29T15:51:45Z

test/Makefile

@@ -83,6 +98,12 @@ base64_unittest: $(GTEST_LIB) base64_unittest.o $(SRC_DIR)/base64.o
 	$(CXX) $(LDFLAGS) $^ $(LDLIBS) -o $@
 configuration_unittest: $(GTEST_LIB) $(YAML_CPP_LIBS) configuration_unittest.o $(SRC_DIR)/configuration.o
 	$(CXX) $(LDFLAGS) $^ $(LDLIBS) -o $@
+kubernetes_unittest: $(GTEST_LIB) kubernetes_unittest.o $(SRC_DIR)/kubernetes.o
+	$(SRC_DIR)/json.o $(SRC_DIR)/logging.o $(SRC_DIR)/environment.o


I think you meant these as a continuation of the dependencies, right?.. Let's just keep them all on one line for now, even if it runs over 80 characters.

igorpeshansky · 2018-03-29T15:52:28Z

test/kubernetes_unittest.cc

@@ -0,0 +1,70 @@
+#include "../src/kubernetes.h"
+#include "../src/updater.h"
+#include "../src/configuration.h"


Let's alphabetize these.

igorpeshansky · 2018-03-29T15:53:53Z

test/kubernetes_unittest.cc

+        {"version", json::string("TestVersion")},
+        {"raw", json::object({
+          {"infrastructureResource",
+           InstanceReader::InstanceResource(environment).ToJSON()},


Just construct the instance resource JSON, rather than replicating the construction from the source. We can test this function separately.

Not sure what you mean here - can you give some psudocode?

Better — I can give you code 😄:

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

Done. However, to me, this seems more like testing the InstanceReader::InstanceResource function to me; If that function changes, this test will break.

We are actually testing that the metadata for a Kubernetes node looks as expected, regardless of the InstanceReader::InstanceResource implementation. If that function's implementation changes, we'll need to fix the call, not the test.

igorpeshansky · 2018-03-29T15:55:09Z

test/kubernetes_unittest.cc

+  json::value node_json = json::object({
+    {"metadata", json::object({
+      {"name", json::string("testname")},
+      {"creationTimestamp", json::string("2018-03-03T01:23:45.678901234Z")}


Let's keep trailing commas in these key/value pairs for inline JSON. Also below.

igorpeshansky · 2018-03-29T16:20:40Z

test/kubernetes_unittest.cc

+
+TEST_F(KubernetesTest, GetNodeMetadata) {
+  Configuration config(std::stringstream(
+    "KubernetesClusterName: 'TestClusterName'\n"


Single quotes behave weirdly in YAML. Since these are all bare words, let's just not quote them at all?..

Removed quotes.

igorpeshansky · 2018-03-29T16:21:20Z

test/kubernetes_unittest.cc

+    })}
+  });
+  const json::Object* node = node_json->As<json::Object>();
+  MetadataUpdater::ResourceMetadata metadata(


const auto metadata = ...

You forgot to change the parens to =...

igorpeshansky · 2018-03-29T16:22:14Z

test/kubernetes_unittest.cc

+                                {"node_name", "testname"},
+                                {"location", "TestClusterLocation"}
+                              }));
+  EXPECT_EQ(metadata.metadata.version, "TestVersion");


metadata.metadata looks weird (and metadata.metadata.metadata even weirder 😄). How about we call it rmd (for "resource metadata")?

While I agree it looks weird, I think changing the name to rmd would be more confusing. I'm not sure how to get around having metadata.metadata.metadata, since that really seems to be what this is.

How about just m? It's a test, so variable naming can be a little looser.

Agreed, changed.

igorpeshansky · 2018-03-29T16:24:34Z

test/Makefile

+    -lcppnetlib-client-connections -lcppnetlib-server-parsers -lnetwork-uri \
+    -lpthread -lboost_program_options -lboost_system -lboost_thread \
+    -lboost_filesystem -lyaml-cpp -lyajl -lcrypto -lssl
+LDFLAGS=-L$(CPP_NETLIB_LIBDIR) -L$(NETWORK_URI_LIBDIR) -L$(YAML_CPP_LIBDIR)


Let's put LDFLAGS above LDLIBS to make it consistent with src/Makefile (yes, I know this pre-dates your change, but you're already touching both lines).

igorpeshansky · 2018-03-29T16:26:30Z

test/Makefile

-LDLIBS=-lpthread -lboost_program_options -lyaml-cpp -lyajl
-LDFLAGS=-L$(YAML_CPP_LIBDIR)
+LDLIBS=\
+    -lcppnetlib-client-connections -lcppnetlib-server-parsers -lnetwork-uri \


Let's make the order of the libraries match the one in src/Makefile exactly (since the set is now identical).

igorpeshansky · 2018-03-29T19:46:11Z

test/kubernetes_unittest.cc

+    })}
+  });
+  const json::Object* node = node_json->As<json::Object>();
+  MetadataUpdater::ResourceMetadata metadata(


You forgot to change the parens to =...

igorpeshansky · 2018-03-29T19:46:44Z

test/kubernetes_unittest.cc

+                                {"node_name", "testname"},
+                                {"location", "TestClusterLocation"}
+                              }));
+  EXPECT_EQ(metadata.metadata.version, "TestVersion");


How about just m? It's a test, so variable naming can be a little looser.

igorpeshansky · 2018-03-29T20:24:49Z

test/kubernetes_unittest.cc

+    "KubernetesClusterName: TestClusterName\n"
+    "KubernetesClusterLocation: TestClusterLocation\n"
+    "MetadataIngestionRawContentVersion: TestVersion\n"
+    "InstanceZone: TestZone\n"


Should also add "InstanceResourceType: gce_instance\n".

igorpeshansky · 2018-03-29T20:33:33Z

test/kubernetes_unittest.cc

+        {"version", json::string("TestVersion")},
+        {"raw", json::object({
+          {"infrastructureResource",
+           InstanceReader::InstanceResource(environment).ToJSON()},


Better — I can give you code 😄:

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

igorpeshansky · 2018-03-29T20:43:51Z

test/kubernetes_unittest.cc

+        })},
+      })},
+      {"api", json::object({
+        {"version", json::string("1.6")},  // Hard-coded in kubernetes.cc


Nit: still missing the trailing ..

igorpeshansky · 2018-03-30T02:20:47Z

test/kubernetes_unittest.cc

+namespace google {
+
+class KubernetesTest : public ::testing::Test {
+  protected:


Please indent access modifiers by 1 space, and the class body by 2 spaces (style guide).

igorpeshansky · 2018-03-30T02:22:09Z

test/kubernetes_unittest.cc

+    "KubernetesClusterLocation: TestClusterLocation\n"
+    "MetadataIngestionRawContentVersion: TestVersion\n"
+    "InstanceZone: TestZone\n"
+    "InstanceResourceType: gce_instance\n"


Let's put this above InstanceZone.

igorpeshansky · 2018-03-30T02:25:26Z

test/kubernetes_unittest.cc

+        {"version", json::string("TestVersion")},
+        {"raw", json::object({
+          {"infrastructureResource",
+           InstanceReader::InstanceResource(environment).ToJSON()},


We are actually testing that the metadata for a Kubernetes node looks as expected, regardless of the InstanceReader::InstanceResource implementation. If that function's implementation changes, we'll need to fix the call, not the test.

igorpeshansky · 2018-03-30T02:27:08Z

test/kubernetes_unittest.cc

@@ -0,0 +1,72 @@
+#include "../src/configuration.h"
+#include "../src/instance.h"


No longer used.

igorpeshansky

LGTM

dhrupadb

One minor nit comment, otherwise
LGTM 👌

dhrupadb · 2018-03-30T14:43:23Z

test/kubernetes_unittest.cc

+    "InstanceId: TestID\n"
+  ));
+  Environment environment(config);
+  KubernetesReader reader(config, nullptr);


Let's add a comment with what we're nulling out here -- makes it easier to understand.

Add test for GetNodeMetadata in kubernetes

fec1845

igorpeshansky suggested changes Mar 29, 2018

View reviewed changes

Addressed code review comments

b0a0541

igorpeshansky suggested changes Mar 29, 2018

View reviewed changes

ACEmilG added 3 commits March 29, 2018 17:28

Added library dependencies

f7861ce

Further code review comments

dd19b5d

Fixed spacing in KubernetesTest

b0f1415

igorpeshansky suggested changes Mar 30, 2018

View reviewed changes

Code review changes

7306ba0

igorpeshansky requested a review from dhrupadb March 30, 2018 13:48

igorpeshansky approved these changes Mar 30, 2018

View reviewed changes

dhrupadb approved these changes Mar 30, 2018

View reviewed changes

Added clarifying comment

12742eb

ACEmilG merged commit f6b9b59 into master Mar 30, 2018

ACEmilG deleted the ACEmilG-GetNodeMetadata-unittest branch March 30, 2018 14:59

		@@ -0,0 +1,72 @@
		#include "../src/configuration.h"
		#include "../src/instance.h"

Add test for GetNodeMetadata in kubernetes #102

Add test for GetNodeMetadata in kubernetes #102

Conversation

ACEmilG commented Mar 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ACEmilG Mar 29, 2018 • edited by igorpeshansky Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igorpeshansky left a comment

Choose a reason for hiding this comment

dhrupadb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ACEmilG Mar 29, 2018 •

edited by igorpeshansky

Loading