Add doOnTerminate to Single/Maybe for consistency #6386

skryvets · 2019-01-26T16:15:33Z

This PR adds doOnTerminate operator to Single and Maybe.

Resolves: #6379.

codecov · 2019-01-26T16:30:39Z

Codecov Report

Merging #6386 into 2.x will increase coverage by 0.05%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##                2.x    #6386      +/-   ##
============================================
+ Coverage     98.23%   98.28%   +0.05%     
- Complexity     6287     6299      +12     
============================================
  Files           673      675       +2     
  Lines         45092    45156      +64     
  Branches       6239     6239              
============================================
+ Hits          44297    44383      +86     
+ Misses          256      238      -18     
+ Partials        539      535       -4

Impacted Files	Coverage Δ	Complexity Δ
...x/internal/operators/maybe/MaybeDoOnTerminate.java	`100% <100%> (ø)`	`2 <2> (?)`
src/main/java/io/reactivex/Maybe.java	`100% <100%> (ø)`	`173 <1> (+1)`	⬆️
...internal/operators/single/SingleDoOnTerminate.java	`100% <100%> (ø)`	`2 <2> (?)`
src/main/java/io/reactivex/Single.java	`100% <100%> (ø)`	`149 <1> (+1)`	⬆️
.../operators/observable/ObservableFlatMapSingle.java	`89.55% <0%> (-5.98%)`	`2% <0%> (ø)`
...nternal/operators/parallel/ParallelReduceFull.java	`91.08% <0%> (-3.97%)`	`2% <0%> (ø)`
...tivex/internal/observers/FutureSingleObserver.java	`94.33% <0%> (-3.78%)`	`24% <0%> (-1%)`
.../operators/mixed/FlowableConcatMapCompletable.java	`97.43% <0%> (-2.57%)`	`2% <0%> (ø)`
...perators/mixed/ObservableSwitchMapCompletable.java	`97.89% <0%> (-2.11%)`	`3% <0%> (ø)`
...nternal/operators/observable/ObservableWindow.java	`98% <0%> (-2%)`	`3% <0%> (ø)`
... and 28 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3fbfcc9...3e8284b. Read the comment docs.

akarnokd · 2019-01-26T16:25:28Z

src/main/java/io/reactivex/Maybe.java

@@ -2892,6 +2892,31 @@ public final T blockingGet(T defaultValue) {
        ));
    }

+    /**
+     * Calls the shared consumer with the given onTerminate callback for each


This sentence makes no sense. Please word it like the other similar operators: http://reactivex.io/RxJava/2.x/javadoc/io/reactivex/Completable.html#doOnTerminate-io.reactivex.functions.Action-

akarnokd · 2019-01-26T16:27:27Z

src/test/java/io/reactivex/maybe/MaybeTest.java

@@ -826,6 +827,53 @@ public void accept(Disposable v) throws Exception {
        .assertFailure(TestException.class);
    }

+    @Test(expected = NullPointerException.class)


Please put these unit test into their own files and also verify the cases when the onTerminate callback fail.

Implemented

akarnokd · 2019-01-26T16:27:57Z

src/main/java/io/reactivex/internal/operators/single/SingleDoOnTerminate.java

+                onTerminate.run();
+            } catch (Throwable ex) {
+                Exceptions.throwIfFatal(ex);
+                downstream.onError(ex);


This should be a composite error with the original error followed by the handler crash. Example: https://github.com/ReactiveX/RxJava/blob/2.x/src/main/java/io/reactivex/internal/operators/completable/CompletablePeek.java#L92

Implemented

akarnokd · 2019-01-27T15:59:57Z

src/main/java/io/reactivex/Maybe.java

+     */
+    @CheckReturnValue
+    @NonNull
+    @SchedulerSupport(SchedulerSupport.NONE)


Please add @Experimental.

akarnokd · 2019-01-27T16:00:17Z

src/main/java/io/reactivex/Maybe.java

+     * @return the new Maybe instance
+     * @see <a href="http://reactivex.io/documentation/operators/do.html">ReactiveX operators documentation: Do</a>
+     * @see #doOnTerminate(Action)
+     */


Please add @since 2.2.7 - experimental.

akarnokd · 2019-01-27T16:00:47Z

src/main/java/io/reactivex/Single.java

+     * @return the new Single instance
+     * @see <a href="http://reactivex.io/documentation/operators/do.html">ReactiveX operators documentation: Do</a>
+     * @see #doOnTerminate(Action)
+     * @since 2.0


Please use the next minor version: @since 2.2.7 - experimental.

akarnokd · 2019-01-27T16:00:58Z

src/main/java/io/reactivex/Single.java

+    @CheckReturnValue
+    @NonNull
+    @SchedulerSupport(SchedulerSupport.NONE)
+    public final Single<T> doOnTerminate(final Action onTerminate) {


Please add @Experimental.

akarnokd · 2019-01-27T16:03:41Z

src/test/java/io/reactivex/internal/operators/maybe/MaybeDoOnTerminateTest.java

+            public void run() {
+                atomicBoolean.set(true);
+            }
+        }).subscribe();


You should use .test().assertResult(1); instead of just subscribing blindly.

akarnokd · 2019-01-27T16:03:56Z

src/test/java/io/reactivex/internal/operators/maybe/MaybeDoOnTerminateTest.java

+                atomicBoolean.set(true);
+            }
+        })
+        .subscribe();


You should use .test().assertResult(1); instead.

akarnokd · 2019-01-27T16:04:11Z

src/test/java/io/reactivex/internal/operators/single/SingleDoOnTerminateTest.java

+            public void run() throws Exception {
+                atomicBoolean.set(true);
+            }
+        }).subscribe();


You should use .test().assertResult(1); instead.

akarnokd · 2019-01-27T16:04:31Z

src/test/java/io/reactivex/internal/operators/single/SingleDoOnTerminateTest.java

+            }
+        });
+
+        try {


Why the testing style change?

Refactored to match the rest of the testing style

skryvets · 2019-01-28T00:05:26Z

src/test/java/io/reactivex/internal/operators/maybe/MaybeDoOnTerminateTest.java

+            }
+        })
+        .test()
+        .assertResult();


@akarnokd, as you suggested I've tried to use .assertResult(1) which results in the test failure. I found similar pattern here, where assertResult() has no params since Maybe doesn't emit a value. Please correct me if I'm wrong.

Think about it a bit. You have an empty source and you want to verify it is still empty at the end.

akarnokd · 2019-01-28T10:10:35Z

src/test/java/io/reactivex/internal/operators/single/SingleDoOnTerminateTest.java

+            }
+        })
+        .test()
+        .assertFailure(CompositeException.class);


Please verify that the correct inner exceptions are received. Example: https://github.com/ReactiveX/RxJava/blob/2.x/src/test/java/io/reactivex/internal/operators/completable/CompletableDoOnTest.java#L52

akarnokd · 2019-01-28T10:10:48Z

src/test/java/io/reactivex/internal/operators/maybe/MaybeDoOnTerminateTest.java

+            }
+        })
+        .test()
+        .assertFailure(CompositeException.class);


Please verify that the correct inner exceptions are received. Example: https://github.com/ReactiveX/RxJava/blob/2.x/src/test/java/io/reactivex/internal/operators/completable/CompletableDoOnTest.java#L52

skryvets · 2019-01-29T01:46:21Z

src/test/java/io/reactivex/internal/operators/maybe/MaybeDoOnTerminateTest.java

+            }
+        })
+        .test()
+        .assertNoValues();


I came to a conclusion that assertNoValues() would be the right choice. I also considered assertEmpty() method, but this one will not work as onComplete event is triggered.

Thanks for pointing me in a right direction.

AssertNoValues doesn't verify the lack of errors. Please put assertResult back.

I thought your previous comment mentioned that it was a wrong approach. Maybe I misunderstood. Is it still correct to use assertResult()?

I put it back anyways according to your last review.

You misunderstood me. If a source has no values, there is no reason to put any value into the parameter of assertResult(). A no-arg assertResult verifies exactly that the outcome is no items and no errors.

Got it. Sorry for the for the back and forth from my side.

skryvets · 2019-01-29T11:02:50Z

All improvements and fixes have been implemented

akarnokd requested changes Jan 26, 2019

View reviewed changes

akarnokd requested changes Jan 27, 2019

View reviewed changes

skryvets commented Jan 28, 2019

View reviewed changes

akarnokd added Enhancement 2.x labels Jan 28, 2019

akarnokd added this to the 2.2 backlog milestone Jan 28, 2019

akarnokd reviewed Jan 28, 2019

View reviewed changes

skryvets commented Jan 29, 2019

View reviewed changes

Add doOnTerminate to Single/Maybe for consistency (#6379)

3e8284b

akarnokd approved these changes Jan 29, 2019

View reviewed changes

vanniktech approved these changes Jan 29, 2019

View reviewed changes

akarnokd merged commit 6e266af into ReactiveX:2.x Jan 29, 2019

akarnokd mentioned this pull request Feb 19, 2019

2.2.7 release preparations #6412

Closed

+                          }
+                      });
+                      try {

Add doOnTerminate to Single/Maybe for consistency #6386

Add doOnTerminate to Single/Maybe for consistency #6386

Conversation

skryvets commented Jan 26, 2019

codecov bot commented Jan 26, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skryvets Jan 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skryvets commented Jan 29, 2019

codecov bot commented Jan 26, 2019 •

edited

Loading

skryvets Jan 27, 2019 •

edited

Loading