Add stricter tests for RapporService::LoadSecret

Add stricter tests for RapporService::LoadSecret

BUG=448269
Committed: https://crrev.com/2a1a54720a4e78e912200444016a51009196bbf0
Cr-Commit-Position: refs/heads/master@{#311591}

[Alexei Svitkine (slow), 2015-01-10 19:56:27]

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

[Alexei Svitkine (slow), 2015-01-10 19:56:28]

Is it possible to write a unit test for this, so that this sort of bug doesn't
slip in at some point in the future?

[Steven Holte, 2015-01-10 21:28:04]

On 2015/01/10 19:56:28, Alexei Svitkine wrote:
> Is it possible to write a unit test for this, so that this sort of bug doesn't
> slip in at some point in the future?

We probably can improve the testing, but unfortunately, the test that exists
just doesn't catch this because it's state isn't quite the same as the real
RapporService state, since it's already loaded a different secret.

My inclination is to change this method to a static function which would remove
the unexpected variable from the scope, and insure that there isn't an outside
factor influencing the test.

[Alexei Svitkine (slow), 2015-01-12 16:46:25]

How about the following unit test for LoadSecret() function:

Set some bad data in the testing prefs that the test rappor service is created
for then call LoadSecret() from the test and observe what happens?

If it's hard to inspect the state, we could actually add a histogram for the
different cases and using HistogramTester to see which values were logged. This
would also give insight into what's happening in the wild.

[Steven Holte, 2015-01-12 23:43:55]

On 2015/01/12 16:46:25, Alexei Svitkine wrote:
> How about the following unit test for LoadSecret() function:
> 
> Set some bad data in the testing prefs that the test rappor service is created
> for then call LoadSecret() from the test and observe what happens?
> 
> If it's hard to inspect the state, we could actually add a histogram for the
> different cases and using HistogramTester to see which values were logged.
This
> would also give insight into what's happening in the wild.

I've combined the approaches, and also just moved these methods into their own
file to help isolate them from further refactoring.

[Steven Holte, 2015-01-13 04:49:59]

I've created https://codereview.chromium.org/848823002/
to keep just the minimal fix.  I think we should commit that before moving
forward with adding tests here.

[Alexei Svitkine (slow), 2015-01-13 16:44:30]

I'm not a fan of this structure because it's missing public API (register prefs)
and implementation details (loading secret) functions in the same header file.

Now, you're allowing non-rappor code load its secret, etc - or at least the
header doesn't make it clear what's internal and what's not.

I think I still prefer them to be private/protected members of RapporService
(but can be static)...

[Steven Holte, 2015-01-13 23:14:08]

On 2015/01/13 16:44:30, Alexei Svitkine wrote:
> I'm not a fan of this structure because it's missing public API (register
prefs)
> and implementation details (loading secret) functions in the same header file.
> 
> Now, you're allowing non-rappor code load its secret, etc - or at least the
> header doesn't make it clear what's internal and what's not.
> 
> I think I still prefer them to be private/protected members of RapporService
> (but can be static)...

How about a rappor::internal namespace?  Also added a public static function for
RegisterPrefs so that the full public interface is through RapporService.

[Alexei Svitkine (slow), 2015-01-14 17:29:06]

internal namespace sgtm, some comments below

https://codereview.chromium.org/845863002/diff/40001/components/rappor/rappor...
File components/rappor/rappor_prefs_unittest.cc (right):

https://codereview.chromium.org/845863002/diff/40001/components/rappor/rappor...
components/rappor/rappor_prefs_unittest.cc:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
2015 now.

https://codereview.chromium.org/845863002/diff/40001/components/rappor/rappor...
components/rappor/rappor_prefs_unittest.cc:22: void
StoreSecret(TestingPrefServiceSimple* test_prefs,
Nit: Modifiable params (i.e. test_prefs) should be last.

https://codereview.chromium.org/845863002/diff/40001/components/rappor/rappor...
components/rappor/rappor_prefs_unittest.cc:41: base::HistogramTester histogram;
Nit: tester or histogram_tester, it's not a histogram.

https://codereview.chromium.org/845863002/diff/40001/components/rappor/rappor...
components/rappor/rappor_prefs_unittest.cc:43:
RegisterPrefs(test_prefs.registry());
All these tests begin the same. Please make a test harness class so you can do
make the tester and prefs instance variables of it and the RegisterPrefs() call
in the ctor.

https://codereview.chromium.org/845863002/diff/40001/components/rappor/rappor...
File components/rappor/rappor_service.cc (right):

https://codereview.chromium.org/845863002/diff/40001/components/rappor/rappor...
components/rappor/rappor_service.cc:106:
rappor::internal::LoadCohort(pref_service_),
Nit: No rappor:: prefix.

https://codereview.chromium.org/845863002/diff/40001/components/rappor/rappor...
components/rappor/rappor_service.cc:139:
rappor::internal::RegisterPrefs(registry);
Ditto.

[Steven Holte, 2015-01-14 21:12:17]

https://codereview.chromium.org/845863002/diff/40001/components/rappor/rappor...
File components/rappor/rappor_prefs_unittest.cc (right):

https://codereview.chromium.org/845863002/diff/40001/components/rappor/rappor...
components/rappor/rappor_prefs_unittest.cc:1: // Copyright 2014 The Chromium
Authors. All rights reserved.
On 2015/01/14 17:29:06, Alexei Svitkine wrote:
> 2015 now.

Done.

https://codereview.chromium.org/845863002/diff/40001/components/rappor/rappor...
components/rappor/rappor_prefs_unittest.cc:22: void
StoreSecret(TestingPrefServiceSimple* test_prefs,
On 2015/01/14 17:29:05, Alexei Svitkine wrote:
> Nit: Modifiable params (i.e. test_prefs) should be last.

Done.

https://codereview.chromium.org/845863002/diff/40001/components/rappor/rappor...
components/rappor/rappor_prefs_unittest.cc:41: base::HistogramTester histogram;
On 2015/01/14 17:29:05, Alexei Svitkine wrote:
> Nit: tester or histogram_tester, it's not a histogram.

Done.

https://codereview.chromium.org/845863002/diff/40001/components/rappor/rappor...
components/rappor/rappor_prefs_unittest.cc:43:
RegisterPrefs(test_prefs.registry());
On 2015/01/14 17:29:05, Alexei Svitkine wrote:
> All these tests begin the same. Please make a test harness class so you can do
> make the tester and prefs instance variables of it and the RegisterPrefs()
call
> in the ctor.

Done.

https://codereview.chromium.org/845863002/diff/40001/components/rappor/rappor...
File components/rappor/rappor_service.cc (right):

https://codereview.chromium.org/845863002/diff/40001/components/rappor/rappor...
components/rappor/rappor_service.cc:106:
rappor::internal::LoadCohort(pref_service_),
On 2015/01/14 17:29:06, Alexei Svitkine wrote:
> Nit: No rappor:: prefix.

Done.

https://codereview.chromium.org/845863002/diff/40001/components/rappor/rappor...
components/rappor/rappor_service.cc:139:
rappor::internal::RegisterPrefs(registry);
On 2015/01/14 17:29:06, Alexei Svitkine wrote:
> Ditto.

Done.

[Alexei Svitkine (slow), 2015-01-14 21:45:38]

lgtm

https://codereview.chromium.org/845863002/diff/60001/components/rappor/rappor...
File components/rappor/rappor_prefs.h (right):

https://codereview.chromium.org/845863002/diff/60001/components/rappor/rappor...
components/rappor/rappor_prefs.h:36: // Retrieves the value for secret_ from
preferences, generating it if doesn't
Nit: What's |secret_|? Update comment.

https://codereview.chromium.org/845863002/diff/60001/components/rappor/rappor...
File components/rappor/rappor_prefs_unittest.cc (right):

https://codereview.chromium.org/845863002/diff/60001/components/rappor/rappor...
components/rappor/rappor_prefs_unittest.cc:47: base::HistogramTester tester;
Nit: use _ for member variables.

[Steven Holte, 2015-01-14 23:25:12]

https://codereview.chromium.org/845863002/diff/60001/components/rappor/rappor...
File components/rappor/rappor_prefs.h (right):

https://codereview.chromium.org/845863002/diff/60001/components/rappor/rappor...
components/rappor/rappor_prefs.h:36: // Retrieves the value for secret_ from
preferences, generating it if doesn't
On 2015/01/14 21:45:38, Alexei Svitkine wrote:
> Nit: What's |secret_|? Update comment.

Done.

https://codereview.chromium.org/845863002/diff/60001/components/rappor/rappor...
File components/rappor/rappor_prefs_unittest.cc (right):

https://codereview.chromium.org/845863002/diff/60001/components/rappor/rappor...
components/rappor/rappor_prefs_unittest.cc:47: base::HistogramTester tester;
On 2015/01/14 21:45:38, Alexei Svitkine wrote:
> Nit: use _ for member variables.

Done.

[Steven Holte, 2015-01-14 23:42:18]

The CQ bit was checked by holte@chromium.org

[commit-bot: I haz the power, 2015-01-14 23:43:14]

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

[commit-bot: I haz the power, 2015-01-15 00:43:47]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-01-15 00:45:41]

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