Added tests for storing and loading seed data to and from prefs.

Added tests for storing and loading seed data to and from prefs. 


BUG=136396
TEST=New VariationsServiceTest.StoreSeed and VariationsServiceTest.LoadSeed tests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=145980

[SteveT, 2012-07-09 19:23:25]

Looking good. A few small things inline...

http://r879-cab51e0eb6a4-rogerta.chromiumcodereview-hr.appspot.com/10764005/d...
File chrome/browser/metrics/variations_service.h (right):

http://r879-cab51e0eb6a4-rogerta.chromiumcodereview-hr.appspot.com/10764005/d...
chrome/browser/metrics/variations_service.h:61:
FRIEND_TEST_ALL_PREFIXES(VariationsServiceTest, StoreSeed);
Make sure these two are in alpha order.

http://r879-cab51e0eb6a4-rogerta.chromiumcodereview-hr.appspot.com/10764005/d...
File chrome/browser/metrics/variations_service_unittest.cc (right):

http://r879-cab51e0eb6a4-rogerta.chromiumcodereview-hr.appspot.com/10764005/d...
chrome/browser/metrics/variations_service_unittest.cc:26: void
CreateTestSeed(chrome_variations::TrialsSeed* seed) {
nit: DCHECK(seed);

http://r879-cab51e0eb6a4-rogerta.chromiumcodereview-hr.appspot.com/10764005/d...
chrome/browser/metrics/variations_service_unittest.cc:26: void
CreateTestSeed(chrome_variations::TrialsSeed* seed) {
You currently don't use these seeds more than once in a test. Would it be
cleaner to just have this function return a chrome_variations::TrialSeed object
rather than passing around pointers?

If not, then maybe PopulateTestSeed is a better name.

http://r879-cab51e0eb6a4-rogerta.chromiumcodereview-hr.appspot.com/10764005/d...
chrome/browser/metrics/variations_service_unittest.cc:379:
variations_service.StoreSeedData("this should fail", now, &pref_service);
Optional: Might be nice to have StoreSeedData return a bool indicating success,
so we can test that value here. On the other hand, your check below may be
sufficient in indicating as such.

http://r879-cab51e0eb6a4-rogerta.chromiumcodereview-hr.appspot.com/10764005/d...
chrome/browser/metrics/variations_service_unittest.cc:416:
pref_service.SetString(prefs::kVariationsSeed, "this should fail");
Would this be useful to ensure that we're actually doing work here?
EXPECT_FALSE(
    pref_service.FindPreference(prefs::kVariationsSeed)->IsDefaultValue());

[jwd, 2012-07-09 21:08:13]

http://codereview.chromium.org/10764005/diff/3001/chrome/browser/metrics/vari...
File chrome/browser/metrics/variations_service.h (right):

http://codereview.chromium.org/10764005/diff/3001/chrome/browser/metrics/vari...
chrome/browser/metrics/variations_service.h:61:
FRIEND_TEST_ALL_PREFIXES(VariationsServiceTest, StoreSeed);
On 2012/07/09 19:23:26, SteveT wrote:
> Make sure these two are in alpha order.

Done.

http://codereview.chromium.org/10764005/diff/3001/chrome/browser/metrics/vari...
File chrome/browser/metrics/variations_service_unittest.cc (right):

http://codereview.chromium.org/10764005/diff/3001/chrome/browser/metrics/vari...
chrome/browser/metrics/variations_service_unittest.cc:26: void
CreateTestSeed(chrome_variations::TrialsSeed* seed) {
On 2012/07/09 19:23:26, SteveT wrote:
> You currently don't use these seeds more than once in a test. Would it be
> cleaner to just have this function return a chrome_variations::TrialSeed
object
> rather than passing around pointers?
> 
> If not, then maybe PopulateTestSeed is a better name.

Done.

http://codereview.chromium.org/10764005/diff/3001/chrome/browser/metrics/vari...
chrome/browser/metrics/variations_service_unittest.cc:26: void
CreateTestSeed(chrome_variations::TrialsSeed* seed) {
On 2012/07/09 19:23:26, SteveT wrote:
> nit: DCHECK(seed);

Done.

http://codereview.chromium.org/10764005/diff/3001/chrome/browser/metrics/vari...
chrome/browser/metrics/variations_service_unittest.cc:379:
variations_service.StoreSeedData("this should fail", now, &pref_service);
On 2012/07/09 19:23:26, SteveT wrote:
> Optional: Might be nice to have StoreSeedData return a bool indicating
success,
> so we can test that value here. On the other hand, your check below may be
> sufficient in indicating as such.

Done.

http://codereview.chromium.org/10764005/diff/3001/chrome/browser/metrics/vari...
chrome/browser/metrics/variations_service_unittest.cc:416:
pref_service.SetString(prefs::kVariationsSeed, "this should fail");
On 2012/07/09 19:23:26, SteveT wrote:
> Would this be useful to ensure that we're actually doing work here?
> EXPECT_FALSE(
>     pref_service.FindPreference(prefs::kVariationsSeed)->IsDefaultValue());

Done. Might as well.

[SteveT, 2012-07-09 21:14:02]

Cool. LGTM.

[commit-bot: I haz the power, 2012-07-10 16:56:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jwd@chromium.org/10764005/16001

[commit-bot: I haz the power, 2012-07-10 17:49:32]

Try job failure for 10764005-16001 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-07-10 18:14:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jwd@chromium.org/10764005/16001

[commit-bot: I haz the power, 2012-07-10 19:09:35]

Try job failure for 10764005-16001 (retry) (retry) on linux_rel for step
"interactive_ui_tests".
It's a second try, previously, step "interactive_ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-07-10 19:37:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jwd@chromium.org/10764005/16001

[commit-bot: I haz the power, 2012-07-10 21:38:09]

Try job failure for 10764005-16001 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-07-10 22:08:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jwd@chromium.org/10764005/16001

[commit-bot: I haz the power, 2012-07-10 23:01:45]

Change committed as 145980