Unittests for inherited callbacks and configuration validator in updater.h by dhrupadb · Pull Request #111 · Stackdriver/metadata-agent

dhrupadb · 2018-03-30T21:39:18Z

No description provided.

…ter.h

igorpeshansky · 2018-03-31T03:55:12Z

test/updater_unittest.cc

+    return updater.ValidateConfiguration();
+  }
+
+  void UpdateMetadataCallback(MetadataUpdater::ResourceMetadata& result) {


MetadataUpdater::ResourceMetadata&&.

igorpeshansky · 2018-03-31T03:55:44Z

test/updater_unittest.cc

+    updater.UpdateMetadataCallback(std::move(result));
+  }
+
+  void UpdateResourceCallback(MetadataUpdater::ResourceMetadata& result) {


const MetadataUpdater::ResourceMetadata&.

igorpeshansky · 2018-03-31T03:56:18Z

test/updater_unittest.cc

+  }
+
+  void UpdateResourceCallback(MetadataUpdater::ResourceMetadata& result) {
+    updater.UpdateResourceCallback(std::move(result));


No need to std::move — UpdateResourceCallback takes a const reference.

igorpeshansky · 2018-03-31T03:57:40Z

test/updater_unittest.cc

+      MetadataStore::Metadata::IGNORED()
+  );
+  UpdateResourceCallback(metadata);
+  EXPECT_EQ(MonitoredResource("test_resource", {}), store.LookupResource(""));


I know it fits on one line, but for consistency, let's put the store.LookupResource() call on the next line.

dhrupadb

PTAL

dhrupadb · 2018-03-31T16:30:25Z

test/updater_unittest.cc

+    return updater.ValidateConfiguration();
+  }
+
+  void UpdateMetadataCallback(MetadataUpdater::ResourceMetadata& result) {


dhrupadb · 2018-03-31T16:31:17Z

test/updater_unittest.cc

+    updater.UpdateMetadataCallback(std::move(result));
+  }
+
+  void UpdateResourceCallback(MetadataUpdater::ResourceMetadata& result) {


dhrupadb · 2018-03-31T16:32:18Z

test/updater_unittest.cc

+  }
+
+  void UpdateResourceCallback(MetadataUpdater::ResourceMetadata& result) {
+    updater.UpdateResourceCallback(std::move(result));


dhrupadb · 2018-03-31T16:33:49Z

test/updater_unittest.cc

+      MetadataStore::Metadata::IGNORED()
+  );
+  UpdateResourceCallback(metadata);
+  EXPECT_EQ(MonitoredResource("test_resource", {}), store.LookupResource(""));


igorpeshansky · 2018-03-31T16:56:49Z

src/store.h

 private:
  friend class MetadataReporter;
  friend class MetadataStoreTest;
+  friend class UpdaterTest;


Let's just make GetMetadataMap public instead.

igorpeshansky · 2018-03-31T17:03:24Z

test/updater_unittest.cc

+namespace {
+
+TEST_F(UpdaterTest, ValidateConfiguration) {
+  EXPECT_TRUE(ValidateConfiguration());


Should we also check that PollingMetadataUpdater::ValidateConfiguration fails when the period is negative?
You would have to pull out the variables from the fixture and make the accessor function take an updater object instead, i.e.:

static bool ValidateConfiguration(const MetadataUpdater& updater) { return updater.ValidateConfiguration(); } static void UpdateMetadataCallback( MetadataUpdater* updater, MetadataUpdater::ResourceMetadata&& result) { updater->UpdateMetadataCallback(std::move(result)); } static void UpdateResourceCallback( MetadataUpdater* updater, const MetadataUpdater::ResourceMetadata& result) { updater->UpdateResourceCallback(result); }

dhrupadb

PTAL

dhrupadb · 2018-03-31T17:24:36Z

src/store.h

 private:
  friend class MetadataReporter;
  friend class MetadataStoreTest;
+  friend class UpdaterTest;


dhrupadb · 2018-03-31T17:50:51Z

test/updater_unittest.cc

+namespace {
+
+TEST_F(UpdaterTest, ValidateConfiguration) {
+  EXPECT_TRUE(ValidateConfiguration());


igorpeshansky · 2018-03-31T18:12:01Z

src/store.h


  MetadataStore(const Configuration& config);

+  // Returns a copy of the map containing the mapping from monitored


Just Returns a copy of the mapping from ... is good enough.

igorpeshansky · 2018-03-31T18:15:33Z

test/updater_unittest.cc

+
+namespace {
+
+TEST_F(UpdaterTest, ValidateConfiguration) {


Let's give this a better name, e.g., OneMinutePollingIntervalIsValid?
Can you also add a ZeroPollingIntervalIsValid test?

igorpeshansky · 2018-03-31T18:15:54Z

test/updater_unittest.cc

+  EXPECT_TRUE(ValidateConfiguration(&updater));
+}
+
+TEST_F(UpdaterTest, UpdaterWithInvalidPollingIntervalTest) {


NegativePollingIntervalIsInvalid?

igorpeshansky · 2018-03-31T18:16:13Z

test/updater_unittest.cc

+  EXPECT_FALSE(ValidateConfiguration(&updater));
+}
+
+TEST_F(UpdaterTest, UpdateMetadataCallbackTest) {


The Test suffix in all of these test names is superfluous. Let's get rid of it.

dhrupadb

PTAL

dhrupadb · 2018-03-31T18:18:16Z

src/store.h


  MetadataStore(const Configuration& config);

+  // Returns a copy of the map containing the mapping from monitored


dhrupadb · 2018-03-31T18:19:23Z

test/updater_unittest.cc

+
+namespace {
+
+TEST_F(UpdaterTest, ValidateConfiguration) {


dhrupadb · 2018-03-31T18:19:27Z

test/updater_unittest.cc

+  EXPECT_TRUE(ValidateConfiguration(&updater));
+}
+
+TEST_F(UpdaterTest, UpdaterWithInvalidPollingIntervalTest) {


dhrupadb · 2018-03-31T18:20:01Z

test/updater_unittest.cc

+  EXPECT_FALSE(ValidateConfiguration(&updater));
+}
+
+TEST_F(UpdaterTest, UpdateMetadataCallbackTest) {


igorpeshansky

A minor comment nit.

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

src/store.h


  MetadataStore(const Configuration& config);

+  // Returns a copy of the mapping from monitored resources to metadata


from a monitored resource to — otherwise one is plural and one is singular.
Also the metadata.
So:

// Returns a copy of the mapping from a monitored resource to the // metadata associated with that resource.

dhrupadb

PTAL

dhrupadb · 2018-03-31T20:05:26Z

src/store.h


  MetadataStore(const Configuration& config);

+  // Returns a copy of the mapping from monitored resources to metadata


igorpeshansky

LGTM

bmoyles0117

LGTM

Unittests for inherited callbacks and configuration validator in upda…

b5daae9

…ter.h

dhrupadb requested review from ACEmilG, bmoyles0117 and igorpeshansky March 30, 2018 21:39

igorpeshansky suggested changes Mar 31, 2018

View reviewed changes

Address feedback

7663452

dhrupadb commented Mar 31, 2018

View reviewed changes

igorpeshansky suggested changes Mar 31, 2018

View reviewed changes

Address feedback. Make GetMetadataMap in MetadataStore public

d139dd8

dhrupadb commented Mar 31, 2018

View reviewed changes

igorpeshansky suggested changes Mar 31, 2018

View reviewed changes

Address feedback

4842e7f

dhrupadb commented Mar 31, 2018

View reviewed changes

igorpeshansky suggested changes Mar 31, 2018

View reviewed changes

Address nit

53d36c5

dhrupadb commented Mar 31, 2018

View reviewed changes

igorpeshansky approved these changes Mar 31, 2018

View reviewed changes

bmoyles0117 approved these changes Mar 31, 2018

View reviewed changes

igorpeshansky removed the request for review from ACEmilG March 31, 2018 21:21

igorpeshansky merged commit f8f54a9 into master Mar 31, 2018

igorpeshansky deleted the dhrupadb-metadata-updater-unittests branch March 31, 2018 21:22

dhrupadb mentioned this pull request Apr 3, 2018

Api server test for dispatch operator #123

Closed


		MetadataStore(const Configuration& config);

		// Returns a copy of the map containing the mapping from monitored


		MetadataStore(const Configuration& config);

		// Returns a copy of the mapping from monitored resources to metadata


		namespace {

		TEST_F(UpdaterTest, ValidateConfiguration) {


		namespace {

		TEST_F(UpdaterTest, ValidateConfiguration) {

Conversation

dhrupadb commented Mar 30, 2018

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!

dhrupadb left a comment

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!

dhrupadb left a comment

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!

dhrupadb left a comment

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!

igorpeshansky left a comment

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!

dhrupadb left a comment