-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Fix UseCaseTest #139
Conversation
amatkivskiy
commented
Apr 12, 2016
@jordifierro same thing with testSubscriptionWhenExecutingUseCase test case.
jordifierro
commented
Apr 12, 2016
Thanks @amatkivskiy, you are right!
amatkivskiy
commented
Apr 12, 2016
@jordifierro and I think we need to rethink the way the UseCase is tested.
jordifierro
commented
Apr 12, 2016
What do you mean @amatkivskiy?
amatkivskiy
commented
Apr 12, 2016
@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.
jordifierro
commented
Apr 12, 2016
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!
@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.
jordifierro
commented
Apr 27, 2016
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.
amatkivskiy
commented
Apr 27, 2016
@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).
jordifierro
commented
Apr 27, 2016
Considering the nature of this specific project, you are right @amatkivskiy. I will work on this.
amatkivskiy
commented
Apr 27, 2016
@jordifierro Thanks)
jordifierro
commented
Apr 27, 2016
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!
The
testBuildUseCaseObservableReturnCorrectResulttest fromUseCaseTestis not testing correctly the execute method from theUseCaseclass. It's not a crucial test but it's a good example of how to test RxJava subscriptions.