Migration unit tests to web database for missing images on the search ballot

Migration unit tests to web database for missing images on the search ballot

Adds migration unit tests to web database for missing images on the search ballot.  This is follow-up from review http://codereview.chromium.org/3189004.

BUG=52452, 50699, 10913
TEST=WebDatabaseMigrationTest.MigrateVersion22ToCurrent, WebDatabaseMigrationTest.MigrateVersion22CorruptToCurrent, WebDatabaseMigrationTest.MigrateVersion24ToCurrent

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

[sky, 2010-09-01 16:42:49]

Rather than duplicating all the code for creating the schema couldn't you copy
over a Web DB file in the data directory and open that? The test could be
flexible enough that it tries opening all web db files in a particular
directory. That way it's trivial to add migration checking when we rev, just
check in an existing web db file.

[dhollowa, 2010-09-01 16:51:57]

On 2010/09/01 16:42:49, sky wrote:
> Rather than duplicating all the code for creating the schema couldn't you copy
> over a Web DB file in the data directory and open that? The test could be
> flexible enough that it tries opening all web db files in a particular
> directory. That way it's trivial to add migration checking when we rev, just
> check in an existing web db file.

I considered that but felt that having the SQL dumps right there in the code
seemed more readable and debuggable.  And it is pretty simple to create a dump
and copy paste into the code.  Thoughts?

[sky, 2010-09-01 16:58:33]

That's true. One minor suggestion then.

http://codereview.chromium.org/3295002/diff/1/2
File chrome/browser/webdata/web_database.h (right):

http://codereview.chromium.org/3295002/diff/1/2#newcode311
chrome/browser/webdata/web_database.h:311:
FRIEND_TEST_ALL_PREFIXES(WebDatabaseMigrationTest, MigrateVersion22ToCurrent);
Is the only thing these tests need GetVersionNumber? If so, could you make it
public and nuke the friends?

[dhollowa, 2010-09-01 17:17:33]

http://codereview.chromium.org/3295002/diff/1/2
File chrome/browser/webdata/web_database.h (right):

http://codereview.chromium.org/3295002/diff/1/2#newcode311
chrome/browser/webdata/web_database.h:311:
FRIEND_TEST_ALL_PREFIXES(WebDatabaseMigrationTest, MigrateVersion22ToCurrent);
On 2010/09/01 16:58:33, sky wrote:
> Is the only thing these tests need GetVersionNumber? If so, could you make it
> public and nuke the friends?

Done.

[sky, 2010-09-01 17:24:23]

LGTM

[Scott Hess - ex-Googler, 2010-09-01 18:22:05]

thanks for tackling this!  Overall LGTM, but I think we should tighten it up a
little.

http://codereview.chromium.org/3295002/diff/7001/8002
File chrome/browser/webdata/web_database.h (right):

http://codereview.chromium.org/3295002/diff/7001/8002#newcode302
chrome/browser/webdata/web_database.h:302: int GetVersionNumber();
I'd be comfortable just querying the value directly from the database.  The new
tests really are concerned with whether what's on disk matches expectations,
after all.

http://codereview.chromium.org/3295002/diff/7001/8003
File chrome/browser/webdata/web_database_unittest.cc (right):

http://codereview.chromium.org/3295002/diff/7001/8003#newcode1804
chrome/browser/webdata/web_database_unittest.cc:1804: ASSERT_LE(23,
db.GetVersionNumber());
This needs to be stronger - this wouldn't catch the case where the version
number went to 23, but didn't make it to 24, and nobody wrote a new test for
migrating to 24.

My suggestion would be to hard-code a duplicate of the current version number as
a constant in this file, and have every test assert that it arrived at that
version number.  Put in a baseline test that an empty database gets the expected
version number, so that you can provide a helpful failure message when someone
versions things.  And a comment about requiring a new migration test.

[dhollowa, 2010-09-01 18:37:39]

http://codereview.chromium.org/3295002/diff/7001/8003
File chrome/browser/webdata/web_database_unittest.cc (right):

http://codereview.chromium.org/3295002/diff/7001/8003#newcode1804
chrome/browser/webdata/web_database_unittest.cc:1804: ASSERT_LE(23,
db.GetVersionNumber());
On 2010/09/01 18:22:05, shess wrote:
> This needs to be stronger - this wouldn't catch the case where the version
> number went to 23, but didn't make it to 24, and nobody wrote a new test for
> migrating to 24.
> 
> My suggestion would be to hard-code a duplicate of the current version number
as
> a constant in this file, and have every test assert that it arrived at that
> version number.  Put in a baseline test that an empty database gets the
expected
> version number, so that you can provide a helpful failure message when someone
> versions things.  And a comment about requiring a new migration test.

I think I understand...  You're saying that if someone adds a migration in the
WebDatabase::MigrateOldVersionsAsNeeded() and bumps the value of
kCurrentVersionNumber, then all of these unit tests should fail.  Thus requiring
that these unit tests would need updating to the new kCurrentVersionNumber
value?

Adding to this file:
  const int kCurrentTestedVersionNumber = 25;

and changing checks to:
  ASSERT_EQ(kCurrentTestedVersionNumber, db.GetVersionNumber());

Yes?

[Scott Hess - ex-Googler, 2010-09-01 18:42:56]

On 2010/09/01 18:37:39, dhollowa wrote:
> http://codereview.chromium.org/3295002/diff/7001/8003
> File chrome/browser/webdata/web_database_unittest.cc (right):
> 
> http://codereview.chromium.org/3295002/diff/7001/8003#newcode1804
> chrome/browser/webdata/web_database_unittest.cc:1804: ASSERT_LE(23,
> db.GetVersionNumber());
> On 2010/09/01 18:22:05, shess wrote:
> > This needs to be stronger - this wouldn't catch the case where the version
> > number went to 23, but didn't make it to 24, and nobody wrote a new test for
> > migrating to 24.
> > 
> > My suggestion would be to hard-code a duplicate of the current version
number
> as
> > a constant in this file, and have every test assert that it arrived at that
> > version number.  Put in a baseline test that an empty database gets the
> expected
> > version number, so that you can provide a helpful failure message when
someone
> > versions things.  And a comment about requiring a new migration test.
> 
> I think I understand...  You're saying that if someone adds a migration in the
> WebDatabase::MigrateOldVersionsAsNeeded() and bumps the value of
> kCurrentVersionNumber, then all of these unit tests should fail.  Thus
requiring
> that these unit tests would need updating to the new kCurrentVersionNumber
> value?
> 
> Adding to this file:
>   const int kCurrentTestedVersionNumber = 25;
> 
> and changing checks to:
>   ASSERT_EQ(kCurrentTestedVersionNumber, db.GetVersionNumber());
> 
> Yes?

Yes.  It's kind of a big hammer, but I'm not sure it's worth writing something
with more finesse to it.

[dhollowa, 2010-09-01 20:02:17]

Done, yes.  I like it.

http://codereview.chromium.org/3295002/diff/7001/8003
File chrome/browser/webdata/web_database_unittest.cc (right):

http://codereview.chromium.org/3295002/diff/7001/8003#newcode1804
chrome/browser/webdata/web_database_unittest.cc:1804: ASSERT_LE(23,
db.GetVersionNumber());
On 2010/09/01 18:22:05, shess wrote:
> This needs to be stronger - this wouldn't catch the case where the version
> number went to 23, but didn't make it to 24, and nobody wrote a new test for
> migrating to 24.
> 
> My suggestion would be to hard-code a duplicate of the current version number
as
> a constant in this file, and have every test assert that it arrived at that
> version number.  Put in a baseline test that an empty database gets the
expected
> version number, so that you can provide a helpful failure message when someone
> versions things.  And a comment about requiring a new migration test.

Done.

[Scott Hess - ex-Googler, 2010-09-01 20:15:52]

looking good.

http://codereview.chromium.org/3295002/diff/17001/18002
File chrome/browser/webdata/web_database.h (right):

http://codereview.chromium.org/3295002/diff/17001/18002#newcode302
chrome/browser/webdata/web_database.h:302: int GetVersionNumber();
With your change to pull directly from the meta table, is this needed any
longer?

http://codereview.chromium.org/3295002/diff/17001/18003
File chrome/browser/webdata/web_database_unittest.cc (right):

http://codereview.chromium.org/3295002/diff/17001/18003#newcode1816
chrome/browser/webdata/web_database_unittest.cc:1816:
EXPECT_EQ(WebDatabaseMigrationTest::kCurrentTestedVersionNumber,
Do you really need the class qualifier for these?

http://codereview.chromium.org/3295002/diff/17001/18003#newcode1820
chrome/browser/webdata/web_database_unittest.cc:1820:
ASSERT_TRUE(connection.DoesTableExist("meta"));
Now that you said EXPECT_EQ() above rather than ASSERT_EQ(), you have me
thinking that these assertions about the shape of the schema could be
EXPECT_TRUE(), too.  Though I'd be fine with having the above be ASSERT_EQ() and
ignoring the problem (if it didn't get to the right version, then you probably
should expect lots of stuff to be busted, right?).

[dhollowa, 2010-09-01 20:27:29]

http://codereview.chromium.org/3295002/diff/17001/18002
File chrome/browser/webdata/web_database.h (right):

http://codereview.chromium.org/3295002/diff/17001/18002#newcode302
chrome/browser/webdata/web_database.h:302: int GetVersionNumber();
On 2010/09/01 20:15:52, shess wrote:
> With your change to pull directly from the meta table, is this needed any
> longer?

Oops.  You're right.

http://codereview.chromium.org/3295002/diff/17001/18003
File chrome/browser/webdata/web_database_unittest.cc (right):

http://codereview.chromium.org/3295002/diff/17001/18003#newcode1816
chrome/browser/webdata/web_database_unittest.cc:1816:
EXPECT_EQ(WebDatabaseMigrationTest::kCurrentTestedVersionNumber,
On 2010/09/01 20:15:52, shess wrote:
> Do you really need the class qualifier for these?

Nope.  You're right.

http://codereview.chromium.org/3295002/diff/17001/18003#newcode1820
chrome/browser/webdata/web_database_unittest.cc:1820:
ASSERT_TRUE(connection.DoesTableExist("meta"));
On 2010/09/01 20:15:52, shess wrote:
> Now that you said EXPECT_EQ() above rather than ASSERT_EQ(), you have me
> thinking that these assertions about the shape of the schema could be
> EXPECT_TRUE(), too.  Though I'd be fine with having the above be ASSERT_EQ()
and
> ignoring the problem (if it didn't get to the right version, then you probably
> should expect lots of stuff to be busted, right?).

I like the more vocal EXPECT_, yes.  Done.

[Scott Hess - ex-Googler, 2010-09-01 21:28:33]

LGTM

Again, thanks for putting this in place.  Someday shipping a bug will be
unpossible!