Fix various issues in sqlite implementation. #272

Closed
wants to merge 9 commits into
from

4 participants

@perlow
  • The lifetime of a database was previously tied to the SQLiteDatabase instance. This
    is not the appropriate android behavior and breaks code that opens and closes the
    same database path mulitple times. The behavior is changed to cache the database
    connection which can then be shared across multiple SQLiteDatabase instances. A
    new helper method ShadowSQLiteDatabase.deleteDatabase is added which a test can
    use to delete the database. The database now supports the path and name parameter
    such that calls with the same path/name reference the same database file.

    This is a breaking change for tests. All tests in the project were updated to
    handle the new behavior.

  • Added missing implementations of various methods in ShadowSQLiteDatabase and
    ShadowSQLiteQueryBuilder.

  • Fixed nested transactions in ShadowSQLiteDatabase. The android implementation
    supports nested transactions such that a transaction is only committed when its
    outer-most transaction is ended successfully. I copied the logic from the
    Android implementation to match the correct behavior. Also fixed the locking
    semantics in this area to mach Android. Because h2/sqlite differ in how
    transactions work, I made sqlite match the android behavior and maintained
    backwards compatability with how the code was previously using h2.

  • Removed some dead code from ShadowSQLiteDatabase.

Jon Perlow Fix various issues in sqlite implementation.
- The lifetime of a database was previously tied to the SQLiteDatabase instance. This
  is not the appropriate android behavior and breaks code that opens and closes the
  same database path mulitple times. The behavior is changed to cache the database
  connection which can then be shared across multiple SQLiteDatabase instances. A
  new helper method ShadowSQLiteDatabase.deleteDatabase is added which a test can
  use to delete the database. The database now supports the path and name parameter
  such that calls with the same path/name reference the same database file.

  This is a breaking change for tests. All tests in the project were updated to
  handle the new behavior.

- Added missing implementations of various methods in ShadowSQLiteDatabase and
  ShadowSQLiteQueryBuilder.

- Fixed nested transactions in ShadowSQLiteDatabase. The android implementation
  supports nested transactions such that a transaction is only committed when its
  outer-most transaction is ended successfully. I copied the logic from the
  Android implementation to match the correct behavior. Also fixed the locking
  semantics in this area to mach Android. Because h2/sqlite differ in how
  transactions work, I made sqlite match the android behavior and maintained
  backwards compatability with how the code was previously using h2.

- Removed some dead code from ShadowSQLiteDatabase.
0112cb6
@tylerschultz

Hi Jon.

There is a lot of great looking code here.

I'm concerned that there are 500+ lines of code in this list with only a couple of new tests. There are several new methods annotated with @Implementation that are not covered. I realize this code references a lot of hard to test dependencies on databases. I realize the test coverage in this area of Robolectric is already weak.

Do you feel like there is any test coverage that could be added?

--Tyler

@perlow

I will take a look and see what can be added. Here's my question though. At the testing level, do you differentiate between code that is custom code in robolectric versus code that is purely copied from Android? A large number of the new lines are in ShadowSQLiteQueryBuilder and were just copy/pasted from the android code base.

@tylerschultz

I see, I didn't realize that there was that much Android code in here. There are various places where we've copied Android code in the codebase, and we've only smoke tested that the Shadow itself is properly wired up. Typically we add a comment to the top of the file to note there is copied content. I'll take another look at all this after work tonight. Thanks taking the time to contribute.

@perlow

I have a few more changes that make this easier to use in test. I'll add them to this request hopefulyl tonight.

Jon Perlow and others added some commits Jun 23, 2012
Jon Perlow Cleanup deleting of databases. 6acc98f
Jon Perlow Clone method for ShadowBundle. 9cf34aa
Peter Brook Make android.os.Vibrator work with JellyBean
Summary: android.os.Vibrator became an abstract class in Android 4.1.
This copies the dummy NullVibrator implementation from the 4.1 sdk
source and instructs robolectric to load that class if it sees that
android.os.Vibrator is abstract.

Test Plan: Linked against the r16 (jellybean) android.jar and ran unit
tests which relied on Vibrator.
4eae6f3
Jon Perlow Intellij files. 27d18e8
Jon Perlow Fix Bundle/Parcel to work correctly. 4f47933
Jon Perlow Fix bug in ShadowSQLiteQueryBuilder where it didn't properly handle
empty where clauses.
38d3bb1
Jonathan Perlow Make the implementation of ContentObserver/ContentProvider based on
android's and handle cases that were previously broken.
a858152
Jonathan Perlow ShadowBase64. 73d2a5b
@avh4
Robolectric member

This pull request is too large to easily review. This will be much more likely to be merged if you could submit smaller pull requests for individual features.

@xian
Robolectric member

@perlow, I'm gonna close this for now, happy to have a look at some smaller pull req's (with tests) if you'd like! Thanks!

@xian xian closed this May 13, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment