kaushikgopal / RxJava-Android-Samples

Learning RxJava for Android by example
Apache License 2.0
7.55k stars 1.37k forks source link

Tests #38

Closed EGecius closed 7 years ago

EGecius commented 8 years ago

Would you be interested in me writing at least a few tests to confirm RxJava samples provide expected behaviour? Once we have tests in place, we can make other changes, such updating to latest dependencies, with much greater confidence.

This would probably require some refactoring of existing code. I can start with just one example to check if you are happy with the approach.

kaushikgopal commented 8 years ago

Would i be interested?

No not just interested. I would <3 to have this in. This has been on my todo list forever.

Your strategy of starting with just one example sounds perfect. Would really appreciate it.

EGecius commented 8 years ago

great :) Writing tests is my favourite part in programming :)

I have written a first test as an example but received this message when pushing: "remote: Permission to kaushikgopal/RxJava-Android-Samples.git denied to EGecius."

I guess I need granting a permission by you.

EGecius commented 8 years ago

by the way, I am a huge fan of your podcasts. Thank so much for making them, and especially for making them lively and fun too :) love your sense of playfulness on it :)

kaushikgopal commented 8 years ago

Thanks for the kind words EGecius! do appreciate it.

hmm.. that's interesting. you should open up a pull request with github. you might be trying to push directly to the repo. let me know if you're still facing issues.

EGecius commented 8 years ago

I have a separate branch called 'first_test' but can't push it. I get the following message:

git -c diff.mnemonicprefix=false -c core.quotepath=false -c credential.helper=sourcetree push -v --tags --set-upstream origin refs/heads/first_test:refs/heads/first_test Pushing to https://github.com/kaushikgopal/RxJava-Android-Samples.git remote: Permission to kaushikgopal/RxJava-Android-Samples.git denied to EGecius. fatal: unable to access 'https://github.com/kaushikgopal/RxJava-Android-Samples.git/': The requested URL returned error: 403 Completed with errors, see above

EGecius commented 8 years ago

I have no problems pushing to my own repositories. I think I remember last time I had a similar problem, the owner of the repository added me as a contributor and I had no problems after it.

kaushikgopal commented 8 years ago

Pushing to https://github.com/kaushikgopal/RxJava-Android-Samples.git

So that message indicates that you're trying to push directly to the repo without a PR (Pull request). If you take a look at github's instructions here http://goog_1513273970 https://guides.github.com/activities/contributing-to-open-source/ (see the Pull Request section), that should definitely work.

Other people have contributed to this same repo without any permission changes, so there shouldn't technically be a problem with permissions.

I would need to add you as a contributor only if the project was private. Open source public projects don't have that restriction.

Kaushik

On Wed, Jan 27, 2016 at 8:35 AM, Egidijus Gecius notifications@github.com wrote:

I have no problems pushing to my own repositories. I think I remember last time I had a similar problem, the owner of the repository added me as a contributor and I had no problems after it.

— Reply to this email directly or view it on GitHub https://github.com/kaushikgopal/RxJava-Android-Samples/issues/38#issuecomment-175731756 .

EGecius commented 8 years ago

my apologies. I had not forked the project. It makes total sense now :)

PR created

EGecius commented 8 years ago

feel free to make suggestions how to organise code, packages, etc.. I do have strong preferences as long as I can easily write tests, which is my favourite part.

WassimBenltaief commented 8 years ago

That will be good to include some unit testing. What about refactoring as EGecius said ? Then like he did in the last pull request, what can be done is : Either moving the rx observables out of the activities to some helpers and keep the subscribers or moving all to a second layer (Presenter for ex.) and return only emitted data to the view.

marukami commented 8 years ago

I think keeping the subscriber in the fragments is fine. If you move the subscribers into the helper it's self then it becomes harder to inject the TestSubscriber. Making the Observable s side effect free and returning all the transformed data to the view should work pretty well.

kaushikgopal commented 8 years ago

the test look awesome and is definitely in the right direction imho.

Either moving the rx observables out of the activities to some helpers and keep the subscribers or moving all to a second layer (Presenter for ex.) and return only emitted data to the view.

if this was a full blown project that recommendation would definitely be the way i would like to go. that being said, the whole point of this project is to help folks quickly understand how to use an observable. i don't have a great feeling burying the most important parts of the code with more abstraction using helpers/presenters etc.

it does make it a tad bit tricky though, since enabling the writing of tests requires some re-jiggering of the code (thanks Android!).

Maybe just making ConcurrencyWithSchedulersHelper as an internal class of the fragment might just do it ¯(ツ)

i definitely want to explore more less-intrusive ways of adding tests without changing the code.

WassimBenltaief commented 8 years ago

Yes absolutely. internal Helpers classes are more than enough for test purpose.

marukami commented 8 years ago

I wonder if you can use the RxJavaPlugins.getInstance().registerObservableExecutionHook to inject a test subscriber.

kaushikgopal commented 8 years ago

yup, was thinking along the same lines. I implemented something similar in another repo here: https://github.com/kaushikgopal/rem/blob/master/app/src/test/java/co/kaush/rem/ui/list/TaskListControllerTest.java#L42

kaushikgopal commented 7 years ago

heya, was doing some cleanup. my bad for letting this go unattended, for so long.

if you still feel strongly about this PR, chime in. if we mutually agree it's something that needs to go into the repo, we'll have to work towards rebasing the changes again over the latest master.

✌️