Added basic resource unittests #86

ACEmilG · 2018-03-20T23:50:26Z

No description provided.

igorpeshansky · 2018-03-21T02:23:19Z

test/resource_unittest.cc

+
+TEST(ResourceTest, Type) {
+  std::map<std::string, std::string> m;
+  google::MonitoredResource mr("type", m);


[Optional] Let's go with something other than "type", to make sure the field name does not accidentally leak into the value. I would propose something like "some_resource".

Good idea, changed.

igorpeshansky · 2018-03-21T02:24:44Z

test/resource_unittest.cc

+}
+
+TEST(ResourceTest, Labels) {
+  std::map<std::string, std::string> m;


std::map<std::string, std::string> m{{"foo", "bar"}, {"bar", "baz"}}; google::MonitoredResource mr("", m);

or even

google::MonitoredResource mr("", {{"foo", "bar"}, {"bar", "baz"}});

Great, changed everywhere.

igorpeshansky · 2018-03-21T02:25:16Z

test/resource_unittest.cc

+TEST(ResourceTest, BasicEquality) {
+  std::map<std::string, std::string> m1;
+  std::map<std::string, std::string> m2;
+  google::MonitoredResource mr1("", m1);


google::MonitoredResource mr1("", {}); google::MonitoredResource mr2("", {});

Changed everywhere.

igorpeshansky · 2018-03-21T02:25:45Z

test/resource_unittest.cc

+  google::MonitoredResource mr1("", m1);
+  google::MonitoredResource mr2("", m2);
+
+  EXPECT_TRUE(mr1 == mr2);


EXPECT_EQ?

igorpeshansky · 2018-03-21T02:26:04Z

test/resource_unittest.cc

+  google::MonitoredResource mr1("2", m1);
+  google::MonitoredResource mr2("1", m2);
+
+  EXPECT_TRUE(mr1 < mr2);


EXPECT_LT?

igorpeshansky · 2018-03-21T02:26:15Z

test/resource_unittest.cc

+  google::MonitoredResource mr1("", m1);
+  google::MonitoredResource mr2("", m2);
+
+  EXPECT_FALSE(mr1 == mr2);


EXPECT_NE?

Changed. I had to add the != operator to resource, but luckily the NE test will cover that as well.

igorpeshansky

One more.

igorpeshansky · 2018-03-21T15:36:07Z

test/resource_unittest.cc

+
+TEST(ResourceTest, Type) {
+  std::map<std::string, std::string> m;
+  google::MonitoredResource mr("some_resource", m);


google::MonitoredResource mr("some_resource", {}).

Fixed, thanks

bmoyles0117 · 2018-03-21T15:40:35Z

test/resource_unittest.cc

+  google::MonitoredResource mr1("2", {});
+  google::MonitoredResource mr2("1", {});
+
+  EXPECT_LT(mr1, mr2);


This one confuses me, mr1's key is 2, how is 2 less than 1?

Simple -- it's a bug; the implementation is inverted. :-)
Yay for unit tests?

Not a bug: "2" < "1" and "b" < "a". This is confusing, so I added simple examples above the expects to make it more clear.

Let's be careful here. The specific implementation of operator< for resources does have a bug, because x < y becomes x.operator<(y), which, in the current implementation would evaluate y.type_ < x.type_ || (y.type_ == x.type_ && y.labels_ < x.labels_), which is clearly wrong.
The test @ACEmilG ran to get "2" < "1" is flawed. If you compare std::strings, it would behave as expected: std::string("1") < std::string("2"). When you compare char* values, you're comparing pointers, so the order is essentially random, so that doesn't count.

I see, thanks for clarification!

Sure. It's funny that ultimately, the behavior of the operator didn't matter as long as it's stable, because all it was needed for was using MonitoredResource as a key in std::map. But it's good to get it right.

bmoyles0117 · 2018-03-21T15:41:10Z

test/resource_unittest.cc

+  google::MonitoredResource mr2("", {{"a", "a"}});
+
+  EXPECT_NE(mr1, mr2);
+  EXPECT_LT(mr1, mr2);


Same confusion here.

igorpeshansky · 2018-03-21T17:56:07Z

test/resource_unittest.cc

+  google::MonitoredResource mr1("2", {});
+  google::MonitoredResource mr2("1", {});
+
+  EXPECT_LT(mr1, mr2);


Let's be careful here. The specific implementation of operator< for resources does have a bug, because x < y becomes x.operator<(y), which, in the current implementation would evaluate y.type_ < x.type_ || (y.type_ == x.type_ && y.labels_ < x.labels_), which is clearly wrong.
The test @ACEmilG ran to get "2" < "1" is flawed. If you compare std::strings, it would behave as expected: std::string("1") < std::string("2"). When you compare char* values, you're comparing pointers, so the order is essentially random, so that doesn't count.

igorpeshansky · 2018-03-21T17:57:25Z

test/resource_unittest.cc

+  google::MonitoredResource mr1("2", {});
+  google::MonitoredResource mr2("1", {});
+
+  EXPECT_LT("2", "1");


Leftover debug code?

Removed. It was as an example (for my mistaken understanding of the comparison)

igorpeshansky · 2018-03-21T17:57:52Z

test/resource_unittest.cc

+  google::MonitoredResource mr1("", {{"b", "b"}});
+  google::MonitoredResource mr2("", {{"a", "a"}});
+
+  EXPECT_LT("b", "a");


Leftover debug code?

igorpeshansky

LGTM

Don't forget to "squash and merge".

igorpeshansky · 2018-03-21T18:18:39Z

test/resource_unittest.cc

+  google::MonitoredResource mr1("2", {});
+  google::MonitoredResource mr2("1", {});
+
+  EXPECT_LT(mr1, mr2);


Sure. It's funny that ultimately, the behavior of the operator didn't matter as long as it's stable, because all it was needed for was using MonitoredResource as a key in std::map. But it's good to get it right.

bmoyles0117

LGTM

Added basic resource unittests

5da41aa

igorpeshansky suggested changes Mar 21, 2018

View reviewed changes

ACEmilG added a commit that referenced this pull request Mar 21, 2018

Addressed code review comments on PR #86

7a96877

Addressed code review comments on PR #86

eeeb6e6

ACEmilG force-pushed the ACEmilG-resource-unit-tests branch from 7a96877 to eeeb6e6 Compare March 21, 2018 13:41

igorpeshansky suggested changes Mar 21, 2018

View reviewed changes

bmoyles0117 reviewed Mar 21, 2018

View reviewed changes

Addressed code review comments

a9b146c

igorpeshansky suggested changes Mar 21, 2018

View reviewed changes

Addressed my confusion about comparison operator

c959a1f

igorpeshansky approved these changes Mar 21, 2018

View reviewed changes

bmoyles0117 approved these changes Mar 22, 2018

View reviewed changes

igorpeshansky merged commit 83602d4 into master Mar 22, 2018

igorpeshansky deleted the ACEmilG-resource-unit-tests branch March 22, 2018 23:14

Added basic resource unittests #86

Added basic resource unittests #86

Uh oh!

Conversation

ACEmilG commented Mar 20, 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!

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!

Choose a reason for hiding this comment

Uh oh!

igorpeshansky Mar 21, 2018 • edited 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!

igorpeshansky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmoyles0117 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

igorpeshansky Mar 21, 2018 •

edited

Loading