Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Redesign database related shadows #800

Merged
merged 24 commits into from
Dec 10, 2013
Merged

Conversation

roman-mazur
Copy link
Contributor

Removes a lot of shadows connected with SQLiteDatabase and Cursor, makes Robolectric behaviour much closer to how real Android behaves.

Instead of a bunch of removed shadows we introduce implementations of SQLiteConntection and CursorWindow based on sqlite4java. This approach let us drop a lot of shadowing code and use real Android classes. However both implemented classes belong to a hidden Android API, which means we should be careful when add some new Android API level to what is supported by Robolectric (especially adding 2.x APIs).

Worth to mention in release notes:

  • SQLiteDatabase#openDatabase opens on-disk database until its path parameter is set with ":memory:" value;
  • DatabaseConfig annotation is gone
  • some methods from the shadowed db are gone:
    • in SQLiteDatabase: setThrowOnInsert, hasOpenCursors, getQuerySql, getConnection

History notes

Some time ago I faced with some inconsistencies in shadows of cursors and started digging first removing shadow of matrix cursor and then...
This request is not a merge candidate right now but rather a proposal.

I'm playing with removing cursor shadows completely and shadowing only SQLiteConnection and CursorWindow instead (ShadowSQLiteConnection, ShadowCursorWindow).

I've already made some progress in this direction and have some of database related tests passing. And now I'd like someone from the main team to take a look at the approach and start discussion. Perhaps it's not the way worth to be implemented.

Currently I have 2 main points that do not allow me to sleep sweetly:

So please take a look and tell what you think: should I go on or would I rather stop, delete this branch, and forget :).

@roman-mazur
Copy link
Contributor Author

cc @jberkel

@jberkel
Copy link
Collaborator

jberkel commented Oct 31, 2013

i think this is the right way forward, there are way too many shadows in the current db code. if it helps it is definitely worth dropping the jdbc layer too - we don't need to support any other databases than sqlite anyway.

i wouldn't worry about backwards compatibility, although i don't see how this would be an issue here since the connection is never exposed to the clients?

@roman-mazur
Copy link
Contributor Author

As for older Android versions. To illustrate, look at SQLQuery in Android 2.3. There is no such class as SQLiteConnection and native methods are called directly from SQLQuery.

What will we be able to do when Robolectric tests are run with old Android code? AFAIK this is a planned feature (choosing API level to run tests).
Looks like we'll have to implement different shadowing for different versions.

@coreydowning
Copy link
Collaborator

I think this is a great idea.

As for compatibility with older versions of Android, I think this is a bridge that can be crossed when we get there. At one point (can't remember how much is still there) we did have some code in the 4.3 work that would do slightly different things on 4.3 and 4.1, so we can certainly come up with a solution.

To quote everyone working on Robolectric this year, "Death to shadows!!" :rage4:

@roman-mazur
Copy link
Contributor Author

Ok, then I'll go on with picking some SQLite java wrapper and push here as soon as it is wired.

@roman-mazur
Copy link
Contributor Author

I switched to sqlite4java.
I also found another project on github allowing to get native libs from maven without poluting our pom file.
However their native library loader didn't work (they rely on CodeSource location in order to find path to packaged binaries, which was returning null location on my machine).
Thus I came up with my own SQLiteLibraryLoader and a corresponding test.

Working on getting more tests passing... Currently the only function I could not implement with the wrapper is collator creation.

@dant3
Copy link

dant3 commented Nov 5, 2013

Why not just use Xerial SQLite JDBC Driver?
See https://bitbucket.org/xerial/sqlite-jdbc
It contains natives inside it's jar and it automagically loads them.

@roman-mazur
Copy link
Contributor Author

@dant3 That was the first thing I was looking at. Yet it does not seem to fit our needs.
JDBC interfaces do not provide all the functionality we require to implement SQLite connection shadow (like getting result columns count before actually executing a query).

@roman-mazur
Copy link
Contributor Author

Am I allowed to remove DatabaseConfig, SQLiteMap classes?

@roman-mazur
Copy link
Contributor Author

Well, looks like I've finally managed to have all the tests passing.
Yet my native libraries loader doesn't work well enough and tests in real project using this robolectric version fail being unable to load binaries.

@roman-mazur
Copy link
Contributor Author

I will try to resolve the issue next week but would be grateful if somebody else looked at it too.

@roman-mazur
Copy link
Contributor Author

This PR also brings some changes to public Robolectric API. Please review how tests are changed.
Looking forward to feedback.

@jberkel
Copy link
Collaborator

jberkel commented Nov 7, 2013

i'll take a look at it soon

@jberkel
Copy link
Collaborator

jberkel commented Nov 10, 2013

i tried your branch with one of my projects and the native libs got loaded fine (on osx, using maven). however i could not run tests from within intellij - this is fixed in 7dba412

what native platform are you on?

i have some more test failures with NPEs in nativeGetLong / nativeGetDouble, investigating at the moment.
edit: fixed in ed426b8

@roman-mazur
Copy link
Contributor Author

I'm on osx too. I'll try your branch tomorrow.

@roman-mazur
Copy link
Contributor Author

I've finally resolved my issue with loading natives. I'm using Gradle and it does not resolve maven dependencies from profile section in POM. http://issues.gradle.org/browse/GRADLE-1997
After adding a necessary dependency everything started to work.
I started considering whether we should pack all the binaries with robolectric jar rather than rely on maven and profiles. What do you think?

So I'm currently trying this branch with a project which I was working on some time ago. And in this project there are some tests that run query operations in an AsyncTask which looper is not paused. As a result SQLiteConnection is created in one thread and used from another, which causes an error (I've added some code to fail fast in such case 60a70e6).

I think I'll make native SQLiteConnection to be a thread local variable (actually Android's SQLiteConnection is thread local too).

@roman-mazur
Copy link
Contributor Author

With another project I've worked recently everything works well except the fact that I had to fix MatrixCursor usage in test (after removing the shadow it's not allowed to call addRow with array of length different from the columns count).

@jberkel
Copy link
Collaborator

jberkel commented Nov 12, 2013

let's move all the native libraries into one jar file, similar to what xerial-jdbc does. would be good if users don't have to worry about which platform specific dependencies they have to pull in. or does gradle have something similar to mavennatives ?

<version>3.7.2</version>
<groupId>com.github.axet</groupId>
<artifactId>litedb</artifactId>
<version>0.2.2</version>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0.2.3 is the latest published version

@roman-mazur
Copy link
Contributor Author

Rebased. Logging muted.

jberkel added a commit that referenced this pull request Dec 10, 2013
Redesign database related shadows
@jberkel jberkel merged commit b19e35c into robolectric:master Dec 10, 2013
@roman-mazur
Copy link
Contributor Author

hooray!
🎉 🎆

@roman-mazur roman-mazur deleted the cursor branch December 10, 2013 09:50
@ChristianKatzmann
Copy link
Contributor

Nice job!

@erd
Copy link
Member

erd commented Dec 12, 2013

For some reason, the SQLiteLoader tests fail on my MacBook Air, but work fine on the iMacs we have at work. I have no idea why there would be a difference. I'll see if I can track it down and/or post a stacktrace of the failures.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants