HistogramTester::GetHistogramSamplesSinceCreation never returns null

HistogramTester::GetHistogramSamplesSinceCreation never returns null

Currently, GetHistogramSamplesSinceCreation can return null if no histogram was generated. But that depends also on tests run prior to the current test. This CL changes this to returning empty samples instead, so that independently of previously run tests, the current test always gets the same response even if it does not generate any histograms.

BUG=473689
Committed: https://crrev.com/ae9604308c926cc7df6205b5207dbf63c6e55314
Cr-Commit-Position: refs/heads/master@{#341498}

[vabr (Chromium), 2015-07-31 10:35:59]

vabr@chromium.org changed reviewers:
+ atwilson@chromium.org, phajdan.jr@chromium.org

[vabr (Chromium), 2015-07-31 10:36:00]

Hi all,

@phajdan.jr -- please review the whole patch.
@atwilson -- please review chrome/browser/signin/cross_device_promo_unittest.cc

Thanks!
Vaclav

[Andrew T Wilson (Slow), 2015-07-31 12:58:21]

lgtm

[Lei Zhang, 2015-07-31 18:30:58]

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

[Lei Zhang, 2015-07-31 18:30:59]

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

https://codereview.chromium.org/1264123002/diff/60001/base/test/histogram_tes...
base/test/histogram_tester.cc:123: return scoped_ptr<HistogramSamples>(new
SampleMap);
GetHistogramSamplesSinceCreation() now always return a non-null scoped_ptr.
There's still places like sql/connection_unittest.cc that checks for the
possibility of a null return value. I'm ok with the other tests that
ASSERT_TRUE(returned_scoper);

[vabr (Chromium), 2015-07-31 18:49:44]

vabr@chromium.org changed reviewers:
+ gbillock@chromium.org
- atwilson@chromium.org

[vabr (Chromium), 2015-07-31 18:49:45]

-atwilson, as his file is gone after rebase

+gbillock: Could you please review sql/connection_unittest.cc?

Thanks for review, Lei! Please have a look again, I addressed your comment.

Cheers,
Vaclav

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

https://codereview.chromium.org/1264123002/diff/60001/base/test/histogram_tes...
base/test/histogram_tester.cc:123: return scoped_ptr<HistogramSamples>(new
SampleMap);
On 2015/07/31 18:30:59, Lei Zhang wrote:
> GetHistogramSamplesSinceCreation() now always return a non-null scoped_ptr.
> There's still places like sql/connection_unittest.cc that checks for the
> possibility of a null return value. I'm ok with the other tests that
> ASSERT_TRUE(returned_scoper);

Good catch. I grepped for GetHistogramSamplesSinceCreation and verified that the
sql tests were the only place, where the EXPECT macros still allowed for samples
being null (and removed that). I left the places with ASSERTS untouched,
following your comment.

[Lei Zhang, 2015-07-31 21:45:02]

thestig@chromium.org changed reviewers:
+ shess@chromium.org

[Lei Zhang, 2015-07-31 21:45:02]

+shess

[Lei Zhang, 2015-07-31 21:48:20]

lgtm, please address the static initializers if possible.

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

https://codereview.chromium.org/1264123002/diff/70003/base/test/histogram_tes...
base/test/histogram_tester_unittest.cc:47: const std::string kHistogram =
Can this be static const char kHistogram[] ? Ditto with the kHistogramN
variables on line 17. These cause static initializers and would turn the bot red
if this was non-test code, or if we measured the static initializers for
base_unittests.

[Scott Hess - ex-Googler, 2015-08-01 15:08:16]

Definitely LGTM from sql/ side.  Maybe I should have complained about it (or
complained more).

[vabr (Chromium), 2015-08-03 07:33:37]

Thanks all!
Comment addressed, sending to the CQ.

@thestig -- please read my second comment, in case you want me to do the
suggested follow-up.

Cheers,
Vaclav

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

https://codereview.chromium.org/1264123002/diff/70003/base/test/histogram_tes...
base/test/histogram_tester_unittest.cc:47: const std::string kHistogram =
On 2015/07/31 21:48:20, Lei Zhang wrote:
> Can this be static const char kHistogram[] ?
Sure.

> Ditto with the kHistogramN
> variables on line 17. These cause static initializers and would turn the bot
red
> if this was non-test code, or if we measured the static initializers for
> base_unittests.
Yeah, I was wondering about those as well (unlike the string I added here, those
strings are indeed static). I did not change them, because I tend to focus CLs
on fixing a single problem, unless reviewers ask for more. Which you did, so
there you go. :)

https://codereview.chromium.org/1264123002/diff/110001/base/test/histogram_te...
File base/test/histogram_tester_unittest.cc (right):

https://codereview.chromium.org/1264123002/diff/110001/base/test/histogram_te...
base/test/histogram_tester_unittest.cc:17: static const char kHistogram1[] =
"Test1";
@thestig -- Here I followed you request literally, marking the constants as
static.

However, unlike on line 47, where "static" means that the constant is allocated
outside of the stack and spans the whole program execution, here it means that
the constants are not visible outside of this file (while non-stack allocation
and whole-execution-lifespan are implicit).

If I were writing the code as new, I would use anonymous namespace instead of
"static" here (and also move "namespace base" just before line 23).

I cannot assume your approval for that, so I will keep it as you literally told
me in this CL. But if you want me to do any follow-ups, I'll be happy to, just
let me know.

[vabr (Chromium), 2015-08-03 07:33:42]

The CQ bit was checked by vabr@chromium.org

[vabr (Chromium), 2015-08-03 07:33:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
atwilson@chromium.org, thestig@chromium.org, shess@chromium.org
Link to the patchset: https://codereview.chromium.org/1264123002/#ps110001
(title: "No static initializers")

[commit-bot: I haz the power, 2015-08-03 07:33:52]

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

[commit-bot: I haz the power, 2015-08-03 08:13:05]

Committed patchset #7 (id:110001)

[commit-bot: I haz the power, 2015-08-03 08:13:53]

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

[Lei Zhang, 2015-08-03 18:00:30]

https://codereview.chromium.org/1264123002/diff/110001/base/test/histogram_te...
File base/test/histogram_tester_unittest.cc (right):

https://codereview.chromium.org/1264123002/diff/110001/base/test/histogram_te...
base/test/histogram_tester_unittest.cc:17: static const char kHistogram1[] =
"Test1";
On 2015/08/03 07:33:37, vabr (Chromium) wrote:
> @thestig -- Here I followed you request literally, marking the constants as
> static.
> 
> However, unlike on line 47, where "static" means that the constant is
allocated
> outside of the stack and spans the whole program execution, here it means that
> the constants are not visible outside of this file (while non-stack allocation
> and whole-execution-lifespan are implicit).
> 
> If I were writing the code as new, I would use anonymous namespace instead of
> "static" here (and also move "namespace base" just before line 23).
> 
> I cannot assume your approval for that, so I will keep it as you literally
told
> me in this CL. But if you want me to do any follow-ups, I'll be happy to, just
> let me know.

An anonymous namespace would have been fine. I should have been more clear.

[vabr (Chromium), 2015-08-04 10:24:36]

On 2015/08/03 18:00:30, Lei Zhang wrote:
>
https://codereview.chromium.org/1264123002/diff/110001/base/test/histogram_te...
> File base/test/histogram_tester_unittest.cc (right):
> 
>
https://codereview.chromium.org/1264123002/diff/110001/base/test/histogram_te...
> base/test/histogram_tester_unittest.cc:17: static const char kHistogram1[] =
> "Test1";
> On 2015/08/03 07:33:37, vabr (Chromium) wrote:
> > @thestig -- Here I followed you request literally, marking the constants as
> > static.
> > 
> > However, unlike on line 47, where "static" means that the constant is
> allocated
> > outside of the stack and spans the whole program execution, here it means
that
> > the constants are not visible outside of this file (while non-stack
allocation
> > and whole-execution-lifespan are implicit).
> > 
> > If I were writing the code as new, I would use anonymous namespace instead
of
> > "static" here (and also move "namespace base" just before line 23).
> > 
> > I cannot assume your approval for that, so I will keep it as you literally
> told
> > me in this CL. But if you want me to do any follow-ups, I'll be happy to,
just
> > let me know.
> 
> An anonymous namespace would have been fine. I should have been more clear.

Thanks! I uploaded https://codereview.chromium.org/1271933002/ for that.