Fix: PrefsBackupInvalidChangeTest.Defaults on Mac Lion.

Fix: PrefsBackupInvalidChangeTest.Defaults on Mac Lion.

Make PrefsBackupInvalidChangeTest.Defaults independent of the Mac Lion
session startup pref migration.

BUG=134186
TEST=Enabled PrefsBackupInvalidChangeTest.Defaults


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

[marja, 2012-06-28 09:18:07]

Hi ivankr,

My change https://chromiumcodereview.appspot.com/10581013 broke
PrefsBackupInvalidChangeTest.Defaults on Mac Lion.

There are 2 things which made it break the test:
1. The test now crashes because the migration is done when the BaseSettingChange
is alive; BaseSettingChange gets the notification and BasePrefsChange::Observe
calls ProtectorService::DismissChange, but the change is not in the items_ list.

The migration code is hit 3 times. First it migrates ok, the 2nd time the pref
type is not DEFAULT (so no migration), and in the 3rd time, the pref type is
DEFAULT again, so it migrates again (-> notification -> crash).

2. If we're testing against a version that does the migration, the pref won't be
reset to DEFAULT, but to LAST.

Q: Is 1) it a sign of a real problem somewhere, or just a testing artifact?

For 2, I would just fake in the test that the profile is so recent that the
migration is not needed. Wdyt?

[Ivan Korotkov, 2012-06-28 09:53:50]

Hi Marja, thanks a lot for looking into this.

This is a testing artifact -- here BaseSettingChange instances are not owned by
ProtectorService, which is not supposed to happen IRL, so when ProtectorService
tries to dismiss them on pref change, it crashes. Your fix LGTM.

Given that you have a Lion instance for testing, could you please add a
funtional test to chrome/test/functional/protector.py, like the existing
ProtectorSessionStartupTest.testSessionStartupPrefMigrationFromHomepage and
friends; it would verify that when the  migration happens, protector service is
*not* triggered? Feel free to ask me for additional details.

http://codereview.chromium.org/10717005/diff/1/chrome/browser/protector/prefs...
File chrome/browser/protector/prefs_backup_invalid_change_unittest.cc (right):

http://codereview.chromium.org/10717005/diff/1/chrome/browser/protector/prefs...
chrome/browser/protector/prefs_backup_invalid_change_unittest.cc:35:
prefs->SetString(prefs::kProfileCreatedByVersion, "22.0.0.0.0");
Maybe move this to SetUp?

[marja, 2012-06-28 11:14:50]

Ok, I'll add a test checking that the migration doesn't interfere with protector
(but I'll commit this first).

What's the correct way to check that the protector was not invoked?

http://codereview.chromium.org/10717005/diff/1/chrome/browser/protector/prefs...
File chrome/browser/protector/prefs_backup_invalid_change_unittest.cc (right):

http://codereview.chromium.org/10717005/diff/1/chrome/browser/protector/prefs...
chrome/browser/protector/prefs_backup_invalid_change_unittest.cc:35:
prefs->SetString(prefs::kProfileCreatedByVersion, "22.0.0.0.0");
On 2012/06/28 09:53:50, Ivan Korotkov wrote:
> Maybe move this to SetUp?

Done.

[commit-bot: I haz the power, 2012-06-28 11:15:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/marja@chromium.org/10717005/6001

[Ivan Korotkov, 2012-06-28 11:29:19]

Look for self.assertFalse(self.GetProtectorState()['showing_change']) in
protector.py

[commit-bot: I haz the power, 2012-06-28 13:31:47]

Change committed as 144707