Add HistogramTester::GetAllSamples.

Add HistogramTester::GetAllSamples.

Returns the same value as GetHistogramSamplesSinceCreation, but as a
vector<pair<Sample, Count>> for easier testing.

BUG=

Committed: https://crrev.com/5c8510d1c1078e82357005cf56f0af8acde0c168
Cr-Commit-Position: refs/heads/master@{#336842}

[twifkak, 2015-06-18 00:03:17]

twifkak@chromium.org changed reviewers:
+ phajdan.jr@chromium.org

[twifkak, 2015-06-18 00:03:18]

An example use case would be https://codereview.chromium.org/1179953005/.

[twifkak, 2015-06-18 00:03:57]

On 2015/06/18 00:03:18, twifkak wrote:
> An example use case would be https://codereview.chromium.org/1179953005/.

That's https://codereview.chromium.org/1179953005/patch/80001/90003 in
particular.

[twifkak, 2015-06-22 19:28:23]

twifkak@chromium.org changed reviewers:
+ mlerman@chromium.org

[twifkak, 2015-06-22 19:30:52]

+mlerman, who authored the class.

[Mike Lerman, 2015-06-22 20:40:04]

It was fairly intentional that the raw state of the histogram is not exposed
directly.

I looked at https://codereview.chromium.org/1179953005/patch/80001/90003. Can't
you just use
 histogram.ExpectBucketCount("History.TopHostsVisitsByRank", 1, 1);
 histogram.ExpectBucketCount("History.TopHostsVisitsByRank", 51, 1);
 histogram.ExpectTotalCount("History.TopHostsVisitsByRank", 2);

Directly after the calls to AddPageVisit?

[twifkak, 2015-06-22 20:48:34]

On 2015/06/22 20:40:04, Mike Lerman wrote:
> It was fairly intentional that the raw state of the histogram is not exposed
> directly.

This is just a wrapper around GetHistogramSamplesSinceCreation(), which is
another public method.

> I looked at https://codereview.chromium.org/1179953005/patch/80001/90003.
Can't
> you just use
>  histogram.ExpectBucketCount("History.TopHostsVisitsByRank", 1, 1);
>  histogram.ExpectBucketCount("History.TopHostsVisitsByRank", 51, 1);
>  histogram.ExpectTotalCount("History.TopHostsVisitsByRank", 2);
> 
> Directly after the calls to AddPageVisit?

I can, but it would be strictly worse. Doing a full container comparison will
show you all of the expect/actual buckets, with +/- indicators for differences.
In the above block of code, if for example the actual histogram contains
additional buckets, I would find out because ExpectTotalCount is wrong, and then
I'd have to modify my test code to use GetHistogramSamplesSinceCreation so I can
find out which additional buckets were recorded, re-run it, and hope that it
wasn't a flaky failure.

[Mike Lerman, 2015-06-23 12:37:01]

On 2015/06/22 20:48:34, twifkak wrote:
> On 2015/06/22 20:40:04, Mike Lerman wrote:
> > It was fairly intentional that the raw state of the histogram is not exposed
> > directly.
> 
> This is just a wrapper around GetHistogramSamplesSinceCreation(), which is
> another public method.
> 
> > I looked at https://codereview.chromium.org/1179953005/patch/80001/90003.
> Can't
> > you just use
> >  histogram.ExpectBucketCount("History.TopHostsVisitsByRank", 1, 1);
> >  histogram.ExpectBucketCount("History.TopHostsVisitsByRank", 51, 1);
> >  histogram.ExpectTotalCount("History.TopHostsVisitsByRank", 2);
> > 
> > Directly after the calls to AddPageVisit?
> 
> I can, but it would be strictly worse. Doing a full container comparison will
> show you all of the expect/actual buckets, with +/- indicators for
differences.
> In the above block of code, if for example the actual histogram contains
> additional buckets, I would find out because ExpectTotalCount is wrong, and
then
> I'd have to modify my test code to use GetHistogramSamplesSinceCreation so I
can
> find out which additional buckets were recorded, re-run it, and hope that it
> wasn't a flaky failure.

Can I perhaps direct your attention to this CL?
https://codereview.chromium.org/1190423006/

I think it will meet your needs.

[twifkak, 2015-06-23 16:23:31]

twifkak@chromium.org changed reviewers:
+ isherman@chromium.org

[twifkak, 2015-06-23 16:25:45]

On 2015/06/23 12:37:01, Mike Lerman wrote:
> On 2015/06/22 20:48:34, twifkak wrote:
> > On 2015/06/22 20:40:04, Mike Lerman wrote:
> > > It was fairly intentional that the raw state of the histogram is not
exposed
> > > directly.
> > 
> > This is just a wrapper around GetHistogramSamplesSinceCreation(), which is
> > another public method.
> > 
> > > I looked at https://codereview.chromium.org/1179953005/patch/80001/90003.
> > Can't
> > > you just use
> > >  histogram.ExpectBucketCount("History.TopHostsVisitsByRank", 1, 1);
> > >  histogram.ExpectBucketCount("History.TopHostsVisitsByRank", 51, 1);
> > >  histogram.ExpectTotalCount("History.TopHostsVisitsByRank", 2);
> > > 
> > > Directly after the calls to AddPageVisit?
> > 
> > I can, but it would be strictly worse. Doing a full container comparison
will
> > show you all of the expect/actual buckets, with +/- indicators for
> differences.
> > In the above block of code, if for example the actual histogram contains
> > additional buckets, I would find out because ExpectTotalCount is wrong, and
> then
> > I'd have to modify my test code to use GetHistogramSamplesSinceCreation so I
> can
> > find out which additional buckets were recorded, re-run it, and hope that it
> > wasn't a flaky failure.
> 
> Can I perhaps direct your attention to this CL?
> https://codereview.chromium.org/1190423006/
> 
> I think it will meet your needs.

Thanks for the pointer. That seems to be a more complex version of this CL, also
unsubmitted. It requires learning a new syntax, rather than reusing the one
provided by gMock. Why do you think it is a better fit for my needs and for this
class?

[Mike Lerman, 2015-06-23 17:45:02]

On 2015/06/23 16:25:45, twifkak wrote:
> On 2015/06/23 12:37:01, Mike Lerman wrote:
> > On 2015/06/22 20:48:34, twifkak wrote:
> > > On 2015/06/22 20:40:04, Mike Lerman wrote:
> > > > It was fairly intentional that the raw state of the histogram is not
> exposed
> > > > directly.
> > > 
> > > This is just a wrapper around GetHistogramSamplesSinceCreation(), which is
> > > another public method.
> > > 
> > > > I looked at
https://codereview.chromium.org/1179953005/patch/80001/90003.
> > > Can't
> > > > you just use
> > > >  histogram.ExpectBucketCount("History.TopHostsVisitsByRank", 1, 1);
> > > >  histogram.ExpectBucketCount("History.TopHostsVisitsByRank", 51, 1);
> > > >  histogram.ExpectTotalCount("History.TopHostsVisitsByRank", 2);
> > > > 
> > > > Directly after the calls to AddPageVisit?
> > > 
> > > I can, but it would be strictly worse. Doing a full container comparison
> will
> > > show you all of the expect/actual buckets, with +/- indicators for
> > differences.
> > > In the above block of code, if for example the actual histogram contains
> > > additional buckets, I would find out because ExpectTotalCount is wrong,
and
> > then
> > > I'd have to modify my test code to use GetHistogramSamplesSinceCreation so
I
> > can
> > > find out which additional buckets were recorded, re-run it, and hope that
it
> > > wasn't a flaky failure.
> > 
> > Can I perhaps direct your attention to this CL?
> > https://codereview.chromium.org/1190423006/
> > 
> > I think it will meet your needs.
> 
> Thanks for the pointer. That seems to be a more complex version of this CL,
also
> unsubmitted. It requires learning a new syntax, rather than reusing the one
> provided by gMock. Why do you think it is a better fit for my needs and for
this
> class?

I think it provides greater flexibility than this CL does, 1) by capturing all
histograms that match on a prefix, and 2) that using STL structures will require
very verbose structures as part of the test.

I don't think both CLs need to land, and I think that while the other CL will
meet your needs, that your CL will not meet their needs. Feel free to correct
me.

[twifkak, 2015-06-23 19:41:57]

twifkak@chromium.org changed reviewers:
+ nick@chromium.org

[twifkak, 2015-06-23 19:52:05]

On 2015/06/23 17:45:02, Mike Lerman wrote:
> On 2015/06/23 16:25:45, twifkak wrote:
> > On 2015/06/23 12:37:01, Mike Lerman wrote:
> > > On 2015/06/22 20:48:34, twifkak wrote:
> > > > On 2015/06/22 20:40:04, Mike Lerman wrote:
> > > > > It was fairly intentional that the raw state of the histogram is not
> > exposed
> > > > > directly.
> > > > 
> > > > This is just a wrapper around GetHistogramSamplesSinceCreation(), which
is
> > > > another public method.
> > > > 
> > > > > I looked at
> https://codereview.chromium.org/1179953005/patch/80001/90003.
> > > > Can't
> > > > > you just use
> > > > >  histogram.ExpectBucketCount("History.TopHostsVisitsByRank", 1, 1);
> > > > >  histogram.ExpectBucketCount("History.TopHostsVisitsByRank", 51, 1);
> > > > >  histogram.ExpectTotalCount("History.TopHostsVisitsByRank", 2);
> > > > > 
> > > > > Directly after the calls to AddPageVisit?
> > > > 
> > > > I can, but it would be strictly worse. Doing a full container comparison
> > will
> > > > show you all of the expect/actual buckets, with +/- indicators for
> > > differences.
> > > > In the above block of code, if for example the actual histogram contains
> > > > additional buckets, I would find out because ExpectTotalCount is wrong,
> and
> > > then
> > > > I'd have to modify my test code to use GetHistogramSamplesSinceCreation
so
> I
> > > can
> > > > find out which additional buckets were recorded, re-run it, and hope
that
> it
> > > > wasn't a flaky failure.
> > > 
> > > Can I perhaps direct your attention to this CL?
> > > https://codereview.chromium.org/1190423006/
> > > 
> > > I think it will meet your needs.
> > 
> > Thanks for the pointer. That seems to be a more complex version of this CL,
> also
> > unsubmitted. It requires learning a new syntax, rather than reusing the one
> > provided by gMock. Why do you think it is a better fit for my needs and for
> this
> > class?
> 
> I think it provides greater flexibility than this CL does, 1) by capturing all
> histograms that match on a prefix,

This feature is not mutually exclusive with what I've developed.

> and 2) that using STL structures will require
> very verbose structures as part of the test.

I disagree that the use of STL structure is more verbose. Here's the test code
from the other CL:

std::vector<std::string> xsd_metrics;
xsd_metrics.push_back("SiteIsolation.XSD.DataLength=1");
xsd_metrics.push_back(
    base::StringPrintf("SiteIsolation.XSD.MimeType[%d]=1", mime_type));
// Determine the appropriate conditionally-incremented histograms.
std::string base = "SiteIsolation.XSD." + bucket;
if (should_be_blocked) {
  xsd_metrics.push_back(base + ".Blocked=1");
  xsd_metrics.push_back(base + ".Blocked.RenderableStatusCode[13]=1");
  static_assert(13 == RESOURCE_TYPE_XHR, "Histogram enums mustn't change.");
} else {
  xsd_metrics.push_back(base + ".NotBlocked=1");
  if (MatchPattern(resource_name, "*js.*")) {
    xsd_metrics.push_back(base + ".NotBlocked.MaybeJS=1");
  }
}
std::sort(xsd_metrics.begin(), xsd_metrics.end());
EXPECT_EQ(JoinString(xsd_metrics, "\n"),
          histograms.GetTotalCountsForQuery("SiteIsolation.XSD."))
    << "For resource_name=" << resource_name
    << ", should_be_blocked=" << should_be_blocked;

This is 21 lines, or 64 words. And here is the version using this CL:

EXPECT_EQ(1, histograms.GetTotalCount("SiteIsolation.XSD.DataLength"));
EXPECT_THAT(histograms.GetAllBuckets("SiteIsolation.XSD.MimeType"),
    ElementsAre(make_pair(mime_type, 1)));
if (should_be_blocked) {
  EXPECT_EQ(1, histograms.GetTotalCount("SiteIsolation.XSD.Blocked"));
  EXPECT_THAT(histograms.GetAllBuckets(
      "SiteIsolation.XSD.Blocked.RenderableStatusCode"),
      ElementsAre(make_pair(13, 1)));
} else {
  EXPECT_EQ(1, histograms.GetTotalCount("SiteIsolation.XSD.NotBlocked"));
  if (MatchPattern(resource_name, "*js.*")) {
    EXPECT_EQ(1,
        histograms.GetTotalCount("SiteIsolation.XSD.NotBlocked.MaybeJS"));
  }
}

It is 15 lines, or 27 words. And if uniform initializers land in chromium, it
will be even more succinct, because you will be able to use EXPECT_EQ and brace
syntax.

That said, it fails to test that all other histograms are untouched. If that
were your only complaint, I would be happy to address that.

> I don't think both CLs need to land, and I think that while the other CL will
> meet your needs, that your CL will not meet their needs. Feel free to correct
> me.

If the listed CL were submitted and unittested I would agree that it meets my
needs. However, it is not. Furthermore, I'm not convinced, given your comments
on the that CL, and given the following comment on this CL, that it will be
submitted:

"It was fairly intentional that the raw state of the histogram is not exposed
directly."

By all accounts, the other CL exposes more, since it allows access to an
unbounded list of histograms in one call, while this CL allows access to only
one histogram per call.

I agree that this CL doesn't meet Nick's needs. Of course, it could be modified
to do so, while still meeting my criteria of being simple and unittested.
Likewise, Nick's CL could be modified to be unittested and submitted, and thus
meet most of my criteria. (I'd prefer an EDSL to a brand new syntax, but
wouldn't be heart-broken about it.) I don't really care whose name is on the
author line.

Note that the author of the other CL might actually like the API I present in
this CL. He writes:

> I think passing in a vector<string> or a map<string, int> might be okay,
> though my thinking was that a flattened representation of that data structure
> would be easiest on the caller of this class, since without the (currently
> banned) C++11 uniform initialization syntax, it's kind of a bummer to have to
> manually construct one of those if you have just one metric to test.

I think he is unaware of the ElementsAre() function from gMock, which makes
returning an STL container a succinct option in Chromium. That's just a guess; I
don't wish to speak for him.

[Ilya Sherman, 2015-06-23 21:04:54]

I'm okay with this CL too.  I do think that if we land this CL, it would be nice
to remove the ExpectUniqueSample and ExpectBucketCount methods, in favor of the
uniform EXPECT_THAT syntax throughout our test files.

[twifkak, 2015-06-23 21:07:55]

twifkak@chromium.org changed reviewers:
- nick@chromium.org

[ncarter (slow), 2015-06-24 17:28:43]

nick@chromium.org changed reviewers:
+ nick@chromium.org

[ncarter (slow), 2015-06-24 17:28:44]

https://codereview.chromium.org/1177263004/diff/1/base/test/histogram_tester.h
File base/test/histogram_tester.h (right):

https://codereview.chromium.org/1177263004/diff/1/base/test/histogram_tester....
base/test/histogram_tester.h:54: // bucket and the Count is the count recorded
since creation.
This comment should have code showing the example usage, including the
EXPECT_THAT() and the testing::ElementsAre. Even when I'm looking at the gmock
docs, it's pretty mystifying to figure out how to get these to work from first
principles.

https://codereview.chromium.org/1177263004/diff/1/base/test/histogram_tester_...
File base/test/histogram_tester_unittest.cc (right):

https://codereview.chromium.org/1177263004/diff/1/base/test/histogram_tester_...
base/test/histogram_tester_unittest.cc:93: std::make_pair(5, 1)));
I patched this in and played around with it. I think using EXPECT_THAT is just
as good as the flattened text representation I came up with in the other CL.
Using the text representation arguably has less of a learning curve and gives
very slightly more readable errors, but on the other hand, it introduces a
convention of its own that has to be explained.

If the other reviewers are, like me, okay with unifying behind the EXPECT_THAT
approach, my CL could become this:

  map<string, int> GetTotalCountsForPrefix(string name_prefix);

with example usage:

  EXPECT_THAT(tester.GetTotalCountsForPrefix("MyMetric."),
              ElementsAre(std::make_pair("MyMetric.A", 1), 
                          std::make_pair("MyMetric.A", 2)));

 or for more complex usage:

  std::map<std::string, int> expected_counts;
  expected_counts["MyMetric.A"] = 1;
  expected_counts["MyMetric.B"] = 1;
  EXPECT_THAT(tester.GetTotalCountsForPrefix("MyMetric."),
              ContainerEq(expected_counts));

And if, after using the *ForPrefix to determine which metrics had samples, you
wanted to inspect buckets, you'd use GetAllSamples for that, one metric at a
time.

I think these approaches would then fit together nicely (and let us hoist the
EXPECTS back up into the tests, which is a common purpose of both proposals). I
also think we ought to strive to eliminate the Expect* methods altogether so
that there aren't multiple redundant ways to do this.

[ncarter (slow), 2015-06-24 17:38:14]

On 2015/06/24 17:28:44, ncarter wrote:
> https://codereview.chromium.org/1177263004/diff/1/base/test/histogram_tester.h
> File base/test/histogram_tester.h (right):
> 
>
https://codereview.chromium.org/1177263004/diff/1/base/test/histogram_tester....
> base/test/histogram_tester.h:54: // bucket and the Count is the count recorded
> since creation.
> This comment should have code showing the example usage, including the
> EXPECT_THAT() and the testing::ElementsAre. Even when I'm looking at the gmock
> docs, it's pretty mystifying to figure out how to get these to work from first
> principles.
> 
>
https://codereview.chromium.org/1177263004/diff/1/base/test/histogram_tester_...
> File base/test/histogram_tester_unittest.cc (right):
> 
>
https://codereview.chromium.org/1177263004/diff/1/base/test/histogram_tester_...
> base/test/histogram_tester_unittest.cc:93: std::make_pair(5, 1)));
> I patched this in and played around with it. I think using EXPECT_THAT is just
> as good as the flattened text representation I came up with in the other CL.
> Using the text representation arguably has less of a learning curve and gives
> very slightly more readable errors, but on the other hand, it introduces a
> convention of its own that has to be explained.
> 
> If the other reviewers are, like me, okay with unifying behind the EXPECT_THAT
> approach, my CL could become this:
> 
>   map<string, int> GetTotalCountsForPrefix(string name_prefix);
> 
> with example usage:
> 
>   EXPECT_THAT(tester.GetTotalCountsForPrefix("MyMetric."),
>               ElementsAre(std::make_pair("MyMetric.A", 1), 
>                           std::make_pair("MyMetric.A", 2)));
> 
>  or for more complex usage:
> 
>   std::map<std::string, int> expected_counts;
>   expected_counts["MyMetric.A"] = 1;
>   expected_counts["MyMetric.B"] = 1;
>   EXPECT_THAT(tester.GetTotalCountsForPrefix("MyMetric."),
>               ContainerEq(expected_counts));
> 
> And if, after using the *ForPrefix to determine which metrics had samples, you
> wanted to inspect buckets, you'd use GetAllSamples for that, one metric at a
> time.
> 
> I think these approaches would then fit together nicely (and let us hoist the
> EXPECTS back up into the tests, which is a common purpose of both proposals).
I
> also think we ought to strive to eliminate the Expect* methods altogether so
> that there aren't multiple redundant ways to do this.

For the curious -- I compiled a comparison of the output from various
approaches; it's here: https://gist.github.com/anonymous/95f296c5caed1f474d7c

[Ilya Sherman, 2015-06-24 20:28:08]

On 2015/06/24 17:28:44, ncarter wrote:
> https://codereview.chromium.org/1177263004/diff/1/base/test/histogram_tester.h
> File base/test/histogram_tester.h (right):
> 
>
https://codereview.chromium.org/1177263004/diff/1/base/test/histogram_tester....
> base/test/histogram_tester.h:54: // bucket and the Count is the count recorded
> since creation.
> This comment should have code showing the example usage, including the
> EXPECT_THAT() and the testing::ElementsAre. Even when I'm looking at the gmock
> docs, it's pretty mystifying to figure out how to get these to work from first
> principles.
> 
>
https://codereview.chromium.org/1177263004/diff/1/base/test/histogram_tester_...
> File base/test/histogram_tester_unittest.cc (right):
> 
>
https://codereview.chromium.org/1177263004/diff/1/base/test/histogram_tester_...
> base/test/histogram_tester_unittest.cc:93: std::make_pair(5, 1)));
> I patched this in and played around with it. I think using EXPECT_THAT is just
> as good as the flattened text representation I came up with in the other CL.
> Using the text representation arguably has less of a learning curve and gives
> very slightly more readable errors, but on the other hand, it introduces a
> convention of its own that has to be explained.
> 
> If the other reviewers are, like me, okay with unifying behind the EXPECT_THAT
> approach, my CL could become this:
> 
>   map<string, int> GetTotalCountsForPrefix(string name_prefix);
> 
> with example usage:
> 
>   EXPECT_THAT(tester.GetTotalCountsForPrefix("MyMetric."),
>               ElementsAre(std::make_pair("MyMetric.A", 1), 
>                           std::make_pair("MyMetric.A", 2)));
> 
>  or for more complex usage:
> 
>   std::map<std::string, int> expected_counts;
>   expected_counts["MyMetric.A"] = 1;
>   expected_counts["MyMetric.B"] = 1;
>   EXPECT_THAT(tester.GetTotalCountsForPrefix("MyMetric."),
>               ContainerEq(expected_counts));
> 
> And if, after using the *ForPrefix to determine which metrics had samples, you
> wanted to inspect buckets, you'd use GetAllSamples for that, one metric at a
> time.
> 
> I think these approaches would then fit together nicely (and let us hoist the
> EXPECTS back up into the tests, which is a common purpose of both proposals).
I
> also think we ought to strive to eliminate the Expect* methods altogether so
> that there aren't multiple redundant ways to do this.

SGTM!

[twifkak, 2015-06-24 21:10:09]

On 2015/06/24 17:38:14, ncarter wrote:
> For the curious -- I compiled a comparison of the output from various
> approaches; it's here: https://gist.github.com/anonymous/95f296c5caed1f474d7c

Ah, that's very helpful to see -- thanks! I agree, the text-based diff is much
clearer. I would be happy with that, too.

So, quick pros for each side:

stl approach:
- reuses existing notations (familiar for some people, perhaps)
- eventually gets a free upgrade to EXPECT_EQ({..}, ...) via uniform
initialization syntax
- possibly more succinct (dunno if this is true of the ForPrefix variant)

text approach:
- clearer failure message
- single function call for both bucket-checking and zero-checking

I'm okay with either way. As I mentioned earlier, I have a slight prejudice for
EDSLs, but in this case, it's possibly an irrational one. The clearer failure
message seems to be the most important distinction for users.

Thoughts?

[Ilya Sherman, 2015-06-24 23:22:29]

On 2015/06/24 21:10:09, twifkak wrote:
> On 2015/06/24 17:38:14, ncarter wrote:
> > For the curious -- I compiled a comparison of the output from various
> > approaches; it's here:
https://gist.github.com/anonymous/95f296c5caed1f474d7c
> 
> Ah, that's very helpful to see -- thanks! I agree, the text-based diff is much
> clearer. I would be happy with that, too.
> 
> So, quick pros for each side:
> 
> stl approach:
> - reuses existing notations (familiar for some people, perhaps)
> - eventually gets a free upgrade to EXPECT_EQ({..}, ...) via uniform
> initialization syntax
> - possibly more succinct (dunno if this is true of the ForPrefix variant)
> 
> text approach:
> - clearer failure message
> - single function call for both bucket-checking and zero-checking
> 
> I'm okay with either way. As I mentioned earlier, I have a slight prejudice
for
> EDSLs, but in this case, it's possibly an irrational one. The clearer failure
> message seems to be the most important distinction for users.
> 
> Thoughts?

I don't have a strong preference either way.  I do have a question though: Could
we get clearer failure messages by using a custom struct with custom pretty
printing, rather than an std::pair?

[ncarter (slow), 2015-06-24 23:31:00]

On 2015/06/24 23:22:29, Ilya Sherman wrote:
> On 2015/06/24 21:10:09, twifkak wrote:
> > On 2015/06/24 17:38:14, ncarter wrote:
> > > For the curious -- I compiled a comparison of the output from various
> > > approaches; it's here:
> https://gist.github.com/anonymous/95f296c5caed1f474d7c
> > 
> > Ah, that's very helpful to see -- thanks! I agree, the text-based diff is
much
> > clearer. I would be happy with that, too.
> > 
> > So, quick pros for each side:
> > 
> > stl approach:
> > - reuses existing notations (familiar for some people, perhaps)
> > - eventually gets a free upgrade to EXPECT_EQ({..}, ...) via uniform
> > initialization syntax
> > - possibly more succinct (dunno if this is true of the ForPrefix variant)
> > 
> > text approach:
> > - clearer failure message
> > - single function call for both bucket-checking and zero-checking
> > 
> > I'm okay with either way. As I mentioned earlier, I have a slight prejudice
> for
> > EDSLs, but in this case, it's possibly an irrational one. The clearer
failure
> > message seems to be the most important distinction for users.
> > 
> > Thoughts?
> 
> I don't have a strong preference either way.  I do have a question though:
Could
> we get clearer failure messages by using a custom struct with custom pretty
> printing, rather than an std::pair?

Let's go with the EDSLs/gmock style + clear examples in comments. What breaks
the tie for me is the annoyance of forcing the caller to std::sort their lines.

[twifkak, 2015-06-25 20:08:20]

On 2015/06/24 23:22:29, Ilya Sherman wrote:
> On 2015/06/24 21:10:09, twifkak wrote:
> > On 2015/06/24 17:38:14, ncarter wrote:
> > > For the curious -- I compiled a comparison of the output from various
> > > approaches; it's here:
> https://gist.github.com/anonymous/95f296c5caed1f474d7c
> > 
> > Ah, that's very helpful to see -- thanks! I agree, the text-based diff is
much
> > clearer. I would be happy with that, too.
> > 
> > So, quick pros for each side:
> > 
> > stl approach:
> > - reuses existing notations (familiar for some people, perhaps)
> > - eventually gets a free upgrade to EXPECT_EQ({..}, ...) via uniform
> > initialization syntax
> > - possibly more succinct (dunno if this is true of the ForPrefix variant)
> > 
> > text approach:
> > - clearer failure message
> > - single function call for both bucket-checking and zero-checking
> > 
> > I'm okay with either way. As I mentioned earlier, I have a slight prejudice
> for
> > EDSLs, but in this case, it's possibly an irrational one. The clearer
failure
> > message seems to be the most important distinction for users.
> > 
> > Thoughts?
> 
> I don't have a strong preference either way.  I do have a question though:
Could
> we get clearer failure messages by using a custom struct with custom pretty
> printing, rather than an std::pair?

I don't think gtest supports printing a diff between expected and actual, based
on this line in the implementation of EXPECT_EQ:
https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/src/....

One option is to write a custom matcher in gmock. The resulting syntax would be
something like:

EXPECT_THAT(histogram.GetAllSamples("foo"),
            BucketsAre(Pair(1, 2), Pair(3, 4));

Because this is a variadic matcher, AFAICT, this requires a couple of classes
and a function, per the implementation of ElementsAre in
testing/gmock/include/gmock/gmock-matchers.h. It's a significant enough amount
of magic templated code that I'm not sure the benefit is worth the complexity
cost.

There is an easy-to-use macro for writing unary matchers:
https://code.google.com/p/googlemock/wiki/CookBook#Writing_New_Parameterized_...,
but we're back to where we started, being unable to specific container literals
succinctly.

[Ilya Sherman, 2015-06-25 20:18:42]

On 2015/06/25 20:08:20, twifkak wrote:
> On 2015/06/24 23:22:29, Ilya Sherman wrote:
> > On 2015/06/24 21:10:09, twifkak wrote:
> > > On 2015/06/24 17:38:14, ncarter wrote:
> > > > For the curious -- I compiled a comparison of the output from various
> > > > approaches; it's here:
> > https://gist.github.com/anonymous/95f296c5caed1f474d7c
> > > 
> > > Ah, that's very helpful to see -- thanks! I agree, the text-based diff is
> much
> > > clearer. I would be happy with that, too.
> > > 
> > > So, quick pros for each side:
> > > 
> > > stl approach:
> > > - reuses existing notations (familiar for some people, perhaps)
> > > - eventually gets a free upgrade to EXPECT_EQ({..}, ...) via uniform
> > > initialization syntax
> > > - possibly more succinct (dunno if this is true of the ForPrefix variant)
> > > 
> > > text approach:
> > > - clearer failure message
> > > - single function call for both bucket-checking and zero-checking
> > > 
> > > I'm okay with either way. As I mentioned earlier, I have a slight
prejudice
> > for
> > > EDSLs, but in this case, it's possibly an irrational one. The clearer
> failure
> > > message seems to be the most important distinction for users.
> > > 
> > > Thoughts?
> > 
> > I don't have a strong preference either way.  I do have a question though:
> Could
> > we get clearer failure messages by using a custom struct with custom pretty
> > printing, rather than an std::pair?
> 
> I don't think gtest supports printing a diff between expected and actual,
based
> on this line in the implementation of EXPECT_EQ:
>
https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/src/....
> 
> One option is to write a custom matcher in gmock. The resulting syntax would
be
> something like:
> 
> EXPECT_THAT(histogram.GetAllSamples("foo"),
>             BucketsAre(Pair(1, 2), Pair(3, 4));
> 
> Because this is a variadic matcher, AFAICT, this requires a couple of classes
> and a function, per the implementation of ElementsAre in
> testing/gmock/include/gmock/gmock-matchers.h. It's a significant enough amount
> of magic templated code that I'm not sure the benefit is worth the complexity
> cost.
> 
> There is an easy-to-use macro for writing unary matchers:
>
https://code.google.com/p/googlemock/wiki/CookBook#Writing_New_Parameterized_...,
> but we're back to where we started, being unable to specific container
literals
> succinctly.

I was thinking that we could just change the pretty printing of the elements
themselves, to something a little clearer than (2, 1).  I wasn't even thinking
of the possibility of writing a custom matcher -- I do agree that it's probably
not worth the implementation complexity.

[twifkak, 2015-06-25 20:21:38]

https://codereview.chromium.org/1177263004/diff/1/base/test/histogram_tester.h
File base/test/histogram_tester.h (right):

https://codereview.chromium.org/1177263004/diff/1/base/test/histogram_tester....
base/test/histogram_tester.h:54: // bucket and the Count is the count recorded
since creation.
On 2015/06/24 17:28:44, ncarter wrote:
> This comment should have code showing the example usage, including the
> EXPECT_THAT() and the testing::ElementsAre. Even when I'm looking at the gmock
> docs, it's pretty mystifying to figure out how to get these to work from first
> principles.
> 

Done.

https://codereview.chromium.org/1177263004/diff/1/base/test/histogram_tester_...
File base/test/histogram_tester_unittest.cc (right):

https://codereview.chromium.org/1177263004/diff/1/base/test/histogram_tester_...
base/test/histogram_tester_unittest.cc:93: std::make_pair(5, 1)));
On 2015/06/24 17:28:44, ncarter wrote:
> I patched this in and played around with it. I think using EXPECT_THAT is just
> as good as the flattened text representation I came up with in the other CL.
> Using the text representation arguably has less of a learning curve and gives
> very slightly more readable errors, but on the other hand, it introduces a
> convention of its own that has to be explained.
> 
> If the other reviewers are, like me, okay with unifying behind the EXPECT_THAT
> approach, my CL could become this:
> 
>   map<string, int> GetTotalCountsForPrefix(string name_prefix);
> 
> with example usage:
> 
>   EXPECT_THAT(tester.GetTotalCountsForPrefix("MyMetric."),
>               ElementsAre(std::make_pair("MyMetric.A", 1), 
>                           std::make_pair("MyMetric.A", 2)));
> 
>  or for more complex usage:
> 
>   std::map<std::string, int> expected_counts;
>   expected_counts["MyMetric.A"] = 1;
>   expected_counts["MyMetric.B"] = 1;
>   EXPECT_THAT(tester.GetTotalCountsForPrefix("MyMetric."),
>               ContainerEq(expected_counts));
> 
> And if, after using the *ForPrefix to determine which metrics had samples, you
> wanted to inspect buckets, you'd use GetAllSamples for that, one metric at a
> time.
> 
> I think these approaches would then fit together nicely (and let us hoist the
> EXPECTS back up into the tests, which is a common purpose of both proposals).
I
> also think we ought to strive to eliminate the Expect* methods altogether so
> that there aren't multiple redundant ways to do this.

Okay, sounds good!

Do you want me to implement GetTotalCountsForPrefix, or modify your CL per this
API?

[ncarter (slow), 2015-06-25 21:20:25]

On 2015/06/25 20:21:38, twifkak wrote:
> https://codereview.chromium.org/1177263004/diff/1/base/test/histogram_tester.h
> File base/test/histogram_tester.h (right):
> 
>
https://codereview.chromium.org/1177263004/diff/1/base/test/histogram_tester....
> base/test/histogram_tester.h:54: // bucket and the Count is the count recorded
> since creation.
> On 2015/06/24 17:28:44, ncarter wrote:
> > This comment should have code showing the example usage, including the
> > EXPECT_THAT() and the testing::ElementsAre. Even when I'm looking at the
gmock
> > docs, it's pretty mystifying to figure out how to get these to work from
first
> > principles.
> > 
> 
> Done.
> 
>
https://codereview.chromium.org/1177263004/diff/1/base/test/histogram_tester_...
> File base/test/histogram_tester_unittest.cc (right):
> 
>
https://codereview.chromium.org/1177263004/diff/1/base/test/histogram_tester_...
> base/test/histogram_tester_unittest.cc:93: std::make_pair(5, 1)));
> On 2015/06/24 17:28:44, ncarter wrote:
> > I patched this in and played around with it. I think using EXPECT_THAT is
just
> > as good as the flattened text representation I came up with in the other CL.
> > Using the text representation arguably has less of a learning curve and
gives
> > very slightly more readable errors, but on the other hand, it introduces a
> > convention of its own that has to be explained.
> > 
> > If the other reviewers are, like me, okay with unifying behind the
EXPECT_THAT
> > approach, my CL could become this:
> > 
> >   map<string, int> GetTotalCountsForPrefix(string name_prefix);
> > 
> > with example usage:
> > 
> >   EXPECT_THAT(tester.GetTotalCountsForPrefix("MyMetric."),
> >               ElementsAre(std::make_pair("MyMetric.A", 1), 
> >                           std::make_pair("MyMetric.A", 2)));
> > 
> >  or for more complex usage:
> > 
> >   std::map<std::string, int> expected_counts;
> >   expected_counts["MyMetric.A"] = 1;
> >   expected_counts["MyMetric.B"] = 1;
> >   EXPECT_THAT(tester.GetTotalCountsForPrefix("MyMetric."),
> >               ContainerEq(expected_counts));
> > 
> > And if, after using the *ForPrefix to determine which metrics had samples,
you
> > wanted to inspect buckets, you'd use GetAllSamples for that, one metric at a
> > time.
> > 
> > I think these approaches would then fit together nicely (and let us hoist
the
> > EXPECTS back up into the tests, which is a common purpose of both
proposals).
> I
> > also think we ought to strive to eliminate the Expect* methods altogether so
> > that there aren't multiple redundant ways to do this.
> 
> Okay, sounds good!
> 
> Do you want me to implement GetTotalCountsForPrefix, or modify your CL per
this
> API?

If you want to take over that CL, please feel free (but not compelled)! The
soonest I'd have time to tackle it would be tomorrow.

[twifkak, 2015-06-25 22:11:10]

On 2015/06/25 20:18:42, Ilya Sherman wrote:
> On 2015/06/25 20:08:20, twifkak wrote:
> > On 2015/06/24 23:22:29, Ilya Sherman wrote:
> > > I don't have a strong preference either way.  I do have a question though:
> > Could
> > > we get clearer failure messages by using a custom struct with custom
pretty
> > > printing, rather than an std::pair?
> > 
> > I don't think gtest supports printing a diff between expected and actual,
> based
> > on this line in the implementation of EXPECT_EQ:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/src/....
> > 
> > One option is to write a custom matcher in gmock. The resulting syntax would
> be
> > something like:
> > 
> > EXPECT_THAT(histogram.GetAllSamples("foo"),
> >             BucketsAre(Pair(1, 2), Pair(3, 4));
> > 
> > Because this is a variadic matcher, AFAICT, this requires a couple of
classes
> > and a function, per the implementation of ElementsAre in
> > testing/gmock/include/gmock/gmock-matchers.h. It's a significant enough
amount
> > of magic templated code that I'm not sure the benefit is worth the
complexity
> > cost.
> > 
> > There is an easy-to-use macro for writing unary matchers:
> >
>
https://code.google.com/p/googlemock/wiki/CookBook#Writing_New_Parameterized_...,
> > but we're back to where we started, being unable to specific container
> literals
> > succinctly.
> 
> I was thinking that we could just change the pretty printing of the elements
> themselves, to something a little clearer than (2, 1).  I wasn't even thinking
> of the possibility of writing a custom matcher -- I do agree that it's
probably
> not worth the implementation complexity.

Something like this?

using ::testing::PrintToString;

MATCHER_P2(Bucket,
           min,
           count,
           PrintToString(count) + " sample(s) at bucket " +
               PrintToString(min)) {
  return arg.first == min && arg.second == count;
}

Sample output:

[ RUN      ] HistogramTesterTest.TestGetAllSamples
../../base/test/histogram_tester_unittest.cc:102: Failure
Value of: tester.GetAllSamples(kHistogram2)
Expected: has 3 elements where
element #0 1 sample(s) at bucket 2,
element #1 2 sample(s) at bucket 3,
element #2 1 sample(s) at bucket 5
  Actual: { (2, 1), (3, 1), (5, 1) }, whose element #1 doesn't match
[  FAILED  ] HistogramTesterTest.TestGetAllSamples (0 ms)

[Ilya Sherman, 2015-06-25 22:27:13]

On 2015/06/25 22:11:10, twifkak wrote:
> On 2015/06/25 20:18:42, Ilya Sherman wrote:
> > On 2015/06/25 20:08:20, twifkak wrote:
> > > On 2015/06/24 23:22:29, Ilya Sherman wrote:
> > > > I don't have a strong preference either way.  I do have a question
though:
> > > Could
> > > > we get clearer failure messages by using a custom struct with custom
> pretty
> > > > printing, rather than an std::pair?
> > > 
> > > I don't think gtest supports printing a diff between expected and actual,
> > based
> > > on this line in the implementation of EXPECT_EQ:
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/src/....
> > > 
> > > One option is to write a custom matcher in gmock. The resulting syntax
would
> > be
> > > something like:
> > > 
> > > EXPECT_THAT(histogram.GetAllSamples("foo"),
> > >             BucketsAre(Pair(1, 2), Pair(3, 4));
> > > 
> > > Because this is a variadic matcher, AFAICT, this requires a couple of
> classes
> > > and a function, per the implementation of ElementsAre in
> > > testing/gmock/include/gmock/gmock-matchers.h. It's a significant enough
> amount
> > > of magic templated code that I'm not sure the benefit is worth the
> complexity
> > > cost.
> > > 
> > > There is an easy-to-use macro for writing unary matchers:
> > >
> >
>
https://code.google.com/p/googlemock/wiki/CookBook#Writing_New_Parameterized_...,
> > > but we're back to where we started, being unable to specific container
> > literals
> > > succinctly.
> > 
> > I was thinking that we could just change the pretty printing of the elements
> > themselves, to something a little clearer than (2, 1).  I wasn't even
thinking
> > of the possibility of writing a custom matcher -- I do agree that it's
> probably
> > not worth the implementation complexity.
> 
> Something like this?
> 
> using ::testing::PrintToString;
> 
> MATCHER_P2(Bucket,
>            min,
>            count,
>            PrintToString(count) + " sample(s) at bucket " +
>                PrintToString(min)) {
>   return arg.first == min && arg.second == count;
> }
> 
> Sample output:
> 
> [ RUN      ] HistogramTesterTest.TestGetAllSamples
> ../../base/test/histogram_tester_unittest.cc:102: Failure
> Value of: tester.GetAllSamples(kHistogram2)
> Expected: has 3 elements where
> element #0 1 sample(s) at bucket 2,
> element #1 2 sample(s) at bucket 3,
> element #2 1 sample(s) at bucket 5
>   Actual: { (2, 1), (3, 1), (5, 1) }, whose element #1 doesn't match
> [  FAILED  ] HistogramTesterTest.TestGetAllSamples (0 ms)

I was hoping the "Actual: " line would also have the same pretty-printed format.

[Ilya Sherman, 2015-06-25 22:29:23]

On 2015/06/25 22:27:13, Ilya Sherman wrote:
> On 2015/06/25 22:11:10, twifkak wrote:
> > On 2015/06/25 20:18:42, Ilya Sherman wrote:
> > > On 2015/06/25 20:08:20, twifkak wrote:
> > > > On 2015/06/24 23:22:29, Ilya Sherman wrote:
> > > > > I don't have a strong preference either way.  I do have a question
> though:
> > > > Could
> > > > > we get clearer failure messages by using a custom struct with custom
> > pretty
> > > > > printing, rather than an std::pair?
> > > > 
> > > > I don't think gtest supports printing a diff between expected and
actual,
> > > based
> > > > on this line in the implementation of EXPECT_EQ:
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/src/....
> > > > 
> > > > One option is to write a custom matcher in gmock. The resulting syntax
> would
> > > be
> > > > something like:
> > > > 
> > > > EXPECT_THAT(histogram.GetAllSamples("foo"),
> > > >             BucketsAre(Pair(1, 2), Pair(3, 4));
> > > > 
> > > > Because this is a variadic matcher, AFAICT, this requires a couple of
> > classes
> > > > and a function, per the implementation of ElementsAre in
> > > > testing/gmock/include/gmock/gmock-matchers.h. It's a significant enough
> > amount
> > > > of magic templated code that I'm not sure the benefit is worth the
> > complexity
> > > > cost.
> > > > 
> > > > There is an easy-to-use macro for writing unary matchers:
> > > >
> > >
> >
>
https://code.google.com/p/googlemock/wiki/CookBook#Writing_New_Parameterized_...,
> > > > but we're back to where we started, being unable to specific container
> > > literals
> > > > succinctly.
> > > 
> > > I was thinking that we could just change the pretty printing of the
elements
> > > themselves, to something a little clearer than (2, 1).  I wasn't even
> thinking
> > > of the possibility of writing a custom matcher -- I do agree that it's
> > probably
> > > not worth the implementation complexity.
> > 
> > Something like this?
> > 
> > using ::testing::PrintToString;
> > 
> > MATCHER_P2(Bucket,
> >            min,
> >            count,
> >            PrintToString(count) + " sample(s) at bucket " +
> >                PrintToString(min)) {
> >   return arg.first == min && arg.second == count;
> > }
> > 
> > Sample output:
> > 
> > [ RUN      ] HistogramTesterTest.TestGetAllSamples
> > ../../base/test/histogram_tester_unittest.cc:102: Failure
> > Value of: tester.GetAllSamples(kHistogram2)
> > Expected: has 3 elements where
> > element #0 1 sample(s) at bucket 2,
> > element #1 2 sample(s) at bucket 3,
> > element #2 1 sample(s) at bucket 5
> >   Actual: { (2, 1), (3, 1), (5, 1) }, whose element #1 doesn't match
> > [  FAILED  ] HistogramTesterTest.TestGetAllSamples (0 ms)
> 
> I was hoping the "Actual: " line would also have the same pretty-printed
format.

(Disclaimer: I'm not actually super fluent in Gmock, so I don't know whether the
things I'm asking for are hard or easy to implement.)

[twifkak, 2015-06-25 23:10:20]

On 2015/06/25 22:29:23, Ilya Sherman wrote:
> On 2015/06/25 22:27:13, Ilya Sherman wrote:
> > On 2015/06/25 22:11:10, twifkak wrote:
> > > On 2015/06/25 20:18:42, Ilya Sherman wrote:
> > > > On 2015/06/25 20:08:20, twifkak wrote:
> > > > > On 2015/06/24 23:22:29, Ilya Sherman wrote:
> > > > > > I don't have a strong preference either way.  I do have a question
> > though:
> > > > > Could
> > > > > > we get clearer failure messages by using a custom struct with custom
> > > pretty
> > > > > > printing, rather than an std::pair?
> > > > > 
> > > > > I don't think gtest supports printing a diff between expected and
> actual,
> > > > based
> > > > > on this line in the implementation of EXPECT_EQ:
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/src/....
> > > > > 
> > > > > One option is to write a custom matcher in gmock. The resulting syntax
> > would
> > > > be
> > > > > something like:
> > > > > 
> > > > > EXPECT_THAT(histogram.GetAllSamples("foo"),
> > > > >             BucketsAre(Pair(1, 2), Pair(3, 4));
> > > > > 
> > > > > Because this is a variadic matcher, AFAICT, this requires a couple of
> > > classes
> > > > > and a function, per the implementation of ElementsAre in
> > > > > testing/gmock/include/gmock/gmock-matchers.h. It's a significant
enough
> > > amount
> > > > > of magic templated code that I'm not sure the benefit is worth the
> > > complexity
> > > > > cost.
> > > > > 
> > > > > There is an easy-to-use macro for writing unary matchers:
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/googlemock/wiki/CookBook#Writing_New_Parameterized_...,
> > > > > but we're back to where we started, being unable to specific container
> > > > literals
> > > > > succinctly.
> > > > 
> > > > I was thinking that we could just change the pretty printing of the
> elements
> > > > themselves, to something a little clearer than (2, 1).  I wasn't even
> > thinking
> > > > of the possibility of writing a custom matcher -- I do agree that it's
> > > probably
> > > > not worth the implementation complexity.
> > > 
> > > Something like this?
> > > 
> > > using ::testing::PrintToString;
> > > 
> > > MATCHER_P2(Bucket,
> > >            min,
> > >            count,
> > >            PrintToString(count) + " sample(s) at bucket " +
> > >                PrintToString(min)) {
> > >   return arg.first == min && arg.second == count;
> > > }
> > > 
> > > Sample output:
> > > 
> > > [ RUN      ] HistogramTesterTest.TestGetAllSamples
> > > ../../base/test/histogram_tester_unittest.cc:102: Failure
> > > Value of: tester.GetAllSamples(kHistogram2)
> > > Expected: has 3 elements where
> > > element #0 1 sample(s) at bucket 2,
> > > element #1 2 sample(s) at bucket 3,
> > > element #2 1 sample(s) at bucket 5
> > >   Actual: { (2, 1), (3, 1), (5, 1) }, whose element #1 doesn't match
> > > [  FAILED  ] HistogramTesterTest.TestGetAllSamples (0 ms)
> > 
> > I was hoping the "Actual: " line would also have the same pretty-printed
> format.
> 
> (Disclaimer: I'm not actually super fluent in Gmock, so I don't know whether
the
> things I'm asking for are hard or easy to implement.)

I'm not super fluent in gmock either - I'm learning as you ask these questions.
Looks like to do that, I would need to make a new Bucket class instead of
pair<int, int>, and then return vector<Bucket>. The Bucket class would have its
own PrintTo override, which is retrieved by argument-dependent lookup.

[Ilya Sherman, 2015-06-26 01:53:14]

On 2015/06/25 23:10:20, twifkak wrote:
> On 2015/06/25 22:29:23, Ilya Sherman wrote:
> > On 2015/06/25 22:27:13, Ilya Sherman wrote:
> > > On 2015/06/25 22:11:10, twifkak wrote:
> > > > On 2015/06/25 20:18:42, Ilya Sherman wrote:
> > > > > On 2015/06/25 20:08:20, twifkak wrote:
> > > > > > On 2015/06/24 23:22:29, Ilya Sherman wrote:
> > > > > > > I don't have a strong preference either way.  I do have a question
> > > though:
> > > > > > Could
> > > > > > > we get clearer failure messages by using a custom struct with
custom
> > > > pretty
> > > > > > > printing, rather than an std::pair?
> > > > > > 
> > > > > > I don't think gtest supports printing a diff between expected and
> > actual,
> > > > > based
> > > > > > on this line in the implementation of EXPECT_EQ:
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/src/....
> > > > > > 
> > > > > > One option is to write a custom matcher in gmock. The resulting
syntax
> > > would
> > > > > be
> > > > > > something like:
> > > > > > 
> > > > > > EXPECT_THAT(histogram.GetAllSamples("foo"),
> > > > > >             BucketsAre(Pair(1, 2), Pair(3, 4));
> > > > > > 
> > > > > > Because this is a variadic matcher, AFAICT, this requires a couple
of
> > > > classes
> > > > > > and a function, per the implementation of ElementsAre in
> > > > > > testing/gmock/include/gmock/gmock-matchers.h. It's a significant
> enough
> > > > amount
> > > > > > of magic templated code that I'm not sure the benefit is worth the
> > > > complexity
> > > > > > cost.
> > > > > > 
> > > > > > There is an easy-to-use macro for writing unary matchers:
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/googlemock/wiki/CookBook#Writing_New_Parameterized_...,
> > > > > > but we're back to where we started, being unable to specific
container
> > > > > literals
> > > > > > succinctly.
> > > > > 
> > > > > I was thinking that we could just change the pretty printing of the
> > elements
> > > > > themselves, to something a little clearer than (2, 1).  I wasn't even
> > > thinking
> > > > > of the possibility of writing a custom matcher -- I do agree that it's
> > > > probably
> > > > > not worth the implementation complexity.
> > > > 
> > > > Something like this?
> > > > 
> > > > using ::testing::PrintToString;
> > > > 
> > > > MATCHER_P2(Bucket,
> > > >            min,
> > > >            count,
> > > >            PrintToString(count) + " sample(s) at bucket " +
> > > >                PrintToString(min)) {
> > > >   return arg.first == min && arg.second == count;
> > > > }
> > > > 
> > > > Sample output:
> > > > 
> > > > [ RUN      ] HistogramTesterTest.TestGetAllSamples
> > > > ../../base/test/histogram_tester_unittest.cc:102: Failure
> > > > Value of: tester.GetAllSamples(kHistogram2)
> > > > Expected: has 3 elements where
> > > > element #0 1 sample(s) at bucket 2,
> > > > element #1 2 sample(s) at bucket 3,
> > > > element #2 1 sample(s) at bucket 5
> > > >   Actual: { (2, 1), (3, 1), (5, 1) }, whose element #1 doesn't match
> > > > [  FAILED  ] HistogramTesterTest.TestGetAllSamples (0 ms)
> > > 
> > > I was hoping the "Actual: " line would also have the same pretty-printed
> > format.
> > 
> > (Disclaimer: I'm not actually super fluent in Gmock, so I don't know whether
> the
> > things I'm asking for are hard or easy to implement.)
> 
> I'm not super fluent in gmock either - I'm learning as you ask these
questions.
> Looks like to do that, I would need to make a new Bucket class instead of
> pair<int, int>, and then return vector<Bucket>. The Bucket class would have
its
> own PrintTo override, which is retrieved by argument-dependent lookup.

Yeah, that sounds like what I was thinking of.  What does the output look like
for that?

[twifkak, 2015-06-26 08:25:04]

On 2015/06/26 01:53:14, Ilya Sherman wrote:
> On 2015/06/25 23:10:20, twifkak wrote:
> > On 2015/06/25 22:29:23, Ilya Sherman wrote:
> > > On 2015/06/25 22:27:13, Ilya Sherman wrote:
> > > > On 2015/06/25 22:11:10, twifkak wrote:
> > > > > On 2015/06/25 20:18:42, Ilya Sherman wrote:
> > > > > > On 2015/06/25 20:08:20, twifkak wrote:
> > > > > > > On 2015/06/24 23:22:29, Ilya Sherman wrote:
> > > > > > > > I don't have a strong preference either way.  I do have a
question
> > > > though:
> > > > > > > Could
> > > > > > > > we get clearer failure messages by using a custom struct with
> custom
> > > > > pretty
> > > > > > > > printing, rather than an std::pair?
> > > > > > > 
> > > > > > > I don't think gtest supports printing a diff between expected and
> > > actual,
> > > > > > based
> > > > > > > on this line in the implementation of EXPECT_EQ:
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/src/....
> > > > > > > 
> > > > > > > One option is to write a custom matcher in gmock. The resulting
> syntax
> > > > would
> > > > > > be
> > > > > > > something like:
> > > > > > > 
> > > > > > > EXPECT_THAT(histogram.GetAllSamples("foo"),
> > > > > > >             BucketsAre(Pair(1, 2), Pair(3, 4));
> > > > > > > 
> > > > > > > Because this is a variadic matcher, AFAICT, this requires a couple
> of
> > > > > classes
> > > > > > > and a function, per the implementation of ElementsAre in
> > > > > > > testing/gmock/include/gmock/gmock-matchers.h. It's a significant
> > enough
> > > > > amount
> > > > > > > of magic templated code that I'm not sure the benefit is worth the
> > > > > complexity
> > > > > > > cost.
> > > > > > > 
> > > > > > > There is an easy-to-use macro for writing unary matchers:
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/googlemock/wiki/CookBook#Writing_New_Parameterized_...,
> > > > > > > but we're back to where we started, being unable to specific
> container
> > > > > > literals
> > > > > > > succinctly.
> > > > > > 
> > > > > > I was thinking that we could just change the pretty printing of the
> > > elements
> > > > > > themselves, to something a little clearer than (2, 1).  I wasn't
even
> > > > thinking
> > > > > > of the possibility of writing a custom matcher -- I do agree that
it's
> > > > > probably
> > > > > > not worth the implementation complexity.
> > > > > 
> > > > > Something like this?
> > > > > 
> > > > > using ::testing::PrintToString;
> > > > > 
> > > > > MATCHER_P2(Bucket,
> > > > >            min,
> > > > >            count,
> > > > >            PrintToString(count) + " sample(s) at bucket " +
> > > > >                PrintToString(min)) {
> > > > >   return arg.first == min && arg.second == count;
> > > > > }
> > > > > 
> > > > > Sample output:
> > > > > 
> > > > > [ RUN      ] HistogramTesterTest.TestGetAllSamples
> > > > > ../../base/test/histogram_tester_unittest.cc:102: Failure
> > > > > Value of: tester.GetAllSamples(kHistogram2)
> > > > > Expected: has 3 elements where
> > > > > element #0 1 sample(s) at bucket 2,
> > > > > element #1 2 sample(s) at bucket 3,
> > > > > element #2 1 sample(s) at bucket 5
> > > > >   Actual: { (2, 1), (3, 1), (5, 1) }, whose element #1 doesn't match
> > > > > [  FAILED  ] HistogramTesterTest.TestGetAllSamples (0 ms)
> > > > 
> > > > I was hoping the "Actual: " line would also have the same pretty-printed
> > > format.
> > > 
> > > (Disclaimer: I'm not actually super fluent in Gmock, so I don't know
whether
> > the
> > > things I'm asking for are hard or easy to implement.)
> > 
> > I'm not super fluent in gmock either - I'm learning as you ask these
> questions.
> > Looks like to do that, I would need to make a new Bucket class instead of
> > pair<int, int>, and then return vector<Bucket>. The Bucket class would have
> its
> > own PrintTo override, which is retrieved by argument-dependent lookup.
> 
> Yeah, that sounds like what I was thinking of.  What does the output look like
> for that?

[ RUN      ] HistogramTesterTest.TestGetAllSamples
../../base/test/histogram_tester_unittest.cc:91: Failure
Value of: tester.GetAllSamples(kHistogram2)
Expected: has 3 elements where
element #0 is equal to 1 sample in bucket 2,
element #1 is equal to 2 samples in bucket 3,
element #2 is equal to 1 sample in bucket 5
  Actual: { 1 sample in bucket 2, 1 sample in bucket 3, 1 sample in bucket 5 },
whose element #1 doesn't match
[  FAILED  ] HistogramTesterTest.TestGetAllSamples (1 ms)

[Ilya Sherman, 2015-06-27 07:25:20]

Sweet!  LGTM, with some ideas/suggestions:

https://codereview.chromium.org/1177263004/diff/40001/base/test/histogram_tes...
File base/test/histogram_tester.cc (right):

https://codereview.chromium.org/1177263004/diff/40001/base/test/histogram_tes...
base/test/histogram_tester.cc:142: << "in bucket " << bucket.min_;
WDYT of something shorter, like the following?

  *os << bucket.min << ": " << bucket.count;  // e.g. "1: 4"

I'm not really sure -- that might be too dense, and not much clearer than a
pair.  I guess I'm trying to find something a little denser, without being so
dense as to be confusing.

https://codereview.chromium.org/1177263004/diff/40001/base/test/histogram_tes...
File base/test/histogram_tester.h (right):

https://codereview.chromium.org/1177263004/diff/40001/base/test/histogram_tes...
base/test/histogram_tester.h:55: // object, as pair<Sample, Count>, where the
Sample is the min boundary of the
nit: Please update "pair<Sample, Count>"

https://codereview.chromium.org/1177263004/diff/40001/base/test/histogram_tes...
base/test/histogram_tester.h:112: base::HistogramBase::Count count_;
nit: Can these be const?

https://codereview.chromium.org/1177263004/diff/40001/base/test/histogram_tes...
base/test/histogram_tester.h:112: base::HistogramBase::Count count_;
Optional nit: It's probably fine to make this a struct, with publicly visible
data members, especially if they're const.

https://codereview.chromium.org/1177263004/diff/40001/base/test/histogram_tes...
File base/test/histogram_tester_unittest.cc (right):

https://codereview.chromium.org/1177263004/diff/40001/base/test/histogram_tes...
base/test/histogram_tester_unittest.cc:89: UMA_HISTOGRAM_COUNTS_100(kHistogram2,
5);
nit: Might be clearer to use an UMA_HISTOGRAM_ENUMERATION or UMA_SPARSE_SLOWLY,
since those have buckets of width 1.

[ncarter (slow), 2015-06-29 17:00:03]

lgtm

[twifkak, 2015-06-29 21:52:29]

https://codereview.chromium.org/1177263004/diff/40001/base/test/histogram_tes...
File base/test/histogram_tester.cc (right):

https://codereview.chromium.org/1177263004/diff/40001/base/test/histogram_tes...
base/test/histogram_tester.cc:142: << "in bucket " << bucket.min_;
On 2015/06/27 07:25:19, Ilya Sherman wrote:
> WDYT of something shorter, like the following?
> 
>   *os << bucket.min << ": " << bucket.count;  // e.g. "1: 4"
> 
> I'm not really sure -- that might be too dense, and not much clearer than a
> pair.  I guess I'm trying to find something a little denser, without being so
> dense as to be confusing.

That doesn't seem to be any clearer than a pair. However, I was bothered by the
fact that it was output in reverse order to the constructor args. How about
this?

https://codereview.chromium.org/1177263004/diff/40001/base/test/histogram_tes...
File base/test/histogram_tester.h (right):

https://codereview.chromium.org/1177263004/diff/40001/base/test/histogram_tes...
base/test/histogram_tester.h:55: // object, as pair<Sample, Count>, where the
Sample is the min boundary of the
On 2015/06/27 07:25:19, Ilya Sherman wrote:
> nit: Please update "pair<Sample, Count>"

Done.

https://codereview.chromium.org/1177263004/diff/40001/base/test/histogram_tes...
base/test/histogram_tester.h:112: base::HistogramBase::Count count_;
On 2015/06/27 07:25:19, Ilya Sherman wrote:
> Optional nit: It's probably fine to make this a struct, with publicly visible
> data members, especially if they're const.

Done.

https://codereview.chromium.org/1177263004/diff/40001/base/test/histogram_tes...
base/test/histogram_tester.h:112: base::HistogramBase::Count count_;
On 2015/06/27 07:25:19, Ilya Sherman wrote:
> nit: Can these be const?

Done.

https://codereview.chromium.org/1177263004/diff/40001/base/test/histogram_tes...
File base/test/histogram_tester_unittest.cc (right):

https://codereview.chromium.org/1177263004/diff/40001/base/test/histogram_tes...
base/test/histogram_tester_unittest.cc:89: UMA_HISTOGRAM_COUNTS_100(kHistogram2,
5);
On 2015/06/27 07:25:19, Ilya Sherman wrote:
> nit: Might be clearer to use an UMA_HISTOGRAM_ENUMERATION or
UMA_SPARSE_SLOWLY,
> since those have buckets of width 1.

Done.

[Ilya Sherman, 2015-06-29 22:05:34]

LGTM, thanks.

https://codereview.chromium.org/1177263004/diff/40001/base/test/histogram_tes...
File base/test/histogram_tester.cc (right):

https://codereview.chromium.org/1177263004/diff/40001/base/test/histogram_tes...
base/test/histogram_tester.cc:142: << "in bucket " << bucket.min_;
On 2015/06/29 21:52:29, twifkak wrote:
> On 2015/06/27 07:25:19, Ilya Sherman wrote:
> > WDYT of something shorter, like the following?
> > 
> >   *os << bucket.min << ": " << bucket.count;  // e.g. "1: 4"
> > 
> > I'm not really sure -- that might be too dense, and not much clearer than a
> > pair.  I guess I'm trying to find something a little denser, without being
so
> > dense as to be confusing.
> 
> That doesn't seem to be any clearer than a pair. However, I was bothered by
the
> fact that it was output in reverse order to the constructor args. How about
> this?

Yeah, I think that's a good compromise between clarity and density.  Thanks!

https://codereview.chromium.org/1177263004/diff/60001/base/test/histogram_tes...
File base/test/histogram_tester.h (right):

https://codereview.chromium.org/1177263004/diff/60001/base/test/histogram_tes...
base/test/histogram_tester.h:102: Bucket(base::HistogramBase::Sample min_,
base::HistogramBase::Count count_)
nit: No need to underscore any names -- the compiler will do the right thing if
you write "min(min)".

[twifkak, 2015-06-29 22:38:06]

https://codereview.chromium.org/1177263004/diff/60001/base/test/histogram_tes...
File base/test/histogram_tester.h (right):

https://codereview.chromium.org/1177263004/diff/60001/base/test/histogram_tes...
base/test/histogram_tester.h:102: Bucket(base::HistogramBase::Sample min_,
base::HistogramBase::Count count_)
On 2015/06/29 22:05:34, Ilya Sherman wrote:
> nit: No need to underscore any names -- the compiler will do the right thing
if
> you write "min(min)".

Ah, cool. Done.

[twifkak, 2015-06-29 22:39:57]

mlerman and phajdan.jr: Please review this CL.

[Paweł Hajdan Jr., 2015-06-30 08:15:30]

LGTM

[Mike Lerman, 2015-06-30 10:16:08]

Content with Ilya's review. Lgtm.

[twifkak, 2015-06-30 17:37:51]

The CQ bit was checked by twifkak@chromium.org

[twifkak, 2015-06-30 17:37:52]

The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org,
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1177263004/#ps80001
(title: "Remove unnecessary underscores from Bucket constructor param names.")

[commit-bot: I haz the power, 2015-06-30 17:38:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1177263004/80001

[commit-bot: I haz the power, 2015-06-30 17:54:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-06-30 17:54:35]

Exceeded global retry quota

[twifkak, 2015-06-30 18:04:07]

On 2015/06/30 17:54:35, commit-bot: I haz the power wrote:
> Exceeded global retry quota

Uploaded patch set 6 to fix the platform-specific build errors. Retrying CQ.

[twifkak, 2015-06-30 18:04:14]

The CQ bit was checked by twifkak@chromium.org

[twifkak, 2015-06-30 18:04:14]

The patchset sent to the CQ was uploaded after l-g-t-m from
mlerman@chromium.org, isherman@chromium.org, nick@chromium.org,
phajdan.jr@chromium.org
Link to the patchset: https://codereview.chromium.org/1177263004/#ps100001
(title: "Remove member variable const qualifications, for std::vector.")

[commit-bot: I haz the power, 2015-06-30 18:05:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1177263004/100001

[commit-bot: I haz the power, 2015-06-30 19:13:33]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-06-30 19:14:39]

Patchset 6 (id:??) landed as
https://crrev.com/5c8510d1c1078e82357005cf56f0af8acde0c168
Cr-Commit-Position: refs/heads/master@{#336842}