From b6799202d1897933747ecf26431e82ab131cd16c Mon Sep 17 00:00:00 2001 From: Andrew Emil Date: Fri, 23 Mar 2018 17:06:12 -0400 Subject: [PATCH 01/10] Support for publicly accessible configuration parsing to support testing --- src/configuration.h | 8 +-- test/configuration_unittest.cc | 106 ++++++++++++++++----------------- 2 files changed, 55 insertions(+), 59 deletions(-) diff --git a/src/configuration.h b/src/configuration.h index aaf1ee48..4c3ba74e 100644 --- a/src/configuration.h +++ b/src/configuration.h @@ -30,6 +30,8 @@ class MetadataAgentConfiguration { // value means that parsing succeeded, but all of the arguments were handled // within the function and the program should exit with a success exit code. int ParseArguments(int ac, char** av); + void ParseConfiguration(std::istream& input); + void ParseConfigFile(const std::string& filename); // Shared configuration. const std::string& ProjectId() const { @@ -157,12 +159,6 @@ class MetadataAgentConfiguration { } private: - friend class MetadataAgentConfigurationTest; - friend class HealthCheckerUnittest; - - void ParseConfigFile(const std::string& filename); - void ParseConfiguration(std::istream& input); - mutable std::mutex mutex_; std::string project_id_; std::string credentials_file_; diff --git a/test/configuration_unittest.cc b/test/configuration_unittest.cc index 2ddfc4ca..a1293b7a 100644 --- a/test/configuration_unittest.cc +++ b/test/configuration_unittest.cc @@ -4,62 +4,60 @@ namespace google { -class MetadataAgentConfigurationTest : public ::testing::Test { - protected: - void ParseConfiguration(const std::string& input) { - std::stringstream stream(input); - config.ParseConfiguration(stream); - } +void VerifyDefaultConfig(const MetadataAgentConfiguration& config) { + EXPECT_EQ("", config.ProjectId()); + EXPECT_EQ("", config.CredentialsFile()); + EXPECT_EQ(3, config.MetadataApiNumThreads()); + EXPECT_EQ(8000, config.MetadataApiPort()); + EXPECT_EQ(".", config.MetadataApiResourceTypeSeparator()); + EXPECT_EQ(60, config.MetadataReporterIntervalSeconds()); + EXPECT_EQ(false, config.MetadataReporterPurgeDeleted()); + EXPECT_THAT(config.MetadataReporterUserAgent(), + ::testing::StartsWith("metadata-agent/")); + EXPECT_EQ("https://stackdriver.googleapis.com/" + "v1beta2/projects/{{project_id}}/resourceMetadata:batchUpdate", + config.MetadataIngestionEndpointFormat()); + EXPECT_EQ(8*1024*1024, config.MetadataIngestionRequestSizeLimitBytes()); + EXPECT_EQ("0.1", config.MetadataIngestionRawContentVersion()); + EXPECT_EQ(60*60, config.InstanceUpdaterIntervalSeconds()); + EXPECT_EQ("", config.InstanceResourceType()); + EXPECT_EQ(60, config.DockerUpdaterIntervalSeconds()); + EXPECT_EQ("unix://%2Fvar%2Frun%2Fdocker.sock/", + config.DockerEndpointHost()); + EXPECT_EQ("1.23", config.DockerApiVersion()); + EXPECT_EQ("limit=30", config.DockerContainerFilter()); + EXPECT_EQ(0, config.KubernetesUpdaterIntervalSeconds()); + EXPECT_EQ("https://kubernetes.default.svc", + config.KubernetesEndpointHost()); + EXPECT_EQ("", config.KubernetesPodLabelSelector()); + EXPECT_EQ("", config.KubernetesClusterName()); + EXPECT_EQ("", config.KubernetesClusterLocation()); + EXPECT_EQ("", config.KubernetesNodeName()); + EXPECT_EQ(true, config.KubernetesUseWatch()); + EXPECT_EQ("", config.InstanceId()); + EXPECT_EQ("", config.InstanceZone()); + EXPECT_EQ("/var/run/metadata-agent/health/unhealthy", config.HealthCheckFile()); +} - void VerifyDefaultConfig() const { - EXPECT_EQ("", config.ProjectId()); - EXPECT_EQ("", config.CredentialsFile()); - EXPECT_EQ(3, config.MetadataApiNumThreads()); - EXPECT_EQ(8000, config.MetadataApiPort()); - EXPECT_EQ(".", config.MetadataApiResourceTypeSeparator()); - EXPECT_EQ(60, config.MetadataReporterIntervalSeconds()); - EXPECT_EQ(false, config.MetadataReporterPurgeDeleted()); - EXPECT_THAT(config.MetadataReporterUserAgent(), - ::testing::StartsWith("metadata-agent/")); - EXPECT_EQ("https://stackdriver.googleapis.com/" - "v1beta2/projects/{{project_id}}/resourceMetadata:batchUpdate", - config.MetadataIngestionEndpointFormat()); - EXPECT_EQ(8*1024*1024, config.MetadataIngestionRequestSizeLimitBytes()); - EXPECT_EQ("0.1", config.MetadataIngestionRawContentVersion()); - EXPECT_EQ(60*60, config.InstanceUpdaterIntervalSeconds()); - EXPECT_EQ("", config.InstanceResourceType()); - EXPECT_EQ(60, config.DockerUpdaterIntervalSeconds()); - EXPECT_EQ("unix://%2Fvar%2Frun%2Fdocker.sock/", - config.DockerEndpointHost()); - EXPECT_EQ("1.23", config.DockerApiVersion()); - EXPECT_EQ("limit=30", config.DockerContainerFilter()); - EXPECT_EQ(0, config.KubernetesUpdaterIntervalSeconds()); - EXPECT_EQ("https://kubernetes.default.svc", - config.KubernetesEndpointHost()); - EXPECT_EQ("", config.KubernetesPodLabelSelector()); - EXPECT_EQ("", config.KubernetesClusterName()); - EXPECT_EQ("", config.KubernetesClusterLocation()); - EXPECT_EQ("", config.KubernetesNodeName()); - EXPECT_EQ(true, config.KubernetesUseWatch()); - EXPECT_EQ("", config.InstanceId()); - EXPECT_EQ("", config.InstanceZone()); - EXPECT_EQ("/var/run/metadata-agent/health/unhealthy", config.HealthCheckFile()); - } +static void ParseConfiguration(MetadataAgentConfiguration& config, const std::string& input) { + std::stringstream stream(input); + config.ParseConfiguration(stream); +} +TEST(MetadataAgentConfigurationTest, NoConfig) { MetadataAgentConfiguration config; -}; - -TEST_F(MetadataAgentConfigurationTest, NoConfig) { - VerifyDefaultConfig(); + VerifyDefaultConfig(config); } -TEST_F(MetadataAgentConfigurationTest, EmptyConfig) { - ParseConfiguration(""); - VerifyDefaultConfig(); +TEST(MetadataAgentConfigurationTest, EmptyConfig) { + MetadataAgentConfiguration config; + ParseConfiguration(config, ""); + VerifyDefaultConfig(config); } -TEST_F(MetadataAgentConfigurationTest, PopulatedConfig) { - ParseConfiguration( +TEST(MetadataAgentConfigurationTest, PopulatedConfig) { + MetadataAgentConfiguration config; + ParseConfiguration(config, "ProjectId: TestProjectId\n" "MetadataApiNumThreads: 13\n" "MetadataReporterPurgeDeleted: true\n" @@ -73,8 +71,9 @@ TEST_F(MetadataAgentConfigurationTest, PopulatedConfig) { EXPECT_EQ("/a/b/c", config.HealthCheckFile()); } -TEST_F(MetadataAgentConfigurationTest, CommentSkipped) { - ParseConfiguration( +TEST(MetadataAgentConfigurationTest, CommentSkipped) { + MetadataAgentConfiguration config; + ParseConfiguration(config, "ProjectId: TestProjectId\n" "#MetadataApiNumThreads: 13\n" "MetadataReporterPurgeDeleted: true\n" @@ -82,8 +81,9 @@ TEST_F(MetadataAgentConfigurationTest, CommentSkipped) { EXPECT_EQ(3, config.MetadataApiNumThreads()); } -TEST_F(MetadataAgentConfigurationTest, BlankLine) { - ParseConfiguration( +TEST(MetadataAgentConfigurationTest, BlankLine) { + MetadataAgentConfiguration config; + ParseConfiguration(config, "ProjectId: TestProjectId\n" "\n" "\n" From efc5250ba478b39c731adbea5bf4b1ad6ff35fe3 Mon Sep 17 00:00:00 2001 From: Andrew Emil Date: Fri, 23 Mar 2018 17:48:13 -0400 Subject: [PATCH 02/10] addressed code review comments --- src/configuration.cc | 13 +++++++++---- src/configuration.h | 5 +++-- test/configuration_unittest.cc | 13 ++++--------- 3 files changed, 16 insertions(+), 15 deletions(-) diff --git a/src/configuration.cc b/src/configuration.cc index f378bbe8..cabbcb2a 100644 --- a/src/configuration.cc +++ b/src/configuration.cc @@ -153,7 +153,7 @@ int MetadataAgentConfiguration::ParseArguments(int ac, char** av) { return -1; } - ParseConfigFile(config_file); + ParseConfigurationFile(config_file); return 0; } catch (const boost::program_options::error& arg_error) { std::cerr << arg_error.what() << std::endl; @@ -162,14 +162,19 @@ int MetadataAgentConfiguration::ParseArguments(int ac, char** av) { } } -void MetadataAgentConfiguration::ParseConfigFile(const std::string& filename) { +void MetadataAgentConfiguration::ParseConfigurationFile(const std::string& filename) { if (filename.empty()) return; std::ifstream input(filename); - ParseConfiguration(input); + ParseConfigurationStream(input); } -void MetadataAgentConfiguration::ParseConfiguration(std::istream& input) { +void MetadataAgentConfiguration::ParseConfigurationString(const std::string& input) { + std::stringstream stream(input); + ParseConfigurationStream(stream); +} + +void MetadataAgentConfiguration::ParseConfigurationStream(std::istream& input) { YAML::Node config = YAML::Load(input); std::lock_guard lock(mutex_); project_id_ = diff --git a/src/configuration.h b/src/configuration.h index 4c3ba74e..fc983610 100644 --- a/src/configuration.h +++ b/src/configuration.h @@ -30,8 +30,9 @@ class MetadataAgentConfiguration { // value means that parsing succeeded, but all of the arguments were handled // within the function and the program should exit with a success exit code. int ParseArguments(int ac, char** av); - void ParseConfiguration(std::istream& input); - void ParseConfigFile(const std::string& filename); + void ParseConfigurationStream(std::istream& input); + void ParseConfigurationString(const std::string& input); + void ParseConfigurationFile(const std::string& filename); // Shared configuration. const std::string& ProjectId() const { diff --git a/test/configuration_unittest.cc b/test/configuration_unittest.cc index a1293b7a..c3a83bdc 100644 --- a/test/configuration_unittest.cc +++ b/test/configuration_unittest.cc @@ -39,11 +39,6 @@ void VerifyDefaultConfig(const MetadataAgentConfiguration& config) { EXPECT_EQ("/var/run/metadata-agent/health/unhealthy", config.HealthCheckFile()); } -static void ParseConfiguration(MetadataAgentConfiguration& config, const std::string& input) { - std::stringstream stream(input); - config.ParseConfiguration(stream); -} - TEST(MetadataAgentConfigurationTest, NoConfig) { MetadataAgentConfiguration config; VerifyDefaultConfig(config); @@ -51,13 +46,13 @@ TEST(MetadataAgentConfigurationTest, NoConfig) { TEST(MetadataAgentConfigurationTest, EmptyConfig) { MetadataAgentConfiguration config; - ParseConfiguration(config, ""); + config.ParseConfigurationString(""); VerifyDefaultConfig(config); } TEST(MetadataAgentConfigurationTest, PopulatedConfig) { MetadataAgentConfiguration config; - ParseConfiguration(config, + config.ParseConfigurationString( "ProjectId: TestProjectId\n" "MetadataApiNumThreads: 13\n" "MetadataReporterPurgeDeleted: true\n" @@ -73,7 +68,7 @@ TEST(MetadataAgentConfigurationTest, PopulatedConfig) { TEST(MetadataAgentConfigurationTest, CommentSkipped) { MetadataAgentConfiguration config; - ParseConfiguration(config, + config.ParseConfigurationString( "ProjectId: TestProjectId\n" "#MetadataApiNumThreads: 13\n" "MetadataReporterPurgeDeleted: true\n" @@ -83,7 +78,7 @@ TEST(MetadataAgentConfigurationTest, CommentSkipped) { TEST(MetadataAgentConfigurationTest, BlankLine) { MetadataAgentConfiguration config; - ParseConfiguration(config, + config.ParseConfigurationString( "ProjectId: TestProjectId\n" "\n" "\n" From 5bafe430f93d6812f3a16fd369d484727bc5885b Mon Sep 17 00:00:00 2001 From: Andrew Emil Date: Mon, 26 Mar 2018 10:58:27 -0400 Subject: [PATCH 03/10] Support for constructor that takes istream --- src/configuration.cc | 10 +++++----- src/configuration.h | 7 ++++--- test/configuration_unittest.cc | 16 ++++++++-------- 3 files changed, 17 insertions(+), 16 deletions(-) diff --git a/src/configuration.cc b/src/configuration.cc index cabbcb2a..8c32bd3e 100644 --- a/src/configuration.cc +++ b/src/configuration.cc @@ -116,6 +116,11 @@ MetadataAgentConfiguration::MetadataAgentConfiguration() instance_zone_(kDefaultInstanceZone), health_check_file_(kDefaultHealthCheckFile) {} +MetadataAgentConfiguration::MetadataAgentConfiguration(std::istream& input) + : MetadataAgentConfiguration() { + ParseConfigurationStream(input); +} + int MetadataAgentConfiguration::ParseArguments(int ac, char** av) { std::string config_file; boost::program_options::options_description flags_desc; @@ -169,11 +174,6 @@ void MetadataAgentConfiguration::ParseConfigurationFile(const std::string& filen ParseConfigurationStream(input); } -void MetadataAgentConfiguration::ParseConfigurationString(const std::string& input) { - std::stringstream stream(input); - ParseConfigurationStream(stream); -} - void MetadataAgentConfiguration::ParseConfigurationStream(std::istream& input) { YAML::Node config = YAML::Load(input); std::lock_guard lock(mutex_); diff --git a/src/configuration.h b/src/configuration.h index fc983610..ed423bcf 100644 --- a/src/configuration.h +++ b/src/configuration.h @@ -24,15 +24,13 @@ namespace google { class MetadataAgentConfiguration { public: MetadataAgentConfiguration(); + MetadataAgentConfiguration(std::istream& input); // Parse the command line. // A zero return value means that parsing succeeded and the program should // proceed. A positive return value means that parsing failed. A negative // value means that parsing succeeded, but all of the arguments were handled // within the function and the program should exit with a success exit code. int ParseArguments(int ac, char** av); - void ParseConfigurationStream(std::istream& input); - void ParseConfigurationString(const std::string& input); - void ParseConfigurationFile(const std::string& filename); // Shared configuration. const std::string& ProjectId() const { @@ -160,6 +158,9 @@ class MetadataAgentConfiguration { } private: + void ParseConfigurationStream(std::istream& input); + void ParseConfigurationFile(const std::string& filename); + mutable std::mutex mutex_; std::string project_id_; std::string credentials_file_; diff --git a/test/configuration_unittest.cc b/test/configuration_unittest.cc index c3a83bdc..270aa2a3 100644 --- a/test/configuration_unittest.cc +++ b/test/configuration_unittest.cc @@ -45,20 +45,20 @@ TEST(MetadataAgentConfigurationTest, NoConfig) { } TEST(MetadataAgentConfigurationTest, EmptyConfig) { - MetadataAgentConfiguration config; - config.ParseConfigurationString(""); + std::istringstream stream(""); + MetadataAgentConfiguration config(stream); VerifyDefaultConfig(config); } TEST(MetadataAgentConfigurationTest, PopulatedConfig) { - MetadataAgentConfiguration config; - config.ParseConfigurationString( + std::istringstream stream( "ProjectId: TestProjectId\n" "MetadataApiNumThreads: 13\n" "MetadataReporterPurgeDeleted: true\n" "MetadataReporterUserAgent: \"foobar/foobaz\"\n" "HealthCheckFile: /a/b/c\n" ); + MetadataAgentConfiguration config(stream); EXPECT_EQ("TestProjectId", config.ProjectId()); EXPECT_EQ(13, config.MetadataApiNumThreads()); EXPECT_EQ(true, config.MetadataReporterPurgeDeleted()); @@ -67,23 +67,23 @@ TEST(MetadataAgentConfigurationTest, PopulatedConfig) { } TEST(MetadataAgentConfigurationTest, CommentSkipped) { - MetadataAgentConfiguration config; - config.ParseConfigurationString( + std::istringstream stream( "ProjectId: TestProjectId\n" "#MetadataApiNumThreads: 13\n" "MetadataReporterPurgeDeleted: true\n" ); + MetadataAgentConfiguration config(stream); EXPECT_EQ(3, config.MetadataApiNumThreads()); } TEST(MetadataAgentConfigurationTest, BlankLine) { - MetadataAgentConfiguration config; - config.ParseConfigurationString( + std::istringstream stream( "ProjectId: TestProjectId\n" "\n" "\n" "MetadataReporterPurgeDeleted: true\n" ); + MetadataAgentConfiguration config(stream); EXPECT_EQ("TestProjectId", config.ProjectId()); EXPECT_EQ(true, config.MetadataReporterPurgeDeleted()); } From 83798b9bba658d0b67954382c251d8a91b6d5d85 Mon Sep 17 00:00:00 2001 From: Andrew Emil Date: Mon, 26 Mar 2018 12:42:38 -0400 Subject: [PATCH 04/10] Move ParseArguments private to reflect encapsulation conversation --- src/configuration.h | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/configuration.h b/src/configuration.h index ed423bcf..1cb71798 100644 --- a/src/configuration.h +++ b/src/configuration.h @@ -19,18 +19,13 @@ #include #include -namespace google { +int main(int ac, char** av); +namespace google { class MetadataAgentConfiguration { public: MetadataAgentConfiguration(); MetadataAgentConfiguration(std::istream& input); - // Parse the command line. - // A zero return value means that parsing succeeded and the program should - // proceed. A positive return value means that parsing failed. A negative - // value means that parsing succeeded, but all of the arguments were handled - // within the function and the program should exit with a success exit code. - int ParseArguments(int ac, char** av); // Shared configuration. const std::string& ProjectId() const { @@ -158,8 +153,16 @@ class MetadataAgentConfiguration { } private: + friend int ::main(int ac, char** av); + void ParseConfigurationStream(std::istream& input); void ParseConfigurationFile(const std::string& filename); + // Parse the command line. + // A zero return value means that parsing succeeded and the program should + // proceed. A positive return value means that parsing failed. A negative + // value means that parsing succeeded, but all of the arguments were handled + // within the function and the program should exit with a success exit code. + int ParseArguments(int ac, char** av); mutable std::mutex mutex_; std::string project_id_; From daa4de4bd9e83096ea095157f20131e3ccdbc1e3 Mon Sep 17 00:00:00 2001 From: Andrew Emil Date: Mon, 26 Mar 2018 15:09:19 -0400 Subject: [PATCH 05/10] Addressed code review comments --- src/configuration.cc | 10 +++++----- src/configuration.h | 7 ++++--- test/configuration_unittest.cc | 6 ++---- 3 files changed, 11 insertions(+), 12 deletions(-) diff --git a/src/configuration.cc b/src/configuration.cc index 8c32bd3e..e41c8876 100644 --- a/src/configuration.cc +++ b/src/configuration.cc @@ -118,7 +118,7 @@ MetadataAgentConfiguration::MetadataAgentConfiguration() MetadataAgentConfiguration::MetadataAgentConfiguration(std::istream& input) : MetadataAgentConfiguration() { - ParseConfigurationStream(input); + ParseConfiguration(input); } int MetadataAgentConfiguration::ParseArguments(int ac, char** av) { @@ -158,7 +158,7 @@ int MetadataAgentConfiguration::ParseArguments(int ac, char** av) { return -1; } - ParseConfigurationFile(config_file); + ParseConfigFile(config_file); return 0; } catch (const boost::program_options::error& arg_error) { std::cerr << arg_error.what() << std::endl; @@ -167,14 +167,14 @@ int MetadataAgentConfiguration::ParseArguments(int ac, char** av) { } } -void MetadataAgentConfiguration::ParseConfigurationFile(const std::string& filename) { +void MetadataAgentConfiguration::ParseConfigFile(const std::string& filename) { if (filename.empty()) return; std::ifstream input(filename); - ParseConfigurationStream(input); + ParseConfiguration(input); } -void MetadataAgentConfiguration::ParseConfigurationStream(std::istream& input) { +void MetadataAgentConfiguration::ParseConfiguration(std::istream& input) { YAML::Node config = YAML::Load(input); std::lock_guard lock(mutex_); project_id_ = diff --git a/src/configuration.h b/src/configuration.h index 1cb71798..c3af0560 100644 --- a/src/configuration.h +++ b/src/configuration.h @@ -22,6 +22,7 @@ int main(int ac, char** av); namespace google { + class MetadataAgentConfiguration { public: MetadataAgentConfiguration(); @@ -153,10 +154,10 @@ class MetadataAgentConfiguration { } private: - friend int ::main(int ac, char** av); + friend int ::main(int, char**); // Calls ParseArguments. - void ParseConfigurationStream(std::istream& input); - void ParseConfigurationFile(const std::string& filename); + void ParseConfiguration(std::istream& input); + void ParseConfigFile(const std::string& filename); // Parse the command line. // A zero return value means that parsing succeeded and the program should // proceed. A positive return value means that parsing failed. A negative diff --git a/test/configuration_unittest.cc b/test/configuration_unittest.cc index 270aa2a3..d2ea7075 100644 --- a/test/configuration_unittest.cc +++ b/test/configuration_unittest.cc @@ -22,13 +22,11 @@ void VerifyDefaultConfig(const MetadataAgentConfiguration& config) { EXPECT_EQ(60*60, config.InstanceUpdaterIntervalSeconds()); EXPECT_EQ("", config.InstanceResourceType()); EXPECT_EQ(60, config.DockerUpdaterIntervalSeconds()); - EXPECT_EQ("unix://%2Fvar%2Frun%2Fdocker.sock/", - config.DockerEndpointHost()); + EXPECT_EQ("unix://%2Fvar%2Frun%2Fdocker.sock/", config.DockerEndpointHost()); EXPECT_EQ("1.23", config.DockerApiVersion()); EXPECT_EQ("limit=30", config.DockerContainerFilter()); EXPECT_EQ(0, config.KubernetesUpdaterIntervalSeconds()); - EXPECT_EQ("https://kubernetes.default.svc", - config.KubernetesEndpointHost()); + EXPECT_EQ("https://kubernetes.default.svc", config.KubernetesEndpointHost()); EXPECT_EQ("", config.KubernetesPodLabelSelector()); EXPECT_EQ("", config.KubernetesClusterName()); EXPECT_EQ("", config.KubernetesClusterLocation()); From c0bf4ecd62f79f2db9fc86a5c8df74ac85b6ab8b Mon Sep 17 00:00:00 2001 From: Andrew Emil Date: Mon, 26 Mar 2018 16:47:03 -0400 Subject: [PATCH 06/10] Support for inlining configuration stream, removed args from forward declaration of main --- src/configuration.h | 6 ++++-- test/configuration_unittest.cc | 18 +++++++----------- test/health_checker_unittest.cc | 29 ++++++++++++----------------- 3 files changed, 23 insertions(+), 30 deletions(-) diff --git a/src/configuration.h b/src/configuration.h index c3af0560..d8fdd77b 100644 --- a/src/configuration.h +++ b/src/configuration.h @@ -19,7 +19,7 @@ #include #include -int main(int ac, char** av); +int main(int, char**); namespace google { @@ -27,6 +27,8 @@ class MetadataAgentConfiguration { public: MetadataAgentConfiguration(); MetadataAgentConfiguration(std::istream& input); + MetadataAgentConfiguration(std::istream&& input) + : MetadataAgentConfiguration(input) {} // Shared configuration. const std::string& ProjectId() const { @@ -156,8 +158,8 @@ class MetadataAgentConfiguration { private: friend int ::main(int, char**); // Calls ParseArguments. - void ParseConfiguration(std::istream& input); void ParseConfigFile(const std::string& filename); + void ParseConfiguration(std::istream& input); // Parse the command line. // A zero return value means that parsing succeeded and the program should // proceed. A positive return value means that parsing failed. A negative diff --git a/test/configuration_unittest.cc b/test/configuration_unittest.cc index d2ea7075..751a09a7 100644 --- a/test/configuration_unittest.cc +++ b/test/configuration_unittest.cc @@ -43,20 +43,18 @@ TEST(MetadataAgentConfigurationTest, NoConfig) { } TEST(MetadataAgentConfigurationTest, EmptyConfig) { - std::istringstream stream(""); - MetadataAgentConfiguration config(stream); + MetadataAgentConfiguration config(std::istringstream("")); VerifyDefaultConfig(config); } TEST(MetadataAgentConfigurationTest, PopulatedConfig) { - std::istringstream stream( + MetadataAgentConfiguration config(std::istringstream( "ProjectId: TestProjectId\n" "MetadataApiNumThreads: 13\n" "MetadataReporterPurgeDeleted: true\n" "MetadataReporterUserAgent: \"foobar/foobaz\"\n" "HealthCheckFile: /a/b/c\n" - ); - MetadataAgentConfiguration config(stream); + )); EXPECT_EQ("TestProjectId", config.ProjectId()); EXPECT_EQ(13, config.MetadataApiNumThreads()); EXPECT_EQ(true, config.MetadataReporterPurgeDeleted()); @@ -65,23 +63,21 @@ TEST(MetadataAgentConfigurationTest, PopulatedConfig) { } TEST(MetadataAgentConfigurationTest, CommentSkipped) { - std::istringstream stream( + MetadataAgentConfiguration config(std::istringstream( "ProjectId: TestProjectId\n" "#MetadataApiNumThreads: 13\n" "MetadataReporterPurgeDeleted: true\n" - ); - MetadataAgentConfiguration config(stream); + )); EXPECT_EQ(3, config.MetadataApiNumThreads()); } TEST(MetadataAgentConfigurationTest, BlankLine) { - std::istringstream stream( + MetadataAgentConfiguration config(std::istringstream( "ProjectId: TestProjectId\n" "\n" "\n" "MetadataReporterPurgeDeleted: true\n" - ); - MetadataAgentConfiguration config(stream); + )); EXPECT_EQ("TestProjectId", config.ProjectId()); EXPECT_EQ(true, config.MetadataReporterPurgeDeleted()); } diff --git a/test/health_checker_unittest.cc b/test/health_checker_unittest.cc index b52a241b..d68f6e02 100644 --- a/test/health_checker_unittest.cc +++ b/test/health_checker_unittest.cc @@ -22,35 +22,30 @@ class HealthCheckerUnittest : public ::testing::Test { return health_checker.IsHealthy(); } - void SetIsolationPath(const std::string& isolation_path) { - isolation_path_ = isolation_path; - std::stringstream stream( - "HealthCheckFile: './" + isolation_path_ + "/unhealthy'"); - config_.ParseConfiguration(stream); - } - - std::string isolation_path_; - MetadataAgentConfiguration config_; }; +static const std::string IsolationPath(const std::string& test_name) { + return "HealthCheckFile: './" + test_name + "/unhealthy'"; +} + TEST_F(HealthCheckerUnittest, DefaultHealthy) { - SetIsolationPath(test_info_->name()); - HealthChecker health_checker(config_); + MetadataAgentConfiguration config(std::stringstream(IsolationPath(test_info_->name()))); + HealthChecker health_checker(config); EXPECT_TRUE(IsHealthy(health_checker)); Cleanup(&health_checker); } TEST_F(HealthCheckerUnittest, SimpleFailure) { - SetIsolationPath(test_info_->name()); - HealthChecker health_checker(config_); + MetadataAgentConfiguration config(std::stringstream(IsolationPath(test_info_->name()))); + HealthChecker health_checker(config); SetUnhealthy(&health_checker, "kubernetes_pod_thread"); EXPECT_FALSE(IsHealthy(health_checker)); Cleanup(&health_checker); } TEST_F(HealthCheckerUnittest, MultiFailure) { - SetIsolationPath(test_info_->name()); - HealthChecker health_checker(config_); + MetadataAgentConfiguration config(std::stringstream(IsolationPath(test_info_->name()))); + HealthChecker health_checker(config); EXPECT_TRUE(IsHealthy(health_checker)); SetUnhealthy(&health_checker, "kubernetes_pod_thread"); EXPECT_FALSE(IsHealthy(health_checker)); @@ -60,8 +55,8 @@ TEST_F(HealthCheckerUnittest, MultiFailure) { } TEST_F(HealthCheckerUnittest, FailurePersists) { - SetIsolationPath(test_info_->name()); - HealthChecker health_checker(config_); + MetadataAgentConfiguration config(std::stringstream(IsolationPath(test_info_->name()))); + HealthChecker health_checker(config); EXPECT_TRUE(IsHealthy(health_checker)); SetUnhealthy(&health_checker, "kubernetes_pod_thread"); EXPECT_FALSE(IsHealthy(health_checker)); From 7d08a61d8ad24b6fbf4c540ad44486fc26ce780c Mon Sep 17 00:00:00 2001 From: Andrew Emil Date: Tue, 27 Mar 2018 17:41:22 -0400 Subject: [PATCH 07/10] Added clarifying comment to constructor --- src/configuration.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/configuration.h b/src/configuration.h index d8fdd77b..214d4152 100644 --- a/src/configuration.h +++ b/src/configuration.h @@ -27,6 +27,7 @@ class MetadataAgentConfiguration { public: MetadataAgentConfiguration(); MetadataAgentConfiguration(std::istream& input); + // Used to accept inline construction of streams. MetadataAgentConfiguration(std::istream&& input) : MetadataAgentConfiguration(input) {} From 72c3d5848e289e318556b9f2d1edaa2d83002015 Mon Sep 17 00:00:00 2001 From: Andrew Emil Date: Tue, 27 Mar 2018 18:13:19 -0400 Subject: [PATCH 08/10] Addressed code review comments --- test/health_checker_unittest.cc | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/test/health_checker_unittest.cc b/test/health_checker_unittest.cc index d68f6e02..b6d5a132 100644 --- a/test/health_checker_unittest.cc +++ b/test/health_checker_unittest.cc @@ -24,19 +24,19 @@ class HealthCheckerUnittest : public ::testing::Test { }; -static const std::string IsolationPath(const std::string& test_name) { - return "HealthCheckFile: './" + test_name + "/unhealthy'"; +static std::istringstream IsolationPathConfig(const std::string& test_name) { + return std::istringstream("HealthCheckFile: './" + test_name + "/unhealthy'"); } TEST_F(HealthCheckerUnittest, DefaultHealthy) { - MetadataAgentConfiguration config(std::stringstream(IsolationPath(test_info_->name()))); + MetadataAgentConfiguration config(IsolationPathConfig(test_info_->name())); HealthChecker health_checker(config); EXPECT_TRUE(IsHealthy(health_checker)); Cleanup(&health_checker); } TEST_F(HealthCheckerUnittest, SimpleFailure) { - MetadataAgentConfiguration config(std::stringstream(IsolationPath(test_info_->name()))); + MetadataAgentConfiguration config(IsolationPathConfig(test_info_->name())); HealthChecker health_checker(config); SetUnhealthy(&health_checker, "kubernetes_pod_thread"); EXPECT_FALSE(IsHealthy(health_checker)); @@ -44,7 +44,7 @@ TEST_F(HealthCheckerUnittest, SimpleFailure) { } TEST_F(HealthCheckerUnittest, MultiFailure) { - MetadataAgentConfiguration config(std::stringstream(IsolationPath(test_info_->name()))); + MetadataAgentConfiguration config(IsolationPathConfig(test_info_->name())); HealthChecker health_checker(config); EXPECT_TRUE(IsHealthy(health_checker)); SetUnhealthy(&health_checker, "kubernetes_pod_thread"); @@ -55,7 +55,7 @@ TEST_F(HealthCheckerUnittest, MultiFailure) { } TEST_F(HealthCheckerUnittest, FailurePersists) { - MetadataAgentConfiguration config(std::stringstream(IsolationPath(test_info_->name()))); + MetadataAgentConfiguration config(IsolationPathConfig(test_info_->name())); HealthChecker health_checker(config); EXPECT_TRUE(IsHealthy(health_checker)); SetUnhealthy(&health_checker, "kubernetes_pod_thread"); From 344fe46e5b0e9a66fd582d5696d0c87a3805bbf2 Mon Sep 17 00:00:00 2001 From: Andrew Emil Date: Tue, 27 Mar 2018 18:26:44 -0400 Subject: [PATCH 09/10] Added namespace for static free standing function --- test/health_checker_unittest.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/health_checker_unittest.cc b/test/health_checker_unittest.cc index b6d5a132..584fc3d5 100644 --- a/test/health_checker_unittest.cc +++ b/test/health_checker_unittest.cc @@ -24,9 +24,11 @@ class HealthCheckerUnittest : public ::testing::Test { }; +namespace { static std::istringstream IsolationPathConfig(const std::string& test_name) { return std::istringstream("HealthCheckFile: './" + test_name + "/unhealthy'"); } +} // namespace TEST_F(HealthCheckerUnittest, DefaultHealthy) { MetadataAgentConfiguration config(IsolationPathConfig(test_info_->name())); From 84ceff7955dc181d8057e4abdfbd92eb3ff39303 Mon Sep 17 00:00:00 2001 From: Andrew Emil Date: Tue, 27 Mar 2018 18:51:10 -0400 Subject: [PATCH 10/10] Remove static from function in anonymous namespace --- test/health_checker_unittest.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/health_checker_unittest.cc b/test/health_checker_unittest.cc index 584fc3d5..34827e89 100644 --- a/test/health_checker_unittest.cc +++ b/test/health_checker_unittest.cc @@ -25,7 +25,7 @@ class HealthCheckerUnittest : public ::testing::Test { }; namespace { -static std::istringstream IsolationPathConfig(const std::string& test_name) { +std::istringstream IsolationPathConfig(const std::string& test_name) { return std::istringstream("HealthCheckFile: './" + test_name + "/unhealthy'"); } } // namespace