-
Notifications
You must be signed in to change notification settings - Fork 226
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
Add MainThreadDisposable to allow disposing on the JVM #232
Conversation
3856b30
to
812cf12
Compare
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.
Thanks for catching this. I wonder if it's worth proposing adding this main thread check plugin upstream to RxAndroid directly, since I'm sure this comes up anywhere MainThreadDisposable
is used
@@ -0,0 +1,58 @@ | |||
/* | |||
* Copyright (c) 2017. Uber Technologies |
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.
Since this is from rxandroid, let's preserve the header from it
/** | ||
* Copy of the MainThreadDisposable from RxAndroid which makes use of the | ||
* {@link AutoDisposeAndroidUtil} to check for main thread. This allows | ||
* disposing on the JVM crashing due to the looper check. |
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.
Nit: I think this sentence is missing a word?
|
||
import java.util.concurrent.atomic.AtomicBoolean; | ||
|
||
import static org.junit.Assert.assertTrue; |
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.
Nit: mind using Truth instead like the other tests?
protected void onDispose() { } | ||
}.dispose(); | ||
throw new AssertionError("Expected to fail before this due to Looper not being stubbed!"); | ||
} catch (RuntimeException e) { |
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.
Could we make this more specific? Since this is a super specific test case, I'm kind of fine with having something specific. Even just something simple like "contains "stub"" or something simple to smoke test it
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.
nvm, saw it's copied
}.dispose(); | ||
|
||
assertTrue(called.get()); | ||
AutoDisposeAndroidPlugins.setOnCheckMainThread(null); |
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.
Nit: just put this in a before/after block in the test
@Before @After public void resetPlugins() {
AutoDisposeAndroidPlugins.reset();
}
@@ -0,0 +1,60 @@ | |||
/* | |||
* Licensed under the Apache License, Version 2.0 (the "License"); |
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.
nit: this should mention uber
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.
nvm, saw it's copied too
|
||
public final class MainThreadDisposableTest { | ||
|
||
@Test public void onDisposeRunsSyncWhenMainThreadSkipped() { |
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.
This title is a little unclear to me. How about onDispose_defersToMainThreadHook
?
Finishing this up in #235 |
Description:
Due to
LifecycleEventsObservable
extending theMainThreadDisposable
fromRxAndroid
it was not possible to test disposing of a subscription on the JVM.io.reactivex.android.MainThreadObservable
also contains a checkLooper.myLooper() == Looper.getMainLooper()
which will throw an exception in JVM tests.Using a local copy of
MainThreadObservable
which utilizes theAutoDisposeAndroidUtil.isMainThread()
you will get more consistent "main thread check" behavior.The unit tests that are now included only test new behavior and not the rest of the behavior of
MainThreadObservable
. First wanted to copy over tests from rxandroid, but then saw that those are robolectric, which is not a dependency in this project.