Add tests for version migration tesing in for LoginDatabase.

Add tests for version migration tesing in for LoginDatabase.

Unittest which updates database from version 1 to current version.

BUG=423828
R=gcasto@chromium.org

Committed: https://crrev.com/e0c7f1baf6c4a8adfe9a80d6bab9182de3c7d3a1
Cr-Commit-Position: refs/heads/master@{#310742}

[melandory, 2014-12-18 08:56:04]

melandory@chromium.org changed reviewers:
+ gcasto@chromium.org, mkwst@chromium.org

[melandory, 2014-12-18 08:56:05]

mkwst@chromium.org: Please review changes in 

gcasto@chromium.org: Please also take a look as I promised to show it to you. I
did something wrong and you wasn't marked as reviewer when I submitted CL
yesterday :(

[Mike West, 2014-12-18 09:43:44]

https://codereview.chromium.org/808173002/diff/20001/components/password_mana...
File components/password_manager/core/browser/login_database_unittest.cc
(right):

https://codereview.chromium.org/808173002/diff/20001/components/password_mana...
components/password_manager/core/browser/login_database_unittest.cc:1105:
PathService::Get(base::DIR_SOURCE_ROOT, &database_dump);
Why not do this as part of '::SetUp()'? Then you could just do
'database_dump_location_.AppendASCII(...)' in this method. That seems simpler.

https://codereview.chromium.org/808173002/diff/20001/components/password_mana...
components/password_manager/core/browser/login_database_unittest.cc:1163: //
Assert that the database was successfully opened and migrated.
I suppose this isn't really part of this change, but it's not clear that we're
actually validating migration. If migration fails, what would blow up here? It
looks like we'd just verify that we can intialize the database, but wouldn't we
be able to do that regardless of migration status?

[Mike West, 2014-12-18 09:44:16]

mkwst@chromium.org changed required reviewers:
+ gcasto@chromium.org

[Mike West, 2014-12-18 09:44:16]

Marking Garrett as required; he really needs to look at this before landing it.
I'm not very familiar with this code at all, unfortunately. :)

[Garrett Casto, 2014-12-18 19:17:22]

https://codereview.chromium.org/808173002/diff/20001/components/password_mana...
File components/password_manager/core/browser/login_database_unittest.cc
(right):

https://codereview.chromium.org/808173002/diff/20001/components/password_mana...
components/password_manager/core/browser/login_database_unittest.cc:1163: //
Assert that the database was successfully opened and migrated.
On 2014/12/18 09:43:44, Mike West wrote:
> I suppose this isn't really part of this change, but it's not clear that we're
> actually validating migration. If migration fails, what would blow up here? It
> looks like we'd just verify that we can intialize the database, but wouldn't
we
> be able to do that regardless of migration status?

If the database isn't at the current version we upgrade to the current version
in Init(). So this catches problems like syntax errors in the update (e.g.
https://crbug.com/423716).

https://codereview.chromium.org/808173002/diff/20001/components/password_mana...
components/password_manager/core/browser/login_database_unittest.cc:1179:
DestroyDatabase();
I think that it makes sense to verify that the final version can actually save
all the appropriate fields. So calling AddLogin() on a PasswordForm with all
it's fields filled and then calling GetLogins() will return a form that is equal
to the first form.

https://codereview.chromium.org/808173002/diff/20001/components/test/data/pas...
File components/test/data/password_manager/login_db_v8.sql (right):

https://codereview.chromium.org/808173002/diff/20001/components/test/data/pas...
components/test/data/password_manager/login_db_v8.sql:5: INSERT INTO "meta"
VALUES('version','8');
Unfortunately there are actually two possible v8 versions that clients could
have, one with use_additional_auth and one without (see crbug.com/423716). It
probably makes sense to test both versions update appropriately.

As a follow up it would be nice to actually get rid of use_additional_auth so
that this inconsistency goes away
(https://code.google.com/p/chromium/codesearch#chromium/src/components/passwor...).

[melandory, 2014-12-22 16:43:28]

https://codereview.chromium.org/808173002/diff/20001/components/password_mana...
File components/password_manager/core/browser/login_database_unittest.cc
(right):

https://codereview.chromium.org/808173002/diff/20001/components/password_mana...
components/password_manager/core/browser/login_database_unittest.cc:1105:
PathService::Get(base::DIR_SOURCE_ROOT, &database_dump);
On 2014/12/18 09:43:44, Mike West (OOO until Jan 7th) wrote:
> Why not do this as part of '::SetUp()'? Then you could just do
> 'database_dump_location_.AppendASCII(...)' in this method. That seems simpler.

Done.

https://codereview.chromium.org/808173002/diff/20001/components/password_mana...
components/password_manager/core/browser/login_database_unittest.cc:1163: //
Assert that the database was successfully opened and migrated.
On 2014/12/18 19:17:22, Garrett Casto (back Dec 30th) wrote:
> On 2014/12/18 09:43:44, Mike West wrote:
> > I suppose this isn't really part of this change, but it's not clear that
we're
> > actually validating migration. If migration fails, what would blow up here?
It
> > looks like we'd just verify that we can intialize the database, but wouldn't
> we
> > be able to do that regardless of migration status?
> 
> If the database isn't at the current version we upgrade to the current version
> in Init(). So this catches problems like syntax errors in the update (e.g.
> https://crbug.com/423716). 

I'll add explanation to comment.

https://codereview.chromium.org/808173002/diff/20001/components/password_mana...
components/password_manager/core/browser/login_database_unittest.cc:1179:
DestroyDatabase();
On 2014/12/18 19:17:22, Garrett Casto (back Dec 30th) wrote:
> I think that it makes sense to verify that the final version can actually save
> all the appropriate fields. So calling AddLogin() on a PasswordForm with all
> it's fields filled and then calling GetLogins() will return a form that is
equal
> to the first form.

Done.

https://codereview.chromium.org/808173002/diff/20001/components/test/data/pas...
File components/test/data/password_manager/login_db_v8.sql (right):

https://codereview.chromium.org/808173002/diff/20001/components/test/data/pas...
components/test/data/password_manager/login_db_v8.sql:5: INSERT INTO "meta"
VALUES('version','8');
On 2014/12/18 19:17:22, Garrett Casto (back Dec 30th) wrote:
> Unfortunately there are actually two possible v8 versions that clients could
> have, one with use_additional_auth and one without (see crbug.com/423716). It
> probably makes sense to test both versions update appropriately.
> 
> As a follow up it would be nice to actually get rid of use_additional_auth so
> that this inconsistency goes away
>
(https://code.google.com/p/chromium/codesearch#chromium/src/components/passwor...).

I prefer following solution: leave it like this now and I'll create a follow up
CL, which removes inconsistency.

[melandory, 2015-01-05 10:59:44]

Patchset #5 (id:80001) has been deleted

[melandory, 2015-01-05 11:00:00]

Patchset #1 (id:1) has been deleted

[melandory, 2015-01-05 11:00:52]

https://codereview.chromium.org/808173002/diff/20001/components/test/data/pas...
File components/test/data/password_manager/login_db_v8.sql (right):

https://codereview.chromium.org/808173002/diff/20001/components/test/data/pas...
components/test/data/password_manager/login_db_v8.sql:5: INSERT INTO "meta"
VALUES('version','8');
On 2014/12/22 16:43:27, melandory wrote:
> On 2014/12/18 19:17:22, Garrett Casto (back Dec 30th) wrote:
> > Unfortunately there are actually two possible v8 versions that clients could
> > have, one with use_additional_auth and one without (see crbug.com/423716).
It
> > probably makes sense to test both versions update appropriately.
> > 
> > As a follow up it would be nice to actually get rid of use_additional_auth
so
> > that this inconsistency goes away
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/components/passwor...).
> 
> I prefer following solution: leave it like this now and I'll create a follow
up
> CL, which removes inconsistency.

https://codereview.chromium.org/818443004/

[Garrett Casto, 2015-01-06 21:37:29]

lgtm

[melandory, 2015-01-08 13:46:14]

The CQ bit was checked by melandory@chromium.org

[commit-bot: I haz the power, 2015-01-08 13:47:19]

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

[commit-bot: I haz the power, 2015-01-08 15:17:16]

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

[commit-bot: I haz the power, 2015-01-08 15:17:17]

Try jobs failed on following builders:
  linux_chromium_asan_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[melandory, 2015-01-08 16:13:11]

Patchset #5 (id:110001) has been deleted

[melandory, 2015-01-08 17:03:16]

Patchset #5 (id:130001) has been deleted

[melandory, 2015-01-09 08:50:49]

The CQ bit was checked by melandory@chromium.org

[commit-bot: I haz the power, 2015-01-09 08:51:35]

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

[commit-bot: I haz the power, 2015-01-09 08:52:39]

Committed patchset #6 (id:170001)

[commit-bot: I haz the power, 2015-01-09 08:53:22]

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