base/test/histogram_tester.h: Add a way to query a whole family of related

base/test/histogram_tester.h: Add a way to query a whole family of related
histograms at once.

Use this to clean up site_isolation_stats_gatherer_browsertest.cc

BUG=481066
Committed: https://crrev.com/dd65d77f2f586bb890b9bd3574e7ceb7f9691763
Cr-Commit-Position: refs/heads/master@{#340510}

[ncarter (slow), 2015-06-19 21:19:45]

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

[ncarter (slow), 2015-06-19 21:19:46]

Hi Mike,

Please let me know what you think of this approach. I'm happy to write some unit
tests if you think it's a good idea.

[Mike Lerman, 2015-06-22 14:00:41]

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

[Mike Lerman, 2015-06-22 14:00:42]

My initial reaction is that you're writing a lot of code that's particular to
your needs, that may not belong in the histogram_tester class. It seems there
are two main things you're driving towards.
1) "cases where the code under test is choosing among a family of related
histograms and incrementing one of them"
2) Creating a syntax for querying and reading the results from a bunch of
histograms in various buckets.

I think there's a good argument for 1); this is a pattern that does happen a
fair bit. e.g. A code flow will increment 1 or 2 in a family of histograms, and
the others should remain 0. I'm less convinced about 2) being a good general
case; I think using the provided Expect*Count methods provide good type checking
and all the needed expressiveness. Future developers trying to understand, write
and parse these strings may be pressed.

Perhaps, then, the right method to add is one that checks that a whole bunch of
histograms, specified by a const char*[] perhaps, are all zero? Perhaps allowing
a second parameter where you can exclude one from the family?

+isherman@, who was my reviewer for the class, and is an owner of histograms
code (I am not).

Thanks!
Mike

[ncarter (slow), 2015-06-22 17:07:26]

On 2015/06/22 14:00:42, Mike Lerman wrote:
> My initial reaction is that you're writing a lot of code that's particular to
> your needs, that may not belong in the histogram_tester class. It seems there
> are two main things you're driving towards.
> 1) "cases where the code under test is choosing among a family of related
> histograms and incrementing one of them"
> 2) Creating a syntax for querying and reading the results from a bunch of
> histograms in various buckets.

[2] is really about making the output more understandable in the event of
failure. If you look at the left hand side of
site_isolation_stats_gatherer_browsertest.cc, you'll see that I was indeed able
to write this test using the existing Expect*Count methods. However, I found it
a pain in the butt when things actually started failing -- the EXPECT's were in
a helper function, it was tough to tell who the caller was. And because the
asserts operated a single metric at a time, it was really hard to get a picture
of what was happening in practice. In this proposed change, two differences
conspire to make the output more readable: one, we hoist the EXPECT back up into
the failing test code, which affords better context for the error, and two, we
leverage gtest's ability to compute line-by-line diffs so that the "expected
incremented metric" and "actual incremented metric" wind up being printed
side-by-side. 

Here's an example of the new style output compared to the original, for the same
error: https://gist.github.com/anonymous/5bfd13bef22757072e83 

> I think there's a good argument for 1); this is a pattern that does happen a
> fair bit. e.g. A code flow will increment 1 or 2 in a family of histograms,
and
> the others should remain 0. I'm less convinced about 2) being a good general
> case; I think using the provided Expect*Count methods provide good type
checking
> and all the needed expressiveness. Future developers trying to understand,
write
> and parse these strings may be pressed.

I think this is a fair criticism. My goal was for the strings to be obviously
self-documenting -- from your feedback it sounds like I might have made them too
complicated. I actually only added the "[bucket_min]" syntax at the last minute
-- originally I was just showing Metric=Count, which I think IS pretty
self-explanatory, and I'd be fine going back to that simpler behavior. As you
point out, in the cases where you do care about the bucket value, you can use a
follow-on Expect* behavior.

> Perhaps, then, the right method to add is one that checks that a whole bunch
of
> histograms, specified by a const char*[] perhaps, are all zero? Perhaps
allowing
> a second parameter where you can exclude one from the family?

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.e.:

HistogramTester tester;
vector<string> metrics;
metrics.push_back("My.Metric=1")
tester.ExpectFooBars("My.", metrics);

vs.

HistogramTester tester;
tester.ExpectFooBars("My.", "My.Metric=1");

vs.

HistogramTester tester;
EXPECT_EQ("My.Metric=1", tester.GetFooBars("My.")) << "Extra context";


> 
> +isherman@, who was my reviewer for the class, and is an owner of histograms
> code (I am not).
> 
> Thanks!
> Mike

[Ilya Sherman, 2015-06-23 00:57:41]

I'm okay with this proposed change, though it does seem like it might encourage
folks to write overly complex test code.  (IMO, the test you're cleaning up is
on the overly complex side -- I have to think about the logic and branches
within the test code to understand what exactly is being tested.  Typically, I'd
prefer to just see straightline code, without any branches, in tests, even when
it results in some redundancy across related test cases.)

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

https://codereview.chromium.org/1190423006/diff/20001/base/test/histogram_tes...
base/test/histogram_tester.cc:85: << " many histograms.";
nit: Perhaps output the query?

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

https://codereview.chromium.org/1190423006/diff/20001/base/test/histogram_tes...
base/test/histogram_tester.h:50: // Finds histograms whose names start with
|query|, and return a text
nit: "return" -> "returns"

https://codereview.chromium.org/1190423006/diff/20001/base/test/histogram_tes...
base/test/histogram_tester.h:66: // good job of formatting human-readable diffs
of multi-line strings.
nit: You should probably mention somewhere that the returned list is sorted
alphabetically.

[Mike Lerman, 2015-06-23 17:41:59]

On 2015/06/22 17:07:26, ncarter wrote:
> On 2015/06/22 14:00:42, Mike Lerman wrote:
> > My initial reaction is that you're writing a lot of code that's particular
to
> > your needs, that may not belong in the histogram_tester class. It seems
there
> > are two main things you're driving towards.
> > 1) "cases where the code under test is choosing among a family of related
> > histograms and incrementing one of them"
> > 2) Creating a syntax for querying and reading the results from a bunch of
> > histograms in various buckets.
> 
> [2] is really about making the output more understandable in the event of
> failure. If you look at the left hand side of
> site_isolation_stats_gatherer_browsertest.cc, you'll see that I was indeed
able
> to write this test using the existing Expect*Count methods. However, I found
it
> a pain in the butt when things actually started failing -- the EXPECT's were
in
> a helper function, it was tough to tell who the caller was. And because the
> asserts operated a single metric at a time, it was really hard to get a
picture
> of what was happening in practice. In this proposed change, two differences
> conspire to make the output more readable: one, we hoist the EXPECT back up
into
> the failing test code, which affords better context for the error, and two, we
> leverage gtest's ability to compute line-by-line diffs so that the "expected
> incremented metric" and "actual incremented metric" wind up being printed
> side-by-side. 
> 
> Here's an example of the new style output compared to the original, for the
same
> error: https://gist.github.com/anonymous/5bfd13bef22757072e83 

I've noticed myself that having the EXPECT statements be in a helper function
made things hard to diagnose.

> 
> > I think there's a good argument for 1); this is a pattern that does happen a
> > fair bit. e.g. A code flow will increment 1 or 2 in a family of histograms,
> and
> > the others should remain 0. I'm less convinced about 2) being a good general
> > case; I think using the provided Expect*Count methods provide good type
> checking
> > and all the needed expressiveness. Future developers trying to understand,
> write
> > and parse these strings may be pressed.
> 
> I think this is a fair criticism. My goal was for the strings to be obviously
> self-documenting -- from your feedback it sounds like I might have made them
too
> complicated. I actually only added the "[bucket_min]" syntax at the last
minute
> -- originally I was just showing Metric=Count, which I think IS pretty
> self-explanatory, and I'd be fine going back to that simpler behavior. As you
> point out, in the cases where you do care about the bucket value, you can use
a
> follow-on Expect* behavior.
> 

I do admit, it's really nice when debugging a test to know which bucket you
added a value to, when you were otherwise expecting a total count of 0.
I think the following 2 statements are generally true:
1) As a writer of a test, I want to specify what the _total_ counts need to be
for each histogram. If I need to specify bucket values, I can use the provided
Expect*Count() calls.
2) As a debugger of a failing test, the system should tell me, whenever the
expect total count is not met, what the values in each bucket are.

> > Perhaps, then, the right method to add is one that checks that a whole bunch
> of
> > histograms, specified by a const char*[] perhaps, are all zero? Perhaps
> allowing
> > a second parameter where you can exclude one from the family?
> 
> 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.e.:
> 
> HistogramTester tester;
> vector<string> metrics;
> metrics.push_back("My.Metric=1")
> tester.ExpectFooBars("My.", metrics);
> 
> vs.
> 
> HistogramTester tester;
> tester.ExpectFooBars("My.", "My.Metric=1");
> 
> vs.
> 
> HistogramTester tester;
> EXPECT_EQ("My.Metric=1", tester.GetFooBars("My.")) << "Extra context";

I'm hesitant about strings for structured data, although I'm keen to avoid
un-necessary verbosity. Is it worth adding a helper that can generate these
strings for you?

HistogramTester tester;
EXPECT_EQ(HistogramTester.Params("My.Metric", 1, "My.OtherMetric", 2),
tester.getFooBars("My.")) << "Extra information";

We could then define an object that can accept, 1,2,3..6 parameters (or some
reasonable number) in an overloaded constructor? We would just have to make the
output look nice. It looks like gtest may support this:
https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/incl...

> 
> 
> > 
> > +isherman@, who was my reviewer for the class, and is an owner of histograms
> > code (I am not).
> > 
> > Thanks!
> > Mike

[twifkak, 2015-07-01 20:03:25]

Nick: Reminder that https://codereview.chromium.org/1177263004/ is landed, so
you may begin work on this. (I'm not depending on it, so don't consider this a
request.)

[ncarter (slow), 2015-07-23 20:45:35]

nick@chromium.org changed reviewers:
+ twifkak@chromium.org
- isherman@chromium.org, mlerman@chromium.org

[ncarter (slow), 2015-07-23 20:45:35]

I finally got back to this and . It looks like both sherman & lerman are on
vacation.

twifkak: could you take a look to see if this is consistent with what we talked
about in codereview.chromium.org/1177263004? I'll try to find an owner.

[ncarter (slow), 2015-07-23 20:47:08]

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

[ncarter (slow), 2015-07-23 20:47:09]

+Paweł as base/test OWNER

[twifkak, 2015-07-23 21:21:18]

On 2015/07/23 20:45:35, ncarter wrote:
> I finally got back to this and . It looks like both sherman & lerman are on
> vacation.
> 
> twifkak: could you take a look to see if this is consistent with what we
talked
> about in codereview.chromium.org/1177263004? I'll try to find an owner.

Yup, this is what I had in mind. Does this work for you? Thanks for following
up!

[twifkak, 2015-07-23 21:21:54]

lgtm

nits

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

https://codereview.chromium.org/1190423006/diff/60001/base/test/histogram_tes...
base/test/histogram_tester.h:70: std::vector<Bucket> GetAllSamples(const
std::string& name) const;
Oops, good catch.

https://codereview.chromium.org/1190423006/diff/60001/base/test/histogram_tes...
base/test/histogram_tester.h:83: //   #include
"testing/gmock/include/gmock/gmock-matchers.h"
The documentation at https://wiki.corp.google.com/twiki/bin/view/Main/GMockGuide
says just to include gmock.h, as do existing uses:
https://code.google.com/p/chromium/codesearch#search/&q=%23include.*gmock%20-...

https://codereview.chromium.org/1190423006/diff/60001/base/test/histogram_tes...
base/test/histogram_tester.h:90: using CountsMap = std::map<std::string,
base::HistogramBase::Count>;
Using decls aren't explicitly called out in the style guide, but I think in
spirit this should probably go the same place typedefs go:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...

But I don't feel strongly about it.

https://codereview.chromium.org/1190423006/diff/60001/content/child/site_isol...
File content/child/site_isolation_stats_gatherer_browsertest.cc (right):

https://codereview.chromium.org/1190423006/diff/60001/content/child/site_isol...
content/child/site_isolation_stats_gatherer_browsertest.cc:17: #include
"testing/gmock/include/gmock/gmock-generated-matchers.h"
Ditto here, just gmock.h.

https://codereview.chromium.org/1190423006/diff/60001/content/child/site_isol...
content/child/site_isolation_stats_gatherer_browsertest.cc:109: }
IIUC, this doesn't replicate line 93 of the left side. Is that intended?

[ncarter (slow), 2015-07-23 21:50:43]

Thanks for the feedback! All done.

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

https://codereview.chromium.org/1190423006/diff/60001/base/test/histogram_tes...
base/test/histogram_tester.h:70: std::vector<Bucket> GetAllSamples(const
std::string& name) const;
On 2015/07/23 21:21:54, twifkak wrote:
> Oops, good catch.

Acknowledged.

https://codereview.chromium.org/1190423006/diff/60001/base/test/histogram_tes...
base/test/histogram_tester.h:83: //   #include
"testing/gmock/include/gmock/gmock-matchers.h"
On 2015/07/23 21:21:54, twifkak wrote:
> The documentation at
https://wiki.corp.google.com/twiki/bin/view/Main/GMockGuide
> says just to include gmock.h, as do existing uses:
>
https://code.google.com/p/chromium/codesearch#search/&q=%23include.*gmock%20-...

Done.

https://codereview.chromium.org/1190423006/diff/60001/base/test/histogram_tes...
base/test/histogram_tester.h:90: using CountsMap = std::map<std::string,
base::HistogramBase::Count>;
On 2015/07/23 21:21:54, twifkak wrote:
> Using decls aren't explicitly called out in the style guide, but I think in
> spirit this should probably go the same place typedefs go:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...
> 
> But I don't feel strongly about it.

Yeah, I'm torn about this generally. I'm usually a stickler for doing exactly
what you suggest (and I agree -- using aliases ought definitely be treated as
typedefs) but it's nonetheless a fairly common pattern in chrome to have
just-in-time typedefs, when they're not shared by more than one definition,
e.g.:
https://code.google.com/p/chromium/codesearch#search/&q=%5C_;%5Cn%5Cn%5C%20%5...

I wouldn't mind some chromium-dev clarification on this style, because we say
one thing and do another.

Anyhow, I hoisted it up, because everybody can agree that that's okay. Besides
for a public typedef, it probably matters even more to put them in the canonical
place.

https://codereview.chromium.org/1190423006/diff/60001/content/child/site_isol...
File content/child/site_isolation_stats_gatherer_browsertest.cc (right):

https://codereview.chromium.org/1190423006/diff/60001/content/child/site_isol...
content/child/site_isolation_stats_gatherer_browsertest.cc:17: #include
"testing/gmock/include/gmock/gmock-generated-matchers.h"
On 2015/07/23 21:21:54, twifkak wrote:
> Ditto here, just gmock.h.

Done.

https://codereview.chromium.org/1190423006/diff/60001/content/child/site_isol...
content/child/site_isolation_stats_gatherer_browsertest.cc:109: }
On 2015/07/23 21:21:54, twifkak wrote:
> IIUC, this doesn't replicate line 93 of the left side. Is that intended?

Yes, it's intended -- line 93 in the original was basically doing work that
GetAllSamples does now. 

The only two histograms that have bucket values I care about asserting against,
are UMA_HISTOGRAM_ENUMERATION("SiteIsolation.XSD.MimeType") and the
"RenderableStatusCode" ones. The rest are basically UMA_HISTOGRAM_COUNTS().

[Paweł Hajdan Jr., 2015-07-24 09:48:08]

LGTM

[ncarter (slow), 2015-07-24 16:29:26]

The CQ bit was checked by nick@chromium.org

[ncarter (slow), 2015-07-24 16:29:27]

The patchset sent to the CQ was uploaded after l-g-t-m from twifkak@chromium.org
Link to the patchset: https://codereview.chromium.org/1190423006/#ps80001
(title: "fixes from twifkak")

[commit-bot: I haz the power, 2015-07-24 16:29:57]

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

[commit-bot: I haz the power, 2015-07-24 16:36:03]

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

[commit-bot: I haz the power, 2015-07-24 16:36:04]

Try jobs failed on following builders:
  linux_arm_compile on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_arm_compi...)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[ncarter (slow), 2015-07-27 17:02:37]

The CQ bit was checked by nick@chromium.org

[commit-bot: I haz the power, 2015-07-27 17:03:49]

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

[commit-bot: I haz the power, 2015-07-27 18:30:05]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-07-27 18:30:43]

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