Remove unused SQLLite tables from WebData

Remove unused SQLLite tables from WebData

WebApp code was deleted here:
https://codereview.chromium.org/19746

WebIntents code was deleted here:
https://codereview.chromium.org/22407016/
https://codereview.chromium.org/12225076/

BUG=241909
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283808

[Cait (Slow), 2014-07-10 00:40:50]

pkasting -- PTAL. What is the best way to handle migration tests which are now
failing because they were testing changes to the tables I've deleted?

Thanks!

[Peter Kasting, 2014-07-10 01:01:09]

If there's a migration test that tests, for example, that in version n we added
table X, or we made some set of changes to table X; and then a later migration
to version m > n removes table X entirely, then eliminate all the tests that
tested the table X changes in version n.

The tests test that the current version of Chromium does the right thing, and if
in the current version we don't care about table X, there's no reason to have
any tests related to it anymore.

[Scott Hess - ex-Googler, 2014-07-10 01:55:53]

On 2014/07/10 00:40:50, Cait Phillips wrote:
> pkasting -- PTAL. What is the best way to handle migration tests which are now
> failing because they were testing changes to the tables I've deleted?

Database code tends to have two sorts of tests.  Functional tests, like starting
with a database at a particular version, doing all migrations to current, then
testing that things still work should still work, if they don't work you
probably need to fix something.

The other kind of test is where you're poking through the abstraction and
testing that specific implementation details still hold.  In that case, it
really depends on what specific details are being tested.  As Peter indicates,
usually you can just excise some part of the test expectations.

https://codereview.chromium.org/382703002/diff/1/components/webdata/common/we...
File components/webdata/common/web_database.cc (right):

https://codereview.chromium.org/382703002/diff/1/components/webdata/common/we...
components/webdata/common/web_database.cc:197: db_.Execute("DROP TABLE
web_apps")) &&
This and the others should be like:
  db_.Execute("DROP TABLE IF EXISTS web_apps") &&
rather than manually testing.

https://codereview.chromium.org/382703002/diff/1/components/webdata/common/we...
File components/webdata/common/web_database_migration_unittest.cc (right):

https://codereview.chromium.org/382703002/diff/1/components/webdata/common/we...
components/webdata/common/web_database_migration_unittest.cc:1914:
EXPECT_TRUE(connection.DoesColumnExist("web_intents_defaults", "scheme"));
For instance, since this is testing migration to "current", obviously this data
will no longer be here.  Presumably it was migrated to somewhere else?  I
usually discourage rewriting migration tests, as you can easily fail to test
what you really need, but it's hard to avoid in a case like this.

If the data migrated and this is vestigial, one option is to first land a CL
which tests the final location, because that should succeed before this CL and
after.  If the data is just going to be dumped, though, can't really do that.

[Scott Hess - ex-Googler, 2014-07-10 02:01:46]

BTW, Sqlite.Version.Web shows that only a very small percentage of users see
databases at v52 or older.  Another option would be to deprecate older versions
and just remove a load of code first, which avoids some decisions. 
chrome/browser/history/thumbnail_database.cc and top_sites_database.cc have
examples of deprecation.  The basic idea is to just blow away the entire
database if the version is very old, rather than trying to migrate it forward. 
One possibility for why people are running such old databases is that something
is preventing the database from being migrated, so clearing it out gives a
chance to make forward progress.

[Peter Kasting, 2014-07-10 17:50:48]

https://codereview.chromium.org/382703002/diff/1/components/webdata/common/we...
File components/webdata/common/web_database_migration_unittest.cc (right):

https://codereview.chromium.org/382703002/diff/1/components/webdata/common/we...
components/webdata/common/web_database_migration_unittest.cc:1914:
EXPECT_TRUE(connection.DoesColumnExist("web_intents_defaults", "scheme"));
On 2014/07/10 01:55:53, Scott Hess wrote:
> For instance, since this is testing migration to "current", obviously this
data
> will no longer be here.  Presumably it was migrated to somewhere else?

I believe all the web_intents stuff is gone entirely, not migrated.  This is why
I suggested just removing these tests completely; there's no alternate form of
the test that can test that a migration elsewhere succeeded.

[Scott Hess - ex-Googler, 2014-07-10 19:19:39]

https://codereview.chromium.org/382703002/diff/1/components/webdata/common/we...
File components/webdata/common/web_database_migration_unittest.cc (right):

https://codereview.chromium.org/382703002/diff/1/components/webdata/common/we...
components/webdata/common/web_database_migration_unittest.cc:1914:
EXPECT_TRUE(connection.DoesColumnExist("web_intents_defaults", "scheme"));
On 2014/07/10 17:50:47, Peter Kasting wrote:
> On 2014/07/10 01:55:53, Scott Hess wrote:
> > For instance, since this is testing migration to "current", obviously this
> data
> > will no longer be here.  Presumably it was migrated to somewhere else?
> 
> I believe all the web_intents stuff is gone entirely, not migrated.  This is
why
> I suggested just removing these tests completely; there's no alternate form of
> the test that can test that a migration elsewhere succeeded.

Yeah, if the data is being removed because it is simply obsolete, then
migrate-to-current needs to test that everything works, and the resulting schema
looks right.

I'm less certain of actually removing the test.  Ideally, it would be nice to
have a golden for every starting point, to verify that anyone starting there
migrates successfully to the current schema, even if the actual data migrated at
that point is deleted by a later migration.  I'm agnostic-to-negative about
checks like !DoesTableExist("web_intents") for tests other than
MigrateVersion57ToCurrent, which is testing the specific migration that removes
those tables.

https://codereview.chromium.org/382703002/diff/1/components/webdata/common/we...
components/webdata/common/web_database_migration_unittest.cc:2745:
EXPECT_TRUE(connection.DoesTableExist("web_app_icons"));
Order this above "web_intents", just because.  Also below.

[Cait (Slow), 2014-07-14 23:09:46]

https://codereview.chromium.org/382703002/diff/1/components/webdata/common/we...
File components/webdata/common/web_database.cc (right):

https://codereview.chromium.org/382703002/diff/1/components/webdata/common/we...
components/webdata/common/web_database.cc:197: db_.Execute("DROP TABLE
web_apps")) &&
On 2014/07/10 01:55:53, Scott Hess wrote:
> This and the others should be like:
>   db_.Execute("DROP TABLE IF EXISTS web_apps") &&
> rather than manually testing.

Done.

https://codereview.chromium.org/382703002/diff/1/components/webdata/common/we...
File components/webdata/common/web_database_migration_unittest.cc (right):

https://codereview.chromium.org/382703002/diff/1/components/webdata/common/we...
components/webdata/common/web_database_migration_unittest.cc:1914:
EXPECT_TRUE(connection.DoesColumnExist("web_intents_defaults", "scheme"));
On 2014/07/10 19:19:39, Scott Hess wrote:
> On 2014/07/10 17:50:47, Peter Kasting wrote:
> > On 2014/07/10 01:55:53, Scott Hess wrote:
> > > For instance, since this is testing migration to "current", obviously this
> > data
> > > will no longer be here.  Presumably it was migrated to somewhere else?
> > 
> > I believe all the web_intents stuff is gone entirely, not migrated.  This is
> why
> > I suggested just removing these tests completely; there's no alternate form
of
> > the test that can test that a migration elsewhere succeeded.
> 
> Yeah, if the data is being removed because it is simply obsolete, then
> migrate-to-current needs to test that everything works, and the resulting
schema
> looks right.
> 
> I'm less certain of actually removing the test.  Ideally, it would be nice to
> have a golden for every starting point, to verify that anyone starting there
> migrates successfully to the current schema, even if the actual data migrated
at
> that point is deleted by a later migration.  I'm agnostic-to-negative about
> checks like !DoesTableExist("web_intents") for tests other than
> MigrateVersion57ToCurrent, which is testing the specific migration that
removes
> those tables.

To confirm, yes, the code that used these tables has long-since been deleted
(see CL description for details). 

shess: Are "goldens" the version_XX.sql files? If so, there are several which
define the tables which I am deleting (version 44 and up), so any test migrating
from version 44 to the current version will test that these are properly
deleted. That said, if there's a better way to do this (besides checks like
!DoesTableExist("web_intents")), that doesn't involve shaving too many yaks, I'd
be add it.

https://codereview.chromium.org/382703002/diff/1/components/webdata/common/we...
components/webdata/common/web_database_migration_unittest.cc:2745:
EXPECT_TRUE(connection.DoesTableExist("web_app_icons"));
On 2014/07/10 19:19:39, Scott Hess wrote:
> Order this above "web_intents", just because.  Also below.

Done.

[Scott Hess - ex-Googler, 2014-07-14 23:27:53]

tl;dr - I think you need to put MigrateVersion45ToCurrent and
MigrateVersion45InvalidToCurrent back, stripped down to remove expectations
other than "The migration to worked and got to the current version".  I would
only support removing the tests for those specific endpoints if a good argument
was made for why there will be no users arriving with versions at those specific
endpoints.

https://codereview.chromium.org/382703002/diff/1/components/webdata/common/we...
File components/webdata/common/web_database_migration_unittest.cc (right):

https://codereview.chromium.org/382703002/diff/1/components/webdata/common/we...
components/webdata/common/web_database_migration_unittest.cc:1914:
EXPECT_TRUE(connection.DoesColumnExist("web_intents_defaults", "scheme"));
On 2014/07/14 23:09:45, Cait Phillips wrote:
> On 2014/07/10 19:19:39, Scott Hess wrote:
> > On 2014/07/10 17:50:47, Peter Kasting wrote:
> > > On 2014/07/10 01:55:53, Scott Hess wrote:
> > > > For instance, since this is testing migration to "current", obviously
this
> > > data
> > > > will no longer be here.  Presumably it was migrated to somewhere else?
> > > 
> > > I believe all the web_intents stuff is gone entirely, not migrated.  This
is
> > why
> > > I suggested just removing these tests completely; there's no alternate
form
> of
> > > the test that can test that a migration elsewhere succeeded.
> > 
> > Yeah, if the data is being removed because it is simply obsolete, then
> > migrate-to-current needs to test that everything works, and the resulting
> schema
> > looks right.
> > 
> > I'm less certain of actually removing the test.  Ideally, it would be nice
to
> > have a golden for every starting point, to verify that anyone starting there
> > migrates successfully to the current schema, even if the actual data
migrated
> at
> > that point is deleted by a later migration.  I'm agnostic-to-negative about
> > checks like !DoesTableExist("web_intents") for tests other than
> > MigrateVersion57ToCurrent, which is testing the specific migration that
> removes
> > those tables.
> 
> To confirm, yes, the code that used these tables has long-since been deleted
> (see CL description for details). 
> 
> shess: Are "goldens" the version_XX.sql files? If so, there are several which
> define the tables which I am deleting (version 44 and up), so any test
migrating
> from version 44 to the current version will test that these are properly
> deleted. That said, if there's a better way to do this (besides checks like
> !DoesTableExist("web_intents")), that doesn't involve shaving too many yaks,
I'd
> be add it. 

OK, so my position:

1) have a test which tests the specific transition from "this table exists" to
"this table doesn't exist".  Which you have at MigrateVersion57ToCurrent.

2) have tests which actually exercise the code from every shipped version to
current.  Which you also have by virtue of migrating the golden files (yes,
version_XX.sql).

#2 isn't _quite_ the same as having someone test the transition indirectly. 
It's possible that the transition from 43->44->45 works but 44->45 doesn't
because of something subtle going on.  The sketchy bit which some people may
take offense to is that the resulting test just exercises the migration code,
but doesn't have any expectations about what the migration code did, other than
"the version is current".  To my mind, this is works-as-intended, I just want to
make sure that that migration code actually continues to work without crashing
or stranding users with the old version.

If I were feeling pedantic, I'd suggest that the code might have a helper
function for positively verifying that the schema is correct (somehow says
"Exactly _these_ tables, with exactly _these_ columns").  That's kind of
open-ended and I don't think it's worth blocking things for.

https://codereview.chromium.org/382703002/diff/40001/components/webdata/commo...
File components/webdata/common/web_database.cc (right):

https://codereview.chromium.org/382703002/diff/40001/components/webdata/commo...
components/webdata/common/web_database.cc:199: (db_.Execute("DROP TABLE IF
EXISTS web_intents_defaults")) &&
Now that you aren't distinguishing the || from the &&, you don't need the outer
parenthesis on these four lines.

[Cait (Slow), 2014-07-15 16:58:39]

PTAL. I added back the version 44/45 tests which now just run the migration and
confirm no web_intents table is present at the end.

https://codereview.chromium.org/382703002/diff/40001/components/webdata/commo...
File components/webdata/common/web_database.cc (right):

https://codereview.chromium.org/382703002/diff/40001/components/webdata/commo...
components/webdata/common/web_database.cc:199: (db_.Execute("DROP TABLE IF
EXISTS web_intents_defaults")) &&
On 2014/07/14 23:27:53, Scott Hess wrote:
> Now that you aren't distinguishing the || from the &&, you don't need the
outer
> parenthesis on these four lines.

Done.

[Scott Hess - ex-Googler, 2014-07-15 17:00:20]

lgtm

[Peter Kasting, 2014-07-15 19:15:04]

LGTM

https://codereview.chromium.org/382703002/diff/60001/components/webdata/commo...
File components/webdata/common/web_database_migration_unittest.cc (right):

https://codereview.chromium.org/382703002/diff/60001/components/webdata/commo...
components/webdata/common/web_database_migration_unittest.cc:309:
EXPECT_FALSE(connection.DoesTableExist("web_apps"));
Nit: Add a comment about these similar to the comments on other EXPECT_FALSEs
above.

https://codereview.chromium.org/382703002/diff/60001/components/webdata/commo...
components/webdata/common/web_database_migration_unittest.cc:1875: // modified
to include "scheme" columns.
I disagree with Scott that these two tests should stay.  I don't think they test
anything about migrations more than our other remaining tests do.

If you want to keep them, you need to update the comments on them to explain
what the tests are doing now (and perhaps, if necessary, what they originally
did).  The current comments on them are lies.

[Cait (Slow), 2014-07-17 15:35:50]

thanks for the reviews!

https://codereview.chromium.org/382703002/diff/60001/components/webdata/commo...
File components/webdata/common/web_database_migration_unittest.cc (right):

https://codereview.chromium.org/382703002/diff/60001/components/webdata/commo...
components/webdata/common/web_database_migration_unittest.cc:309:
EXPECT_FALSE(connection.DoesTableExist("web_apps"));
On 2014/07/15 19:15:04, Peter Kasting wrote:
> Nit: Add a comment about these similar to the comments on other EXPECT_FALSEs
> above.

Done.

https://codereview.chromium.org/382703002/diff/60001/components/webdata/commo...
components/webdata/common/web_database_migration_unittest.cc:1875: // modified
to include "scheme" columns.
On 2014/07/15 19:15:04, Peter Kasting wrote:
> I disagree with Scott that these two tests should stay.  I don't think they
test
> anything about migrations more than our other remaining tests do.
> 
> If you want to keep them, you need to update the comments on them to explain
> what the tests are doing now (and perhaps, if necessary, what they originally
> did).  The current comments on them are lies.

Done.

[Cait (Slow), 2014-07-17 15:36:00]

The CQ bit was checked by caitkp@chromium.org

[commit-bot: I haz the power, 2014-07-17 15:37:13]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/caitkp@chromium.org/382703002/120001

[commit-bot: I haz the power, 2014-07-17 17:49:53]

Change committed as 283808