[sql] Initialize StatisticsRecorder in SQLTestSuite.

[sql] Initialize StatisticsRecorder in SQLTestSuite.

Histograms created using the histogram macros are stored in per-site global storage, so any histogram recorded before StatisticsRecorder::Initialize() are unregistered until the process exits.  HistogramTester relies on StatisticsRecorder, so depending on whether HistogramTester is created before or after a particular histogram macro is used, it may or may not be able to find the corresponding histogram.

Pull StatisticsRecorder::Initialize() into the test-suite initialization, which should run before any code under test.

BUG=658070
Committed: https://crrev.com/1cf87f2750870763cfd8bc4230bd4c0ccc91b7e0
Cr-Commit-Position: refs/heads/master@{#427508}

[Scott Hess - ex-Googler, 2016-10-21 05:12:27]

The CQ bit was checked by shess@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-21 05:12:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-21 05:45:37]

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

[commit-bot: I haz the power, 2016-10-21 05:45:37]

Dry run: This issue passed the CQ dry run.

[Scott Hess - ex-Googler, 2016-10-21 16:38:50]

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

[Scott Hess - ex-Googler, 2016-10-21 16:38:50]

Should be pretty much rubberstamp.

[Ilya Sherman, 2016-10-21 20:28:38]

Interesting... makes sense that this could cause issues, though I don't quite
see how it aligns with the behavior you were seeing.  Anyway, LGTM.

[Scott Hess - ex-Googler, 2016-10-24 23:05:49]

The CQ bit was checked by shess@chromium.org

[commit-bot: I haz the power, 2016-10-24 23:06:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Scott Hess - ex-Googler, 2016-10-25 00:04:43]

On 2016/10/21 20:28:38, Ilya Sherman wrote:
> Interesting... makes sense that this could cause issues, though I don't quite
> see how it aligns with the behavior you were seeing.  Anyway, LGTM.

If the base::HistogramTester() constructor is the first one in the process to
call base::StatisticsRecorder::Initialize(), then any
UMA_HISTOGRAM_ENUMERATION() type calls before then will have already cached a
histogram that isn't registered globally in their local static storage.

When the specific test is run with --gtest_filter=SQLRecoveryTest.AttachFailure,
the histogram tester is constructed before the target histogram is first logged,
so it works right.

When given --gtest_filter=SQLRecoveryTest.*, one of the earlier tests hits the
histogram before this test, so it caches an unregistered histogram, and things
don't work.

When run --single-process-tests, SQLConnectionTest.* run before
SQLRecoveryTest.* in a single process, and SQLConnectionTest calls
base::StatisticsRecorder::Initialize() in SetUp(), so the recovery tests work
right.

When given no parameters, the coordinator allocates tests to a pool of runners,
so the results depend on which tests share test runner workers.  Depending on
what other tests there are, this test could fail.  But the coordinator reruns
failed tests specifically, so based on the specific case it succeeds.

tl;dr: Nothing in the test-running system seems to call
base::StatisticsRecorder::Initialize().  The test runners for components/,
content/, ios/, and net/ all call StatisticsRecorder::Initialize().

[Scott Hess - ex-Googler, 2016-10-25 00:04:51]

The CQ bit was unchecked by shess@chromium.org

[Scott Hess - ex-Googler, 2016-10-25 00:06:43]

On 2016/10/25 00:04:43, Scott Hess wrote:
> tl;dr: Nothing in the test-running system seems to call
> base::StatisticsRecorder::Initialize().  The test runners for components/,
> content/, ios/, and net/ all call StatisticsRecorder::Initialize().

Obvious conclusion: I'm going to abandon this CL in favor of one which pulls the
initialization up into the test suite.  I'll be back once I figure out what
various pieces of Chromium code do.

[Ilya Sherman, 2016-10-25 01:11:13]

On 2016/10/25 00:04:43, Scott Hess wrote:
> On 2016/10/21 20:28:38, Ilya Sherman wrote:
> > Interesting... makes sense that this could cause issues, though I don't
quite
> > see how it aligns with the behavior you were seeing.  Anyway, LGTM.
> 
> If the base::HistogramTester() constructor is the first one in the process to
> call base::StatisticsRecorder::Initialize(), then any
> UMA_HISTOGRAM_ENUMERATION() type calls before then will have already cached a
> histogram that isn't registered globally in their local static storage.
> 
> When the specific test is run with
--gtest_filter=SQLRecoveryTest.AttachFailure,
> the histogram tester is constructed before the target histogram is first
logged,
> so it works right.
> 
> When given --gtest_filter=SQLRecoveryTest.*, one of the earlier tests hits the
> histogram before this test, so it caches an unregistered histogram, and things
> don't work.
> 
> When run --single-process-tests, SQLConnectionTest.* run before
> SQLRecoveryTest.* in a single process, and SQLConnectionTest calls
> base::StatisticsRecorder::Initialize() in SetUp(), so the recovery tests work
> right.
> 
> When given no parameters, the coordinator allocates tests to a pool of
runners,
> so the results depend on which tests share test runner workers.  Depending on
> what other tests there are, this test could fail.  But the coordinator reruns
> failed tests specifically, so based on the specific case it succeeds.
> 
> tl;dr: Nothing in the test-running system seems to call
> base::StatisticsRecorder::Initialize().  The test runners for components/,
> content/, ios/, and net/ all call StatisticsRecorder::Initialize().

Ah, I see, that does make sense.  Thanks, and thanks for investigating how to
fix this for the whole test suite!

[Scott Hess - ex-Googler, 2016-10-25 15:13:57]

Pull fix to SQLTestSuite.

[Scott Hess - ex-Googler, 2016-10-25 15:14:27]

The CQ bit was checked by shess@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-25 15:14:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Scott Hess - ex-Googler, 2016-10-25 15:25:14]

Description was changed from

==========
[sql] Make SQLRecoverTest.AttachDatabase not flaky.

In some situations, the test would reliably fail then succeed on
retry.  Since the histogram macros keep static storage, in some cases
it could be setup with a statistics recorder the test could see, in
other cases it was setup with one that the test wasn't seeing.  Setup
the recorder in SetUp() to make it consistent.

BUG=658070
==========

to

==========
[sql] Initialize StatisticsRecorder in SQLTestSuite.

Histograms created using the histogram macros are stored in per-site global
storage, so any histogram recorded before StatisticsRecorder::Initialize() are
unregistered until the process exits.  HistogramTester relies on
StatisticsRecorder, so depending on whether HistogramTester is created before or
after a particular histogram macro is used, it may or may not be able to find
the corresponding histogram.

Pull StatisticsRecorder::Initialize() into the test-suite initialization, which
should run before any code under test.

BUG=658070
==========

[commit-bot: I haz the power, 2016-10-25 15:28:49]

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

[commit-bot: I haz the power, 2016-10-25 15:28:50]

Dry run: This issue passed the CQ dry run.

[Scott Hess - ex-Googler, 2016-10-25 15:32:41]

On 2016/10/25 01:11:13, Ilya Sherman wrote:
> Ah, I see, that does make sense.  Thanks, and thanks for investigating how to
> fix this for the whole test suite!

I'm not brave enough to directly pull this into the base:: test suite - does
that seem like a good idea?  It seems subtle to me, but I can't really think of
any code which would work wrong if histograms worked correctly.  I can think of
how test code might organically grown around an assumption that histograms won't
fire, but since histogram consumers are mostly not in Chromium, it's probably
not common, and AFAICT performance-wise it shouldn't matter (the histogram is
there, just not registered globally).

Anyhow, PTAL at the new version.  The SQLConnectionTest change to an alias is
odd, it's because Mojo needs an alternate hierarchy, so most of the real guts
are elsewhere.  Having it different just to hold an arbitrary callback seemed
out of step.

[Ilya Sherman, 2016-10-25 21:03:51]

On 2016/10/25 15:32:41, Scott Hess wrote:
> On 2016/10/25 01:11:13, Ilya Sherman wrote:
> > Ah, I see, that does make sense.  Thanks, and thanks for investigating how
to
> > fix this for the whole test suite!
> 
> I'm not brave enough to directly pull this into the base:: test suite - does
> that seem like a good idea?  It seems subtle to me, but I can't really think
of
> any code which would work wrong if histograms worked correctly.  I can think
of
> how test code might organically grown around an assumption that histograms
won't
> fire, but since histogram consumers are mostly not in Chromium, it's probably
> not common, and AFAICT performance-wise it shouldn't matter (the histogram is
> there, just not registered globally).

FWIW, if all of our other test suites have it, I don't see much harm to pulling
into the base:: test suite.

> Anyhow, PTAL at the new version.  The SQLConnectionTest change to an alias is
> odd, it's because Mojo needs an alternate hierarchy, so most of the real guts
> are elsewhere.  Having it different just to hold an arbitrary callback seemed
> out of step.

LGTM

[Scott Hess - ex-Googler, 2016-10-25 22:09:06]

The CQ bit was checked by shess@chromium.org

[commit-bot: I haz the power, 2016-10-25 22:09:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-25 22:18:56]

Description was changed from

==========
[sql] Initialize StatisticsRecorder in SQLTestSuite.

Histograms created using the histogram macros are stored in per-site global
storage, so any histogram recorded before StatisticsRecorder::Initialize() are
unregistered until the process exits.  HistogramTester relies on
StatisticsRecorder, so depending on whether HistogramTester is created before or
after a particular histogram macro is used, it may or may not be able to find
the corresponding histogram.

Pull StatisticsRecorder::Initialize() into the test-suite initialization, which
should run before any code under test.

BUG=658070
==========

to

==========
[sql] Initialize StatisticsRecorder in SQLTestSuite.

Histograms created using the histogram macros are stored in per-site global
storage, so any histogram recorded before StatisticsRecorder::Initialize() are
unregistered until the process exits.  HistogramTester relies on
StatisticsRecorder, so depending on whether HistogramTester is created before or
after a particular histogram macro is used, it may or may not be able to find
the corresponding histogram.

Pull StatisticsRecorder::Initialize() into the test-suite initialization, which
should run before any code under test.

BUG=658070
==========

[commit-bot: I haz the power, 2016-10-25 22:18:58]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-25 22:23:05]

Description was changed from

==========
[sql] Initialize StatisticsRecorder in SQLTestSuite.

Histograms created using the histogram macros are stored in per-site global
storage, so any histogram recorded before StatisticsRecorder::Initialize() are
unregistered until the process exits.  HistogramTester relies on
StatisticsRecorder, so depending on whether HistogramTester is created before or
after a particular histogram macro is used, it may or may not be able to find
the corresponding histogram.

Pull StatisticsRecorder::Initialize() into the test-suite initialization, which
should run before any code under test.

BUG=658070
==========

to

==========
[sql] Initialize StatisticsRecorder in SQLTestSuite.

Histograms created using the histogram macros are stored in per-site global
storage, so any histogram recorded before StatisticsRecorder::Initialize() are
unregistered until the process exits.  HistogramTester relies on
StatisticsRecorder, so depending on whether HistogramTester is created before or
after a particular histogram macro is used, it may or may not be able to find
the corresponding histogram.

Pull StatisticsRecorder::Initialize() into the test-suite initialization, which
should run before any code under test.

BUG=658070

Committed: https://crrev.com/1cf87f2750870763cfd8bc4230bd4c0ccc91b7e0
Cr-Commit-Position: refs/heads/master@{#427508}
==========

[commit-bot: I haz the power, 2016-10-25 22:23:06]

Patchset 2 (id:??) landed as
https://crrev.com/1cf87f2750870763cfd8bc4230bd4c0ccc91b7e0
Cr-Commit-Position: refs/heads/master@{#427508}