Adding TestLifecycleScopeProvider. #48
Conversation
|
|
||
| @Override | ||
| public Object peekLifecycle() { | ||
| return lifecycleSubject.getValue(); |
There was a problem hiding this comment.
Does this block or return null?
| return TestLifecycle.STOPPED; | ||
| case STOPPED: | ||
| throw new LifecycleEndedException(); | ||
| default: |
There was a problem hiding this comment.
This can be removed and just throw at the end
There was a problem hiding this comment.
I think it's worth having a distinction between the lifecycle actually ending versus some random unknown event.
(Side note: I wish Java had exhaustive switches - can just avoid this entirely.)
| */ | ||
| public final class TestLifecycleScopeProvider implements LifecycleScopeProvider { | ||
|
|
||
| private final BehaviorSubject<TestLifecycle> lifecycleSubject = BehaviorSubject.create(); |
There was a problem hiding this comment.
Would it be worthwhile to have a constructor that initializes this with an event?
There was a problem hiding this comment.
Sure, I can make an UNKNOWN event types. It adds a bit of complexity for the corresponding event function, but it's better than having a subject with nothing.
|
Since this doesn't actually require any test dependencies, what would you think of just including it in the main artifact? Similar to how RxJava includes test tools |
I think that's fine if this is the only test utility (+ moving the one that exists internally for |
|
I think I'd rather keep it simple for now and separate it if need be. Between kotlin, android, and the main artifact we already have a few. |
| lifecycleSubject.onNext(TestLifecycle.STOPPED); | ||
| } | ||
|
|
||
| private enum TestLifecycle { | ||
| enum TestLifecycle { | ||
| UNINITIALIZED, |
There was a problem hiding this comment.
I don't think we want to promote this. peekLifecycle() expects null if not started, and autodispose should handle the LifecycleNotStartedException throwing under the hood in accordance
There was a problem hiding this comment.
Sorry, misinterpreted your comment above.
| /** | ||
| * @return a new {@link TestLifecycleScopeProvider} instance. | ||
| */ | ||
| public static TestLifecycleScopeProvider create() { |
There was a problem hiding this comment.
Do we want overloads for initial state?
| * @return a new {@link TestLifecycleScopeProvider} instance with {@param initialValue} as its initial lifecycle | ||
| * event. | ||
| */ | ||
| public static TestLifecycleScopeProvider createInitial(TestLifecycle initialValue) { |
There was a problem hiding this comment.
Should we keep an empty create() still?
There was a problem hiding this comment.
I think it's still useful, I can see a lot of cases where you want to create one without necessarily putting it in a started state.
There was a problem hiding this comment.
Oh oops, I thought you'd replaced it with this, didn't see you'd just added this :P. LGTM
|
I was planning on just porting the one you wrote in another PR if that works. |
|
That's fine |
#45
This isn't quite ready for merging, but I wanted to open a PR to discuss the API for
TestLifecycleScopeProvider.