Support configuration testing #80

ACEmilG · 2018-03-14T21:15:15Z

No description provided.

supriyagarg

Minor comment.

supriyagarg · 2018-03-14T23:02:11Z

test/configuration_unittest.cc

+  TestDefaultConfig(config);
+}
+
+TEST(ConfigurationTest, SpecificTest) {


Rename SpecificTest to PopulatedYamlTest or similar for parity with the previous name.

Or PopulatedConfigFileTest...

Changed tests to "NoConfigTest", "EmptyConfigTest", "PopulatedConfigTest"

igorpeshansky · 2018-03-15T00:55:08Z

src/configuration.h

  MetadataAgentConfiguration();
  int ParseArguments(int ac, char** av);
+  void ParseConfigFile(const std::string& filename);
+  void ParseYamlConfig(YAML::Node config);


I would prefer to not expose the implementation details in the header, but instead define

void ParseConfiguration(std::istream& input);

and use YAML::Load() instead of YAML::LoadFile() in the implementation of ParseConfigFile. Then you can just use std::stringstream in the test.

igorpeshansky · 2018-03-15T00:55:29Z

src/configuration.h

  }

 private:
-  void ParseConfigFile(const std::string& filename);


Keep this as private and add the test class as a friend instead. I think there's even a FRIEND_TEST macro...

igorpeshansky · 2018-03-15T01:00:48Z

test/Makefile

 CPP_NETLIB_DIR=$(LIBDIR)/cpp-netlib
 YAML_CPP_DIR=$(LIBDIR)/yaml-cpp
+YAML_CPP_LIBDIR=$(YAML_CPP_DIR)
+SUBMODULE_DIRS=$(CPP_NETLIB_DIR) $(YAML_CPP_DIR)


Is this used anywhere? If not, let's remove it.

igorpeshansky · 2018-03-15T01:01:12Z

test/Makefile

 # TODO: Factor out the common variables.
 CPP_NETLIB_DIR=$(LIBDIR)/cpp-netlib
 YAML_CPP_DIR=$(LIBDIR)/yaml-cpp
+YAML_CPP_LIBDIR=$(YAML_CPP_DIR)


We should really factor these into a common include file, but I am not asking you to do it now -- just musing.

igorpeshansky · 2018-03-15T01:07:57Z

test/Makefile

+	    -DMAKE_C_COMPILER=gcc -DCMAKE_CXX_COMPILER=g++ \
+	    -DYAML_CPP_BUILD_TOOLS=OFF
+
+$(YAML_CPP_LIBS): build-yaml-cpp


Instead of copying all of this make cruft from src/Makefile here, why not delegate:

$(YAML_CPP_LIBS): cd $(SRC_DIR) && $(MAKE) $@

?

Much better, changed.

igorpeshansky · 2018-03-15T01:10:24Z

test/configuration_unittest.cc

+  TestDefaultConfig(config);
+}
+
+TEST(ConfigurationTest, BlankYamlTest) {


How about BlankConfigFileTest?

Modified test names, see below

igorpeshansky · 2018-03-15T01:10:59Z

test/configuration_unittest.cc

+  TestDefaultConfig(config);
+}
+
+TEST(ConfigurationTest, SpecificTest) {


Or PopulatedConfigFileTest...

igorpeshansky · 2018-03-15T01:11:53Z

test/configuration_unittest.cc

+
+TEST(ConfigurationTest, SpecificTest) {
+  YAML::Node node = YAML::Load(
+      "ProjectId: TestProjectId\n"


You should also test that comments are skipped appropriately, possibly in another test case...

igorpeshansky · 2018-03-15T01:16:01Z

test/configuration_unittest.cc

+
+namespace {
+
+void TestDefaultConfig(google::MetadataAgentConfiguration& config) {


I'm not too happy about this, as it'll need to be updated when defaults change, but I don't have any better ideas for now, so let's keep it.

igorpeshansky · 2018-03-15T01:17:26Z

test/configuration_unittest.cc

+  EXPECT_EQ(false, config.MetadataReporterPurgeDeleted());
+  EXPECT_EQ(
+      "https://stackdriver.googleapis.com/"
+      "v1beta2/projects/{{project_id}}/resourceMetadata:batchUpdate",


We should eventually add a CheckConfiguration function that verifies, e.g., that these format strings have the appropriate substitution parameters, etc. Not asking you to do it now, though.

Agreed, left for later

igorpeshansky · 2018-03-15T21:26:11Z

src/configuration.cc

-  std::lock_guard<std::mutex> lock(mutex_);
  if (filename.empty()) return;
+  std::ifstream ifs;
+  ifs.open (filename, std::ifstream::in);


How about combining this with construction, i.e.:

std::ifstream input(filename); ParseConfiguration(input);

? BTW, in is the default mode.

Much better, thanks.

igorpeshansky · 2018-03-15T21:27:49Z

src/configuration.h

 private:
+  friend class MetadataAgentConfigurationTest;
  void ParseConfigFile(const std::string& filename);
-


Let's keep this blank line to separate functions from instance variables.

igorpeshansky · 2018-03-15T21:30:11Z

test/Makefile

 base64_unittest: $(GTEST_LIB) base64_unittest.o $(SRC_DIR)/base64.o
 	$(CXX) $(LDFLAGS) $^ $(LDLIBS) -o $@
-
+configuration_unittest: $(GTEST_LIB) configuration_unittest.o $(SRC_DIR)/configuration.o


I believe it's still relevant -- if you don't add that dependency, then make test in a fresh checkout will fail to link (because it won't rebuild $(YAML_CPP_LIBS)).

igorpeshansky · 2018-03-15T21:31:18Z

test/Makefile

+$(YAML_CPP_LIBS):
+	cd $(SRC_DIR) && $(MAKE) $@
+
+build-yaml-cpp: $(YAML_CPP_DIR)/Makefile


Since you're delegating $(YAML_CPP_LIBS) to src/Makefile, the build-yaml-cpp and yaml-cpp targets are no longer needed.

Fixed, thanks.

igorpeshansky · 2018-03-15T21:40:48Z

test/configuration_unittest.cc

+  static void SetUpTestCase() {}
+  static void TearDownTestCase() {}
+
+  void TestDefaultConfig(MetadataAgentConfiguration& config) {


This doesn't depend on any state in the fixture and only calls public methods. Is there any reason to not keep it as a standalone top-level function?

No particular reason - moved out of the class.

igorpeshansky · 2018-03-15T21:43:32Z

test/configuration_unittest.cc

+
+class MetadataAgentConfigurationTest : public ::testing::Test {
+ public:
+  void CallParseConfiguration(MetadataAgentConfiguration& config, std::istream& stream) {


Just ParseConfiguration is fine.

igorpeshansky · 2018-03-15T21:43:55Z

test/configuration_unittest.cc

+};
+
+TEST_F(MetadataAgentConfigurationTest, NoConfigTest) {
+  MetadataAgentConfiguration config;


You start every test with this. Might as well make this a field in the fixture.

igorpeshansky · 2018-03-15T21:45:16Z

test/configuration_unittest.cc

+  EXPECT_EQ(true, config.MetadataReporterPurgeDeleted());
+}
+
+TEST_F(MetadataAgentConfigurationTest, CommentSkippedTest) {


Let's also test for blank lines...

Added a new test

igorpeshansky · 2018-03-15T21:53:31Z

test/configuration_unittest.cc

+
+class MetadataAgentConfigurationTest : public ::testing::Test {
+ public:
+  void CallParseConfiguration(MetadataAgentConfiguration& config, std::istream& stream) {


As written, this can be static.

igorpeshansky · 2018-03-15T21:56:50Z

test/configuration_unittest.cc

+
+class MetadataAgentConfigurationTest : public ::testing::Test {
+ public:
+  void CallParseConfiguration(MetadataAgentConfiguration& config, std::istream& stream) {


Since you always pass in a stringstream initialized from a string, and you have to call this function anyway to get access to the private method, why not make this method take a string (well, const std::string&) and do the stream stuff in this method?

igorpeshansky · 2018-03-15T22:38:07Z

src/configuration.cc

  if (filename.empty()) return;

-  YAML::Node config = YAML::LoadFile(filename);
+  std::ifstream ifs(filename);


Let's replace ifs with input -- it reads better, IMO.

Agreed, done.

igorpeshansky · 2018-03-15T22:38:34Z

src/configuration.h

  }

 private:
+  friend class MetadataAgentConfigurationTest;


Please add a blank line after this to visually separate it from the functions.

igorpeshansky · 2018-03-15T22:40:42Z

test/Makefile

 base64_unittest: $(GTEST_LIB) base64_unittest.o $(SRC_DIR)/base64.o
 	$(CXX) $(LDFLAGS) $^ $(LDLIBS) -o $@
-
+configuration_unittest: $(GTEST_LIB) configuration_unittest.o $(SRC_DIR)/configuration.o $(YAML_CPP_LIBS)


Need to put $(YAML_CPP_LIBS) first, otherwise the build of configuration.o will fail because the includes won't find yaml-cpp/yaml.h.

Done, thanks.

igorpeshansky · 2018-03-15T22:41:02Z

test/Makefile

 	$(CXX) $(LDFLAGS) $^ $(LDLIBS) -o $@

-.PHONY: all test clean purge
+.PHONY: all test clean purge yaml-cpp


You've removed the yaml-cpp target, right?

Correct, removed this from phony list.

igorpeshansky · 2018-03-15T22:42:24Z

test/configuration_unittest.cc

+namespace google {
+
+class MetadataAgentConfigurationTest : public ::testing::Test {
+ public:


protected?

Sure, done.

igorpeshansky · 2018-03-15T22:45:55Z

test/configuration_unittest.cc

+class MetadataAgentConfigurationTest : public ::testing::Test {
+ public:
+  MetadataAgentConfiguration config;
+  static void ParseConfiguration(MetadataAgentConfiguration& config, const std::string configString) {


const std::string&.

igorpeshansky · 2018-03-15T22:46:12Z

test/configuration_unittest.cc

+class MetadataAgentConfigurationTest : public ::testing::Test {
+ public:
+  MetadataAgentConfiguration config;
+  static void ParseConfiguration(MetadataAgentConfiguration& config, const std::string configString) {


Actually, now that you have an instance variable, you might as well remove the config parameter and let it refer to the config instance variable. Of course, this can then no longer be static.

igorpeshansky · 2018-03-15T22:48:33Z

test/configuration_unittest.cc

+  }
+};
+
+static void TestDefaultConfig(MetadataAgentConfiguration& config) {


Also didn't notice earlier: this can take a const reference. However, if you make it an instance function, you should instead make it a const function (i.e.,

void VerifyDefaultConfig() const { ... }

).

igorpeshansky · 2018-03-15T22:48:57Z

test/configuration_unittest.cc

+  EXPECT_EQ(3, config.MetadataApiNumThreads());
+}
+
+TEST_F(MetadataAgentConfigurationTest, EmptyLineTest) {


s/Empty/Blank/.

igorpeshansky · 2018-03-15T22:49:33Z

test/configuration_unittest.cc

+
+TEST_F(MetadataAgentConfigurationTest, EmptyLineTest) {
+  ParseConfiguration(config,
+      "ProjectId: TestProjectId\n\n\n\n"


While this is technically correct, it's easier to visualize what's going on if you move each of the "\n" to its own line.

Agreed, changed.

igorpeshansky

Minor nits.

igorpeshansky · 2018-03-15T23:07:30Z

test/configuration_unittest.cc

+
+class MetadataAgentConfigurationTest : public ::testing::Test {
+ protected:
+  void ParseConfiguration(const std::string& configString) {


The Google style is to use snake_case for variables and parameter names. But you could just name it input instead.

Changed to input.

igorpeshansky · 2018-03-15T23:11:59Z

test/configuration_unittest.cc

+  ParseConfiguration(
+      "ProjectId: TestProjectId\n"
+      "MetadataApiNumThreads: 13\n"
+      "MetadataReporterPurgeDeleted: true");


Tip: if you put the closing parenthesis on the next line, you can add extra lines even after the last one without affecting git blame. If you choose to do this, apply it below as well.

Interesting. Changed below and above.

This only applied to cases where the only argument is a multi-line string with implicit concatenation. Not the EXPECT_EQ calls above (which have two arguments each, and the strings aren't even multi-line). Let's revert those two...

Reverted, sorry about that.

igorpeshansky · 2018-03-15T23:14:22Z

test/configuration_unittest.cc

+  MetadataAgentConfiguration config;
+};
+
+TEST_F(MetadataAgentConfigurationTest, NoConfigTest) {


Nit: the Test suffix is redundant (in the test names in all TEST_F lines).

… errors

igorpeshansky

One remaining issue.

igorpeshansky · 2018-03-15T23:42:39Z

test/configuration_unittest.cc

+  ParseConfiguration(
+      "ProjectId: TestProjectId\n"
+      "MetadataApiNumThreads: 13\n"
+      "MetadataReporterPurgeDeleted: true");


This only applied to cases where the only argument is a multi-line string with implicit concatenation. Not the EXPECT_EQ calls above (which have two arguments each, and the strings aren't even multi-line). Let's revert those two...

igorpeshansky

One last look unearthed a few more minor things in the Makefile...

igorpeshansky · 2018-03-16T17:49:35Z

test/Makefile

 CPPFLAGS+=-isystem $(GTEST_DIR)/include
-CXXFLAGS=-std=c++11 -g -pthread
-LDLIBS=-lpthread
+CXXFLAGS=-std=c++11 -g -pthread -I$(YAML_CPP_DIR)/include


The -I directive should go into CPPFLAGS.

igorpeshansky · 2018-03-16T17:49:56Z

test/Makefile


+YAML_CPP_LIBS=$(YAML_CPP_LIBDIR)/libyaml-cpp.a
+
+CMAKE=cmake


igorpeshansky · 2018-03-16T17:54:15Z

test/Makefile

 TESTS=\
      base64_unittest \
      format_unittest \
+      base64_unittest \


This looks like a merge conflict. Let's have only one of these (in alphabetical order).

Correct - fixed.

igorpeshansky

LGTM

igorpeshansky · 2018-03-16T19:51:51Z

FYI, I've backed out the merge and instead squashed all of the commits from this PR into one.

ACEmilG and others added 2 commits March 14, 2018 17:05

Support configuration testing

a7b35b2

Merge branch 'master' into ACEmilG-configuration-unit-tests

1413a5c

ACEmilG requested review from igorpeshansky and supriyagarg March 14, 2018 21:17

supriyagarg reviewed Mar 14, 2018

View reviewed changes

igorpeshansky suggested changes Mar 15, 2018

View reviewed changes

Changes based on PR #80

b0c6413

igorpeshansky suggested changes Mar 15, 2018

View reviewed changes

Addressed further code review comments on PR #80

6a9cc69

igorpeshansky suggested changes Mar 15, 2018

View reviewed changes

More code review on PR #80

ad50268

igorpeshansky suggested changes Mar 15, 2018

View reviewed changes

Addressed futher code review comments from PR #80, as well as linting…

4b55cda

… errors

igorpeshansky suggested changes Mar 15, 2018

View reviewed changes

Reverted unneeded whitespace changes

95e5d16

igorpeshansky suggested changes Mar 16, 2018

View reviewed changes

Makefile changes request on PR #80

8d09148

igorpeshansky approved these changes Mar 16, 2018

View reviewed changes

ACEmilG merged commit 6368a19 into master Mar 16, 2018

igorpeshansky pushed a commit that referenced this pull request Mar 16, 2018

Support configuration testing (#80)

c764582

igorpeshansky deleted the ACEmilG-configuration-unit-tests branch March 16, 2018 19:51


		namespace {

		void TestDefaultConfig(google::MetadataAgentConfiguration& config) {


		YAML_CPP_LIBS=$(YAML_CPP_LIBDIR)/libyaml-cpp.a

		CMAKE=cmake

Support configuration testing #80

Support configuration testing #80

Uh oh!

Conversation

ACEmilG commented Mar 14, 2018

Uh oh!

supriyagarg left a comment

Choose a reason for hiding this comment

Uh oh!

supriyagarg Mar 14, 2018 • edited by igorpeshansky Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

supriyagarg Mar 14, 2018 •

edited by igorpeshansky

Loading