rx.Completable support for write operations #629
5 participants
Can you please squash commits? Main reason for squashing — each commit is stable state of the repo (mostly)
| @@ -0,0 +1,18 @@ | ||
| +package com.pushtorefresh.storio.operations; | ||
| + | ||
| +import android.support.annotation.CheckResult; | ||
| +import android.support.annotation.NonNull; | ||
| + | ||
| +import rx.Completable; | ||
| + | ||
| +/** | ||
| + * Common API of prepared write operations | ||
| + * | ||
| + * @param <Result> type of result | ||
| + */ | ||
| +public interface PreparedWriteOperation<Result> extends PreparedOperation<Result> { |
|
Well, this makes Do you have a better name in mind? I did it because I didn't want to force implement No :( I'm actually not sure if we want cc @karlicoss
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
artem-zinnatullin
and 1 other
commented on an outdated diff
| + this.preparedOperation = preparedOperation; | ||
| + } | ||
| + | ||
| + /** | ||
| + * Creates new instance of {@link OnSubscribeExecuteAsBlockingCompletable} | ||
| + * | ||
| + * @param preparedOperation non-null instance of {@link PreparedOperation} which will be used to provide result to subscribers | ||
| + * @return new instance of {@link OnSubscribeExecuteAsBlockingCompletable} | ||
| + */ | ||
| + @NonNull | ||
| + public static Completable.CompletableOnSubscribe newInstance(@NonNull PreparedOperation preparedOperation) { | ||
| + return new OnSubscribeExecuteAsBlockingCompletable(preparedOperation); | ||
| + } | ||
| + | ||
| + @Override | ||
| + public void call(Completable.CompletableSubscriber subscriber) { |
|
nullability (as you think, I understand that it's not presented in RxJava) Just I mean, in StorIO we want to have nullability annotations everywhere, but sometimes you override methods that originally don't have nullability, then you can set
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
| + | ||
| + new PreparedDeleteByQuery.Builder(storIOSQLite, DeleteQuery.builder().table("test_table").build()) | ||
| + .withDeleteResolver(deleteResolver) | ||
| + .prepare() | ||
| + .asRxComleatable() | ||
| + .subscribe(testSubscriber); | ||
| + | ||
| + testSubscriber.awaitTerminalEvent(); | ||
| + testSubscriber.assertNoValues(); | ||
| + testSubscriber.assertError(StorIOException.class); | ||
| + | ||
| + //noinspection ThrowableResultOfMethodCallIgnored | ||
| + StorIOException expected = (StorIOException) testSubscriber.getOnErrorEvents().get(0); | ||
| + | ||
| + IllegalStateException cause = (IllegalStateException) expected.getCause(); | ||
| + assertThat(cause).hasMessage("test exception"); |
|
Maybe let's check that exception is
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
karlicoss
and 2 others
commented on an outdated diff
| @@ -126,6 +128,25 @@ public DeleteResult executeAsBlocking() { | ||
| } | ||
| /** | ||
| + * Creates {@link Completable} which will perform Delete Operation lazily when somebody subscribes to it. | ||
| + * <dl> | ||
| + * <dt><b>Scheduler:</b></dt> | ||
| + * <dd>Operates on {@link Schedulers#io()}.</dd> | ||
| + * </dl> | ||
| + * | ||
| + * @return non-null {@link Completable} which will perform Delete Operation. | ||
| + */ | ||
| + @NonNull | ||
| + @CheckResult | ||
| + @Override | ||
| + public Completable asRxComleatable() { |
|
oh pipec, @geralt-encore we need tests for method names :D
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
Nice work
Yup, sorry, I'll try to finish #630 today (blocked by GitHub organization access settings…)
Messed up :)
You need to perform git rebase master on your branch, just make sure that your master is up-to-date
Current coverage is 82.07%
Merging #629 into master will increase coverage by +0.27% as of
ff9c283
@@ master #629 diff @@
======================================
Files 84 85 +1
Stmts 2484 2522 +38
Branches 323 323
Methods 0 0
======================================
+ Hit 2032 2070 +38
Partial 47 47
Missed 405 405 Review entire Coverage Diff as of
ff9c283Powered by Codecov. Updated on successful CI builds.
| +package com.pushtorefresh.storio.operations.internal; | ||
| + | ||
| +import android.support.annotation.NonNull; | ||
| + | ||
| +import com.pushtorefresh.storio.StorIOException; | ||
| +import com.pushtorefresh.storio.operations.PreparedOperation; | ||
| + | ||
| +import rx.Completable; | ||
| + | ||
| +/** | ||
| + * Required to avoid problems with ClassLoader when RxJava is not in ClassPath | ||
| + * We can not use anonymous classes from RxJava directly in StorIO, ClassLoader won't be happy :( | ||
| + * <p> | ||
| + * For internal usage only! | ||
| + */ | ||
| +public final class OnSubscribeExecuteAsBlockingCompletable implements Completable.CompletableOnSubscribe { |
|
We need to cover this class with tests! Yay, thanks to coverage service! I am not sure how and what to test in this class since both of
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
| + | ||
| +import android.support.annotation.NonNull; | ||
| + | ||
| +import com.pushtorefresh.storio.StorIOException; | ||
| +import com.pushtorefresh.storio.operations.PreparedOperation; | ||
| + | ||
| +import rx.Completable; | ||
| + | ||
| +/** | ||
| + * Required to avoid problems with ClassLoader when RxJava is not in ClassPath | ||
| + * We can not use anonymous classes from RxJava directly in StorIO, ClassLoader won't be happy :( | ||
| + * <p> | ||
| + * For internal usage only! | ||
| + */ | ||
| +public final class OnSubscribeExecuteAsBlockingCompletable implements Completable.CompletableOnSubscribe { | ||
| + |
|
Just regular unit testing, check that it executes operation correctly, handles errors and does Rx stuff right
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
| +public class OnSubscribeExecuteAsBlockingCompletableTest { | ||
| + | ||
| + @SuppressWarnings("ResourceType") | ||
| + @Test | ||
| + public void shouldExecuteAsBlockingAfterSubscription() { | ||
| + final PreparedWriteOperation preparedOperation = mock(PreparedWriteOperation.class); | ||
| + | ||
| + TestSubscriber testSubscriber = new TestSubscriber(); | ||
| + | ||
| + Completable.create(OnSubscribeExecuteAsBlockingCompletable.newInstance(preparedOperation)) | ||
| + .subscribe(testSubscriber); | ||
| + | ||
| + testSubscriber.assertNoErrors(); | ||
| + testSubscriber.assertCompleted(); | ||
| + | ||
| + verify(preparedOperation).executeAsBlocking(); |
|
Can you please also check that it's not invoked before It's okay to do it in a separate test if you want :)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
Awesome, just one more test case and I'm merging this, @geralt-encore you're machine!
cc @nikitin-da, @karlicoss any input from you guys?
| +import static org.mockito.Mockito.mock; | ||
| +import static org.mockito.Mockito.times; | ||
| +import static org.mockito.Mockito.verify; | ||
| +import static org.mockito.Mockito.verifyZeroInteractions; | ||
| +import static org.mockito.Mockito.when; | ||
| + | ||
| +public class OnSubscribeExecuteAsBlockingCompletableTest { | ||
| + | ||
| + @SuppressWarnings("ResourceType") | ||
| + @Test | ||
| + public void shouldExecuteAsBlockingAfterSubscription() { | ||
| + final PreparedWriteOperation preparedOperation = mock(PreparedWriteOperation.class); | ||
| + | ||
| + TestSubscriber testSubscriber = new TestSubscriber(); | ||
| + | ||
| + verifyZeroInteractions(preparedOperation); |
|
This makes no sense because you don't pass it anywhere so it won't be invoked, can you move this line after Happens to everybody :)
On Fri, 18 Mar 2016, 00:07 Ilya, <notifications@github.com> wrote:
In
storio-common/src/test/java/com/pushtorefresh/storio/operations/internal/OnSubscribeExecuteAsBlockingCompletableTest.java
<#629 (comment)>:
> +import static org.mockito.Mockito.mock;
> +import static org.mockito.Mockito.times;
> +import static org.mockito.Mockito.verify;
> +import static org.mockito.Mockito.verifyZeroInteractions;
> +import static org.mockito.Mockito.when;
> +
> +public class OnSubscribeExecuteAsBlockingCompletableTest {
> +
> + @SuppressWarnings("ResourceType")
> + @Test
> + public void shouldExecuteAsBlockingAfterSubscription() {
> + final PreparedWriteOperation preparedOperation = mock(PreparedWriteOperation.class);
> +
> + TestSubscriber testSubscriber = new TestSubscriber();
> +
> + verifyZeroInteractions(preparedOperation);
Right, not sure what I was thinking about
—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
<https://github.com/pushtorefresh/storio/pull/629/files/a2d8e90a41cffedb7ca712ab109f4d27eb706e40#r56577872>
--
@artem_zin
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
|
Looks good, but CI =( failed to find target with hash string 'android-23' in: /usr/local/android-sdk
It's weird, I've just changed a couple of minor things in tests and the previous build was ok
Don't worry, looks like it's not only we in trouble, I'll comment when issue on Travis will be resolved
#628