-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
considers async option in HandlerScheduler.scheduleDirect #442
Conversation
This needs tests. Are you able to write them? If not, I can get to it sometime in the next week or two. |
oh sorry, i forget writing test... But, I think already HandlerSchedulerTest is considering the async option test by Parameterized test. Are there other missing tests? Please tell me if any missing test. |
Coverage is definitely missing. Either the test should have been failing before without this change or we aren't covering this codepath. Since the test is passing, we have missing coverage on this codepath. |
The parameterized test just expects no regressions. There weren't any cases added that would have failed the the non-async implementation, as I'm not really sure how to coerce vsync behavior out of robolectric :X |
You don't need to. We just need to assert the flag is set through all scheduler code paths. There's already a test which asserts that flag is not set on pre-16 which I added. |
Ah didn't know that was added 😬 |
We just need to add ones which check each scheduler method and assert the flag is set to true on the messages. We should have been doing that anyway... |
thx 😄
i write a test! |
@JakeWharton @hzsweers i added test for async option 😃 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm. Not terribly familiar with those robolectric APIs but not sure of any better ways anyway
Thanks!
…On Fri, Feb 15, 2019 at 7:10 PM Jake Wharton ***@***.***> wrote:
Merged #442 <#442> into 2.x.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#442 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABTEvkIfSusnBGmMZWDeX6l6vaE3hdElks5vNwYsgaJpZM4WaNVE>
.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
io.reactivex.rxjava3:rxandroid:3.0.0
close #441