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

Initial support for scheduling on Android Handler threads #318

Merged
merged 10 commits into from
Aug 23, 2013

Conversation

mttkay
Copy link
Contributor

@mttkay mttkay commented Aug 14, 2013

To add to the discussion in #317, this is pretty much what we're using in our production app since a few weeks now. It used to live in our app project, but since we've already started talking about adding to rxjava-contrib, I've pulled out the code in question and integrated it into RxJava under rxjava-android.

This comprises:

  • a Scheduler implementation which schedules work on Android's Handler threads, including the main UI thread.
  • an AndroidSchedulers class which provides factory methods to instantiate Handler schedulers, especially the main thread scheduler
  • unit test support via Robolectric
  • build integration

There is some discussion needed around both test support and build integration. @mustafasezgin will want to weigh in on this. In order to write Android unit tests, you're unfortunately required to take a few obstacles, so we had to add some cruft to the production code (an empty Android R class and a custom test runner.)

@cloudbees-pull-request-builder

RxJava-pull-requests #196 FAILURE
Looks like there's a problem with this pull request

apply plugin: 'eclipse'
apply plugin: 'idea'
apply plugin: 'osgi'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I basically copied this file over from the Swing contrib module. There's a lot of cruft in here which I think can be remove or at least shared with the parent modules? I'm thinking about IDE project file generation and JavaDoc setup.

Copy link
Member

Choose a reason for hiding this comment

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

Yes that's probably the case, I have not yet attempted doing that as I'm not the most comfortable with Gradle.

@cloudbees-pull-request-builder

RxJava-pull-requests #197 FAILURE
Looks like there's a problem with this pull request

@muzzah
Copy link

muzzah commented Aug 14, 2013

Unit testing in Android land is a bit tricky as the sources for android are all stubs which throw exceptions. There are quite a few final methods that exist as well in many components making mocking not possible with libraries like mockito. As a result a framework which is popular called Robolectric has tried to help developers by providing a barebones implementation of the android sdk (using some classloader magic) which is helpful when wanting to write unit tests (Other option is to write integration tests which run on device which is slow and not useful in this instance).

Why is this relevant in this case? To get robolectric up and running we need some default folders and files along with a custom test runner. To be consistent with the rest of rx core we have to put these in the main src package but really we should be not shipping these folders/files with the release artifact. So one solution is to have a custom build step to strip these out (which is probably preferred) and the other is to break convention and have a separate test directory. @benjchristensen would be good to get your thoughts on how you would want to move forward with this.

@benjchristensen
Copy link
Member

@mustafasezgin go ahead and put the unit tests in /src/test if that's what makes sense for this module.

I mix and match as needed. The RxJava build files should pick up tests in both /src/main and /src/test and run them via the ./gradlew build scripts already.

Also, we should be stripping *UnitTest files from the published artifacts but aren't right now because of an odd issue with Scala. That will be solved when we start publishing different artifacts for different languages once this pull request finally hits: #304

In short, put your tests where you need/want in this rxjava-android module.

@benjchristensen
Copy link
Member

@mttkay and @mustafasezgin Is there anything else needed before merging and releasing this?

@prabirshrestha Since you had a similar pull request can you weigh in on whether this is meeting your needs and functions correctly?

Anyone else with Android experience able to comment?

@prabirshrestha
Copy link
Contributor

Looks good to me.

@muzzah
Copy link

muzzah commented Aug 15, 2013

@benjchristensen Just to respond to your comments, to keep close to existing convention I will keep the tests where they are but will try to move the test driver and empty R file into the test package so they are not bundle with the release artifact. Will try to get that in tonight and then we should be good to go .We actually have a few more components to add but they need a little more cleanup before they are ready...

@benjchristensen
Copy link
Member

Sounds good, I'll await your final go ahead before merging.

Excluded the test support package from being included when building a jar or generating javadoc
Javadoc comments added
@cloudbees-pull-request-builder

RxJava-pull-requests #198 FAILURE
Looks like there's a problem with this pull request

@muzzah
Copy link

muzzah commented Aug 17, 2013

@benjchristensen I have modified the build script to exclude the test support package at jar build time and doc generation time by adding the exclude option to the gradle tasks. If this looks good to you then happy to have it merged.

@cloudbees-pull-request-builder

RxJava-pull-requests #199 FAILURE
Looks like there's a problem with this pull request

@cloudbees-pull-request-builder

RxJava-pull-requests #200 FAILURE
Looks like there's a problem with this pull request

@muzzah
Copy link

muzzah commented Aug 21, 2013

@benjchristensen Just to let you know this is all ready to be merged from my perspective.

@benjchristensen
Copy link
Member

Great, thank you @mustafasezgin I will aim to have this done this week. Also working on getting #319 out the door which will benefit Android.

@benjchristensen
Copy link
Member

This builds successfully and unit tests pass on my machine so I'm merging this.

benjchristensen added a commit that referenced this pull request Aug 23, 2013
Initial support for scheduling on Android Handler threads
@benjchristensen benjchristensen merged commit 87fd889 into ReactiveX:master Aug 23, 2013
@benjchristensen
Copy link
Member

I'm held up on releasing this while dealing with an internal build issue (that posts to Maven Central) handling the new com.google.android and org.roboelectric dependencies.

@mttkay
Copy link
Contributor Author

mttkay commented Aug 23, 2013

Do you need help with this? What's the problem?

@benjchristensen
Copy link
Member

No, it's an internal issue ... it's related to how the Netflix build system supports multiple different repos and expects the dependencies from Maven Central to also be available in the internal repo ... which is a headache for this type of thing as these dependencies have never been seen by it before.

I know what I need to do, just ran out of time last night ... will try to deal with it today.

@benjchristensen
Copy link
Member

This is released in 0.10.1 making its way to Maven Central now.

@benjchristensen
Copy link
Member

Announced on Twitter at https://twitter.com/RxJava/status/371124391590887424

@benjchristensen
Copy link
Member

I've added a README at https://github.com/Netflix/RxJava/blob/master/rxjava-contrib/rxjava-android/README.md

It would be helpful to show people usage examples and some documentation. Can one of you contribute any of that?

Here are examples of contrib modules in the Hystrix project with README files if you want to model after any of them:

Would probably make sense to have something on the wiki as well that at least links to the README.

@mttkay
Copy link
Contributor Author

mttkay commented Aug 24, 2013

Great, thanks Ben! I'll look after the documentation.

rickbw pushed a commit to rickbw/RxJava that referenced this pull request Jan 9, 2014
Initial support for scheduling on Android Handler threads
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

5 participants