Add assertValueAt(int, value) to TestObserver #5529

NickFirmani · 2017-07-31T21:54:25Z

I found myself frequently writing tests that want to check that an observable emitted a certain value second or third, but I don't care what the first or other values one, so assertValues(Object...)) is more brittle.

This PR implements an assertValueAt(index, value) method, similar to assertValue(value). Tests are also added.

akarnokd · 2017-07-31T22:10:25Z

There is an assertValueAt(int, Predicate) just below your changes, what's wrong with that? Also there could be an overload problem because of a T - Predicate conflict in Java 8+.

NickFirmani · 2017-07-31T22:46:40Z

I personally make heavy use of the assertValue(T) method, BaseConsumer#324, which also has a similar paired method assertValue(Predicate<T>). Using that method keeps my tests cleaner, when I know the exact value I expect. It seems pretty verbose to me to implement a Predicate when the body will just be value.equals(expected).

If this causes an overload problem, we'll have the same problem with assertValue(T) and assertValue(Predicate<T>) as well

codecov · 2017-07-31T23:26:51Z

Codecov Report

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

@@             Coverage Diff             @@
##                2.x   #5529      +/-   ##
===========================================
+ Coverage     96.14%   96.2%   +0.05%     
- Complexity     5806    5821      +15     
===========================================
  Files           631     631              
  Lines         41285   41414     +129     
  Branches       5732    5740       +8     
===========================================
+ Hits          39693   39841     +148     
+ Misses          629     621       -8     
+ Partials        963     952      -11

Impacted Files	Coverage Δ	Complexity Δ
.../java/io/reactivex/observers/BaseTestConsumer.java	`99.65% <100%> (+0.04%)`	`112 <4> (+4)`	⬆️
...l/operators/observable/ObservableFlatMapMaybe.java	`84.96% <0%> (-10.46%)`	`2% <0%> (ø)`
...tivex/internal/schedulers/InstantPeriodicTask.java	`61.11% <0%> (-5.56%)`	`7% <0%> (-2%)`
.../operators/observable/ObservableFlatMapSingle.java	`91.79% <0%> (-4.48%)`	`2% <0%> (ø)`
...rnal/operators/flowable/FlowableFlatMapSingle.java	`94.02% <0%> (-2.72%)`	`2% <0%> (ø)`
...ternal/operators/completable/CompletableMerge.java	`96.42% <0%> (-2.39%)`	`2% <0%> (ø)`
...rnal/operators/flowable/FlowableSkipLastTimed.java	`95.91% <0%> (-2.05%)`	`2% <0%> (ø)`
...java/io/reactivex/processors/PublishProcessor.java	`98.29% <0%> (-1.71%)`	`45% <0%> (-1%)`
...internal/operators/completable/CompletableAmb.java	`96.61% <0%> (-1.7%)`	`10% <0%> (-1%)`
...rnal/subscriptions/DeferredScalarSubscription.java	`98.46% <0%> (-1.54%)`	`28% <0%> (-1%)`
... and 75 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 ac06c24...bfbd517. Read the comment docs.

akarnokd · 2017-08-01T07:42:40Z

/cc @vanniktech, @artem-zinnatullin

artem-zinnatullin

LGTM, I would use that!

artem-zinnatullin · 2017-08-01T07:47:15Z

src/test/java/io/reactivex/observers/TestObserverTest.java

+
+        thrown.expect(AssertionError.class);
+        thrown.expectMessage("Expected: 2 (class: Integer), Actual: 3 (class: Integer) (latch = 0, values = 3, errors = 0, completions = 1)");
+        ts.assertValueAt(2, 2);


Can you please compare something else than integers (String will be ok)?

Silly code change in assert method can break its logic but pass this test because both index and value are 2 :)

vanniktech · 2017-08-01T07:49:31Z

We need to find some kind of policy for adding new API methods or not.

I'd say let's not get this in. Write a Kotlin extension functions 🙃 or use the assertValueAt with a Predicate and then define a test class that implements the interface and has a static factory method Predicate<T> just(T).

If this gets in I also would like to have mergeWith, concatWith, startWith variants for Single, Completable and Maybe on Observable & Flowable.

artem-zinnatullin · 2017-08-01T08:02:06Z

I'm pretty sure this method will be much more popular than assertValueAt(int, Predicate) but that's just my guess

Mr ~~Negotiator~~ @vanniktech 😸

For simple equals comparisons (which I believe are 95+% of asserts on values) this variant is definitely better than assertValueAt(int, Predicate):

It has nicer message and prints both expected and actual values including their classes (sometimes that saves tons of time)
It requires less symbols to type assertValueAt(3, expected) even comparing with Kotlin assertValueAt(3) { it == expected }

akarnokd · 2017-08-01T08:15:47Z

Independent additions are individually evaluated based on the merits, value, quality and maintenance requirements they pose.

This PR is self contained and adds a convenience pair to an existing method. Plus, once merged in, there will be no need for further development on it.

In contrast, all those mergeWith(Single) and co are just shorthands for mergeWith(Single.toObservable())s and alike and the premise of "optimize later" actually adds burden to the maintainer(s) of RxJava.

vanniktech · 2017-08-01T08:19:27Z

Alright, you got me convinced now. If those shorthands were to be implemented one by one and also in an optimized way, would you consider PRs for that?

akarnokd · 2017-08-01T08:26:16Z

Yes.

akarnokd · 2017-08-01T08:31:46Z

src/main/java/io/reactivex/observers/BaseTestConsumer.java

@@ -403,6 +403,31 @@ public final U assertNever(Predicate<? super T> valuePredicate) {

    /**
     * Asserts that this TestObserver/TestSubscriber received an onNext value at the given index
+     * which is equal to the given value with respect to Objects.equals.


Objects is a Java 8 component but ObjectHelper is internal, thus maybe let's say "with respect to null-safe Object.equals()".

Fixed the other javadoc I copied that from too :)

akarnokd · 2017-08-01T08:32:11Z

src/main/java/io/reactivex/observers/BaseTestConsumer.java

+     * @param index the position to assert on
+     * @param value the value to expect
+     * @return this
+     */


@since 2.1.3 - experimental

akarnokd · 2017-08-01T08:32:25Z

src/main/java/io/reactivex/observers/BaseTestConsumer.java

+     * @return this
+     */
+    @SuppressWarnings("unchecked")
+    public final U assertValueAt(int index, T value) {


@Experimental

akarnokd · 2017-08-01T08:32:52Z

src/main/java/io/reactivex/observers/BaseTestConsumer.java

+            throw fail("No values");
+        }
+
+        if (index >= values.size()) {


Use s instead of values.size() here.

Nice catch.

NickFirmani · 2017-08-01T19:09:34Z

Thanks for the reviews!

NickFirmani · 2017-08-01T19:34:52Z

CI is failing due to
ERROR: JAVA_HOME is set to an invalid directory: /usr/lib/jvm/java-7-oracle

akarnokd · 2017-08-01T20:47:36Z

There has been an image change on Travis CI and JDK 7 is no longer supported. See #5531.

akarnokd · 2017-08-01T21:55:58Z

Could you rebase the PR so the build settings can take effect?

* 2.x: Try fixing Travis CI lack of java * Force dist: precise * Oracle JDK 8 is then

NickFirmani · 2017-08-02T20:58:53Z

Thanks!

Add assertValueAt(int, value) to TestObserver

0ea4cfa

akarnokd added the 2.x label Jul 31, 2017

Fix test to call correct method

829e047

artem-zinnatullin suggested changes Aug 1, 2017

View reviewed changes

vanniktech approved these changes Aug 1, 2017

View reviewed changes

akarnokd requested changes Aug 1, 2017

View reviewed changes

Nicholas Firmani added 2 commits August 1, 2017 12:05

David's comments

0bec07a

Artem's comments

6ae5f38

akarnokd approved these changes Aug 1, 2017

View reviewed changes

NickFirmani closed this Aug 1, 2017

NickFirmani reopened this Aug 1, 2017

2.x: Try fixing Travis CI lack of java (ReactiveX#5531)

bfbd517

* 2.x: Try fixing Travis CI lack of java * Force dist: precise * Oracle JDK 8 is then

akarnokd merged commit 5242cf3 into ReactiveX:2.x Aug 2, 2017

akarnokd mentioned this pull request Aug 8, 2017

2.1.3 release preparations #5542

Closed

jared-bot mentioned this pull request Aug 15, 2017

Dependency update: rxjava xmartlabs/bigbang#133

Closed

jared-bot mentioned this pull request Sep 27, 2017

Dependency update: rxjava xmartlabs/bigbang-template#14

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add assertValueAt(int, value) to TestObserver #5529

Add assertValueAt(int, value) to TestObserver #5529

NickFirmani commented Jul 31, 2017

akarnokd commented Jul 31, 2017

NickFirmani commented Jul 31, 2017

codecov bot commented Jul 31, 2017 •

edited

Loading

akarnokd commented Aug 1, 2017

artem-zinnatullin left a comment

artem-zinnatullin Aug 1, 2017

NickFirmani Aug 1, 2017

vanniktech commented Aug 1, 2017

artem-zinnatullin commented Aug 1, 2017

akarnokd commented Aug 1, 2017

vanniktech commented Aug 1, 2017

akarnokd commented Aug 1, 2017

akarnokd Aug 1, 2017

NickFirmani Aug 1, 2017

akarnokd Aug 1, 2017

akarnokd Aug 1, 2017

akarnokd Aug 1, 2017

NickFirmani Aug 1, 2017

NickFirmani commented Aug 1, 2017

NickFirmani commented Aug 1, 2017

akarnokd commented Aug 1, 2017

akarnokd commented Aug 1, 2017

NickFirmani commented Aug 2, 2017

Add assertValueAt(int, value) to TestObserver #5529

Add assertValueAt(int, value) to TestObserver #5529

Conversation

NickFirmani commented Jul 31, 2017

akarnokd commented Jul 31, 2017

NickFirmani commented Jul 31, 2017

codecov bot commented Jul 31, 2017 • edited Loading

Codecov Report

akarnokd commented Aug 1, 2017

artem-zinnatullin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vanniktech commented Aug 1, 2017

artem-zinnatullin commented Aug 1, 2017

akarnokd commented Aug 1, 2017

vanniktech commented Aug 1, 2017

akarnokd commented Aug 1, 2017

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

NickFirmani commented Aug 1, 2017

NickFirmani commented Aug 1, 2017

akarnokd commented Aug 1, 2017

akarnokd commented Aug 1, 2017

NickFirmani commented Aug 2, 2017

codecov bot commented Jul 31, 2017 •

edited

Loading