Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Fix UseCaseTest #139

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

Open
jordifierro wants to merge 3 commits into android10:master
base: master
Choose a base branch
Loading
from jordifierro:fix-usecasetest
Open

Conversation

Copy link

@jordifierro jordifierro commented Apr 11, 2016

The testBuildUseCaseObservableReturnCorrectResult test from UseCaseTest is not testing correctly the execute method from the UseCase class. It's not a crucial test but it's a good example of how to test RxJava subscriptions.

Copy link

@jordifierro same thing with testSubscriptionWhenExecutingUseCase test case.

Copy link
Author

Thanks @amatkivskiy, you are right!

Copy link

@jordifierro and I think we need to rethink the way the UseCase is tested.

Copy link
Author

What do you mean @amatkivskiy?

Copy link

@jordifierro I meant that your approach with DefaultThreadExecutor made UseCaseTest work correctly. Previously the tests were fake passing. For example you can comment line 63 in previous version of the UseCaseTest and nothing changes.

I am telling this because while implementing #140 I need to write tests for it.

Copy link
Author

Sorry @amatkivskiy, I asked because I thought you wanted to change the way of testing UseCase (not just fix the tests). As you say, tests were always passing and I just wanted to fix that here, not try to modify or improve them.

And about your pull request, feel free to use the same approach used here if you don't want to wait for this pull request, no problem for me!

Copy link

amatkivskiy commented Apr 27, 2016
edited
Loading

@jordifierro Recently I've faced the problem that TestScheduler executes the onNext of the subscriber NOT in the thread that test run in.
To reproduce this scenario replace simple Observable.just(1, 2, 3) with some delayed Observable.delay(). As the result TestSubscriber wont wait for the observable to finish.
The way to hold this scenario one needs to add testSubscriber.awaitTerminalEvent(); before asserting the subscriber but this leads the current thread to be blocked. I tried to use Schedulers.immediate() instead of TestScheduler.

Copy link
Author

Thanks for commenting @amatkivskiy!

I've faced a similar problem when testing my repositories against a MockWebServer (I've written about it here) and, as you say, I've solved it adding testSubscriber.awaitTerminalEvent(); before the asserts to wait for the mocked server response.

However, in my opinion, the goal of this test is to check that the subscriber receives whatever from wherever is emitted through the data stream after buildUseCaseObservable() method from the UseCase child class is called, so I see no point in testing deeply here.

Copy link

@jordifierro my point is that people will see this tests and will try to apply the same approach in their tests and it will fail because it works only for this situation (I mean testing base UseCase when result is return immediately).

Copy link
Author

Considering the nature of this specific project, you are right @amatkivskiy. I will work on this.

amatkivskiy reacted with thumbs up emoji

Copy link

@jordifierro Thanks)

Copy link
Author

After all, the update hasn't made the test more complex nor larger (which I didn't want) so it's fine. The Schedulers.immediate() you told me did the trick.

@amatkivskiy Thanks to you for your help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /