Open Bug 1066892 Opened 5 years ago Updated 4 years ago

Basic database migration tests

Categories

(Firefox for Android :: Data Providers, defect)

All
Android
defect
Not set

Tracking

()

People

(Reporter: wesj, Unassigned)

Details

Attachments

(1 file, 1 obsolete file)

We've been looking to delete a bunch of old database code. I'd like to make slightly better migration testing in place before we do.
Attached patch testDBMigration (obsolete) — Splinter Review
This isn't great. Just starts at one (pulled that code out of the original db patch) and works its way up to current. Doesn't test much along the way, but it seems better than having nothing. Try run:

https://tbpl.mozilla.org/?tree=Try&rev=79e8f25299cd
Attachment #8488914 - Flags: review?(rnewman)
Comment on attachment 8488914 [details] [diff] [review]
testDBMigration

Review of attachment 8488914 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/db/BrowserDatabaseHelper.java
@@ +70,5 @@
> +        this(context, databasePath, DATABASE_VERSION);
> +    }
> +
> +    // This is only to be used for testing
> +    @JNITarget

This should be @RobocopTarget.

@@ +791,4 @@
>      @Override
>      public void onCreate(SQLiteDatabase db) {
> +        // We maintain code to create the lowest version of the database we'll support updating from
> +        // purely so we can test the upgrade path.

Can we do this by putting that code in, say:

  public class StubbedBrowserDatabaseHelper extends BrowserDatabaseHelper {
      @Override
      public void onCreate(SQLiteDatabase db) {
          // We maintain...
          switch (db.getVersion()) {
              case 1:
                  createVersion1(db);
                  return;
              default:
                  super.onCreate(db);
                  return;
          }
      }

That'll avoid us shipping code we don't use, or damaging it by other work we do.

::: mobile/android/base/tests/testMigration.java
@@ +8,5 @@
> +import android.database.Cursor;
> +
> +import java.io.File;
> +
> +public class testMigration extends PixelTest {

Why not BaseTest or BaseRobocopTest?

See testBrowserProviderPerf, which uses the latter.

@@ +13,5 @@
> +    public void testMigration() {
> +        blockForGeckoReady();
> +
> +        File f = getActivity().getFileStreamPath("test.db");
> +        for (int i = 1; i <= BrowserDatabaseHelper.DATABASE_VERSION; i++) {

I am so amazed that this upgrade path works without the intermediates!

@@ +23,5 @@
> +            mAsserter.is(db.getVersion(), i, "Got the right version");
> +
> +            Cursor c = db.rawQuery("SELECT * FROM bookmarks", new String[0]);
> +            mAsserter.isnot(c, null, "Got a cursor");
> +            c.close();

Please put all of these close()s in finally blocks. If the assertion fails, we'll get logspam about unclosed cursors, and that makes spotting a problem more difficult.

Yes, this will slightly clutter this test code. I think it's worth it.
Attachment #8488914 - Flags: review?(rnewman) → feedback+
Assignee: nobody → wjohnston
Status: NEW → ASSIGNED
OS: Linux → Android
Hardware: x86_64 → All
Attached patch PatchSplinter Review
Updated. I added some basic data to the original db rows, and found things were less... happy. i.e. we're missing some migrations, and we've busted a few others on accident (adding random things to createBookmarksTable for instance). Fixed them up here.

Pushed this to try:
https://tbpl.mozilla.org/?tree=Try&rev=73b4e154aeed
Attachment #8488914 - Attachment is obsolete: true
Attachment #8489679 - Flags: review?(rnewman)
Comment on attachment 8489679 [details] [diff] [review]
Patch

Review of attachment 8489679 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with nits, moved into test suite instead of m/a/b/db, and with that index line moved into the real code!

Thanks for taking the time to do this.

::: mobile/android/base/db/BrowserContract.java
@@ -382,5 @@
>                  Obsolete.TABLE_IMAGES + " ON " + Bookmarks.TABLE_NAME + "." + Bookmarks.URL + " = " +
>                  Obsolete.TABLE_IMAGES + "." + Obsolete.Images.URL;
>  
>          static final String TABLE_HISTORY_JOIN_IMAGES = History.TABLE_NAME + " LEFT OUTER JOIN " +
> -                Obsolete.TABLE_IMAGES + " ON " + Bookmarks.TABLE_NAME + "." + History.URL + " = " +

Nice catch!

::: mobile/android/base/db/BrowserDatabaseHelper.java
@@ +1095,2 @@
>          migrateHistoryTable(db);
> +        debug("migrate images");

If you opt to leave these in, please change to "Migrating images..." etc.

::: mobile/android/base/db/BrowserDatabaseTestingHelper.java
@@ +18,5 @@
> +/* This class is only used for testing purposes. It contains information about earlier
> + * versions of the history/bookmarks database so that we can create an early version of it
> + * and test migration paths.
> + */
> +public final class BrowserDatabaseTestingHelper extends BrowserDatabaseHelper {

Can we move this into the test suite? That it has members decorated @RobocopTarget means it won't be proguarded out, and it's not used from the main app.

@@ +99,5 @@
> +        super.onCreate(db);
> +    }
> +
> +    @Override
> +    public void onUpgrade(SQLiteDatabase db, int oldVersion, int newVersion) {

What's the earliest version that we practically support? (I.e., earlier than that it's "uninstall and reinstall", not "upgrade".)

Looks like Firefox 14 was v8:

http://hg.mozilla.org/releases/mozilla-release/file/6d69e50846fa/mobile/android/base/db/BrowserProvider.java.in#l70

and Beta 12 was v1:

http://hg.mozilla.org/releases/mozilla-release/file/1ae0f96df824/mobile/android/base/db/BrowserProvider.java.in#l70


So assuming we only support v8+, we should have 1-8 in this method, and 8+ in BDH, no?

See below...

@@ +135,5 @@
> +                       "SELECT url_key, favicon, favicon_url, thumbnail, created, modified, guid, deleted FROM images_tmp");
> +            db.execSQL("DROP TABLE IF EXISTS images_tmp");
> +        } else if (newVersion == 11) {
> +            // The bookmarks table creation code assumes this is being inserted for the first time
> +            db.execSQL("DROP INDEX IF EXISTS bookmarks_type_deleted_index");

This is because we refactored migrations such that 1->2, 3->4, and 6->7 call migrateBookmarksTable, which creates this index.

migrateBookmarksTable also drops this index if it exists.

migrateBookmarksTable isn't called in upgradeDatabaseFrom10to11, even though it adds that index.

That implies that this is a real bug -- not something to work around in testing -- and the main upgradeDatabaseFrom10to11 method should actually include this line!

::: mobile/android/base/tests/testMigration.java
@@ +91,5 @@
> +            }
> +        }
> +    }
> +
> +    // Its not that fair to test migration on empty databases. This method adds

Nit: "It's"
Attachment #8489679 - Flags: review?(rnewman) → review+
Not ready to go just yet, it seems:
https://tbpl.mozilla.org/php/getParsedLog.php?id=49573722&tree=Try&full=1

I'll try and find time to poke this next week, unless someone beats me to it.
Unassigning. These are useful though, and should land someday :)
Assignee: wjohnston → nobody
Status: ASSIGNED → NEW
You need to log in before you can comment on or make changes to this bug.