Remove unused SQLLite tables from WebData

components/webdata/common/web_database_migration_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <string>
 
 #include "base/file_util.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/guid.h"
 #include "base/message_loop/message_loop.h"
 #include "base/path_service.h"
 #include "base/stl_util.h"
 #include "base/strings/string16.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/time/time.h"
 #include "base/values.h"
-#include "chrome/browser/webdata/web_apps_table.h"
-#include "chrome/browser/webdata/web_intents_table.h"
 #include "components/autofill/core/browser/autofill_country.h"
 #include "components/autofill/core/browser/autofill_profile.h"
 #include "components/autofill/core/browser/autofill_type.h"
 #include "components/autofill/core/browser/credit_card.h"
 #include "components/autofill/core/browser/webdata/autofill_change.h"
 #include "components/autofill/core/browser/webdata/autofill_entry.h"
 #include "components/autofill/core/browser/webdata/autofill_table.h"
 #include "components/password_manager/core/browser/webdata/logins_table.h"
 #include "components/search_engines/keyword_table.h"
 #include "components/signin/core/browser/webdata/token_service_table.h"
@@ skipping 151 matching lines @@
   // the current version.
   void DoMigration() {
     // TODO(joi): This whole unit test file needs to stay in //chrome
     // for now, as it needs to know about all the different table
     // types. Once all webdata datatypes have been componentized, this
     // could move to components_unittests.
     AutofillTable autofill_table("en-US");
     KeywordTable keyword_table;
     LoginsTable logins_table;
     TokenServiceTable token_service_table;
-    WebAppsTable web_apps_table;
-    WebIntentsTable web_intents_table;
 
     WebDatabase db;
     db.AddTable(&autofill_table);
     db.AddTable(&keyword_table);
     db.AddTable(&logins_table);
     db.AddTable(&token_service_table);
-    db.AddTable(&web_apps_table);
-    db.AddTable(&web_intents_table);
 
     // This causes the migration to occur.
     ASSERT_EQ(sql::INIT_OK, db.Init(GetDatabasePath()));
   }
 
  protected:
   // Current tested version number.  When adding a migration in
   // |WebDatabase::MigrateOldVersionsAsNeeded()| and changing the version number
   // |kCurrentVersionNumber| this value should change to reflect the new version
   // number and a new migration test added below.
@@ skipping 37 matching lines @@
   //   > .output version_nn.sql
   //   > .dump
   void LoadDatabase(const base::FilePath::StringType& file);
 
  private:
   base::ScopedTempDir temp_dir_;
 
   DISALLOW_COPY_AND_ASSIGN(WebDatabaseMigrationTest);
 };
 
-const int WebDatabaseMigrationTest::kCurrentTestedVersionNumber = 57;
+const int WebDatabaseMigrationTest::kCurrentTestedVersionNumber = 58;
 
 void WebDatabaseMigrationTest::LoadDatabase(
     const base::FilePath::StringType& file) {
   std::string contents;
   ASSERT_TRUE(GetWebDatabaseData(base::FilePath(file), &contents));
 
   sql::Connection connection;
   ASSERT_TRUE(connection.Open(GetDatabasePath()));
   ASSERT_TRUE(connection.Execute(contents.data()));
 }
@@ skipping 35 matching lines @@
     // The autofill_dates table is obsolete. (It's been merged into the autofill
     // table.)
     EXPECT_FALSE(connection.DoesTableExist("autofill_dates"));
     EXPECT_TRUE(connection.DoesTableExist("autofill_profiles"));
     EXPECT_TRUE(connection.DoesTableExist("credit_cards"));
     EXPECT_TRUE(connection.DoesTableExist("keywords"));
     // The logins table is obsolete. (We used to store saved passwords here.)
     EXPECT_FALSE(connection.DoesTableExist("logins"));
     EXPECT_TRUE(connection.DoesTableExist("meta"));
     EXPECT_TRUE(connection.DoesTableExist("token_service"));
-    EXPECT_TRUE(connection.DoesTableExist("web_app_icons"));
-    EXPECT_TRUE(connection.DoesTableExist("web_apps"));
-    EXPECT_TRUE(connection.DoesTableExist("web_intents"));
-    EXPECT_TRUE(connection.DoesTableExist("web_intents_defaults"));
+    EXPECT_FALSE(connection.DoesTableExist("web_app_icons"));
+    EXPECT_FALSE(connection.DoesTableExist("web_apps"));
+    EXPECT_FALSE(connection.DoesTableExist("web_intents"));
+    EXPECT_FALSE(connection.DoesTableExist("web_intents_defaults"));
   }
 }
 
 // Tests that absent Autofill tables do not create any problems when migrating
 // from a DB written by the earliest publicly released version of Chrome.
 TEST_F(WebDatabaseMigrationTest, MigrateVersion20ToCurrent) {
   ASSERT_NO_FATAL_FAILURE(LoadDatabase(FILE_PATH_LITERAL("version_20.sql")));
 
   // Verify pre-conditions.
   {
@@ skipping 1581 matching lines @@
     EXPECT_EQ(kCurrentTestedVersionNumber, VersionFromConnection(&connection));
 
     sql::MetaTable meta_table;
     ASSERT_TRUE(meta_table.Init(
         &connection,
         kCurrentTestedVersionNumber,
         kCurrentTestedVersionNumber));
 
     // A new "scheme" column should have been added to each web_intents table.
     EXPECT_TRUE(connection.DoesColumnExist("web_intents", "scheme"));
     EXPECT_TRUE(connection.DoesColumnExist("web_intents_defaults", "scheme"));

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

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.

[Peter Kasting, 2014/07/10 17:50:47]

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]

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.

[Cait (Slow), 2014/07/14 23:09:45]

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. 

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

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.

 
     // Verify existing user data was copied.
     sql::Statement s1(
         connection.GetUniqueStatement("SELECT * FROM web_intents"));
 
     ASSERT_TRUE(s1.Step());
     EXPECT_EQ("http://poodles.com/fuzzer", s1.ColumnString(0));
     EXPECT_EQ(ASCIIToUTF16("fuzz"), s1.ColumnString16(1));
     EXPECT_EQ(ASCIIToUTF16("poodle/*"), s1.ColumnString16(2));
     EXPECT_EQ(ASCIIToUTF16("Poodle Fuzzer"), s1.ColumnString16(3));
@@ skipping 795 matching lines @@
     EXPECT_EQ("B41FE6E0-B13E-2A2A-BF0B-29FCE2C3ADBD", s_names.ColumnString(0));
     EXPECT_EQ(ASCIIToUTF16("Jon"), s_names.ColumnString16(1));
     EXPECT_EQ(base::string16(), s_names.ColumnString16(2));
     EXPECT_EQ(ASCIIToUTF16("Smith"), s_names.ColumnString16(3));
     EXPECT_EQ(base::string16(), s_names.ColumnString16(4));
 
     // No more entries expected.
     ASSERT_FALSE(s_names.Step());
   }
 }
+
+// Tests that migrating from version 57 to version 58 drops the web_intents and
+// web_apps tables.
+TEST_F(WebDatabaseMigrationTest, MigrateVersion57ToCurrent) {
+  ASSERT_NO_FATAL_FAILURE(LoadDatabase(FILE_PATH_LITERAL("version_57.sql")));
+
+  // Verify pre-conditions. These are expectations for version 57 of the
+  // database.
+  {
+    sql::Connection connection;
+    ASSERT_TRUE(connection.Open(GetDatabasePath()));
+    ASSERT_TRUE(sql::MetaTable::DoesTableExist(&connection));
+
+    EXPECT_TRUE(connection.DoesTableExist("web_apps"));
+    EXPECT_TRUE(connection.DoesTableExist("web_intents"));
+    EXPECT_TRUE(connection.DoesTableExist("web_app_icons"));

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

Order this above "web_intents", just because.  Also below.

[Cait (Slow), 2014/07/14 23:09:45]

Done.

+    EXPECT_TRUE(connection.DoesTableExist("web_intents_defaults"));
+  }
+
+  DoMigration();
+
+  // Verify post-conditions. These are expectations for current version of the
+  // database.
+  {
+    sql::Connection connection;
+    ASSERT_TRUE(connection.Open(GetDatabasePath()));
+    ASSERT_TRUE(sql::MetaTable::DoesTableExist(&connection));
+
+    // Check version.
+    EXPECT_EQ(kCurrentTestedVersionNumber, VersionFromConnection(&connection));
+
+    EXPECT_FALSE(connection.DoesTableExist("web_apps"));
+    EXPECT_FALSE(connection.DoesTableExist("web_intents"));
+    EXPECT_FALSE(connection.DoesTableExist("web_app_icons"));
+    EXPECT_FALSE(connection.DoesTableExist("web_intents_defaults"));
+  }
+}