Rxjava: 3.x: review features of the test consumers

[akarnokd]

• Review available assertions.
• Remove redundant assertions.
• Resolve ambiguities about the assertions.
• Rething naming of assertions.

[vanniktech]

The more I'm using it the more I feel like assertValues should be removed. It only asserts half of the things. Using assertValuesOnly, assertResult, or the overload with the error matcher would be more pragmatic.

If the main code gets changed and suddenly the source completes the unit test would not break with assertValues. Often times distinguishing between a completing and not completing Observable / Flowable really does matter.

[sircelsius]

Maybe I'm being naive, but for most of my more advanced test cases I end up getting the onNext events and asserting on them directly. I have never encountered a case where assertValues was useful.

My understanding is that assertCompleted, assertValueCount and the likes are enough for simple test cases, and clients get little value from assertValues for more complex cases.

[akarnokd]

Please don't forget that the test consumer is there to test RxJava itself, not just your code out there.

[JakeWharton]

But if those requirements differ then the APIs should be split.

[sircelsius]

+1, I don't really get why methods used to test RxJava internals should be exposed to clients

[akarnokd]

The Java language doesn't allow hiding methods from non-project usages.

[JakeWharton]

.to(test()) isn't that much longer than .test().

[akarnokd]

Type inference may not work most of the time, requiring a full expression with type arguments for that test.

[JakeWharton]

And thankfully as the library we bear that extra burden so that our consumers don't have to!

[akarnokd]

I've composed a questionare about the test support:

https://docs.google.com/forms/d/1kiuYMVxbDjsPhIjWMG4V4aiHUevhCI9BzOmlRd-OWhs

Let me know what you think and if adequate, I'll tweet it out.

[vanniktech]

Maybe reduce the number of options one can give to each operator or use a better control UX-wise.

[akarnokd]

I've tried a matrix-choice control but Google Docs doesn't allow me to change the width of the form thus it introduced a horizontal scrollbar you'd have to scroll left-right all the time. The choices should also help with the original tasks, for example, which operators should be renamed.

[artem-zinnatullin]

Good questionare 👍

I'm a bit afraid though, that because many don't understand the difference between some of the assertion methods in the first place, questionare results might be misleading.

Hopefully people will look into javadocs, maybe questionare should have links to each method/type javadoc in case people don't have IDE/sources at hand.

[akarnokd]

There is the implicit assumption that people know which methods they have used and if the others don't ring a bell, they will simply chose the remove/not-using/no opinion.

[akarnokd]

Updated the form with links to each method's JavaDoc.

[JakeWharton]

My issue is that the APIs don't consume events as you assert so you constantly have to supply all events or awkwardly remove events separate from assertion. I always end up writing my own test observer as a result.

[akarnokd]

Reactor uses a different approach: StepVerifier. Note however that there were complications: expressing testing events over time, synchronizing through a test scheduler, etc.

[akarnokd]

I wonder if content-assist filters in one's IDE would help with the method "bloat" experienced with RxJava?

[arturdryomov]

I’ll express the feedback I’ve collected browsing through the project I’m working on (and I’m too lazy to describe all assertions available). We have something like 15K+ tests, a lot of them include RxJava checks.

We have a specific rule to generally avoid termination events, especially errors — basically we treat them as _failures_, i. e. _something went horribly wrong and someone produced onError_. That said, we don’t use much.

• Most of the time we create TestObserver explicitly, subscribe and work with it directly.
• The test() method is rarely used since we have complex multi-step checks (hey BDD) and it does not scale for such situations.
• The most common assertion we use is assertValuesOnly and there is an ongoing push to replace every assertValue with it since it provides more checks under the hood. And actually something like assertValueOnly would be a better fit for us.
• Next go assertResult and assertEmpty — again, both of them are better checks than assertValue and assertNoValues.

Regarding what can be done better — we have a custom _cleanup_ extension that clears current values in TestObserver. It allows us to write more lean tests:

context("event") {
    it("emits value A") { valueObserver.assertValuesOnly("A") }

    context("different event") {
        beforeEachTest { valueObserver.clear() }

        it("emits value B") { valueObserver.assertValuesOnly("B") }
        // Without clear() above we need to check both values, i. e. assertValuesOnly("A", "B")
    }
}

It is similar to Mockito.clearInvocations() and can be useful, but we are fine with using our own thing.

I wouldn’t mind moving TestScheduler, TestObserver and friends into a separate artifact. Pretty sure we can live without the Observable.test() method.

Otherwise we are satisfied with the current set of asserts. The naming is a bit confusing and it leads to discoverability issues — I had to explain the difference between assertValue and assertResult a couple of times. Maybe something like assertOnNext, assertOnNextOnly can be a better fit. Also, think about removing complex checks like assertValuesOnly altogether since it forces you to create smart naming which combines multiple actions. assertResult is understandable, but I think there is no _result_ term in reactive world.

[ragunathjawahar]

assertValues is extremely helpful and I would prefer to have it in future releases as well.

[akarnokd]

The results of the questionare (34 responses). Bold indicates the dominant factor for the proposed action to take.

Question | Answers | Action
-------|------------|------
Should a test() operator be available? | Yes (91.2%), No (8.8%) | Keep
Should the TestSubscriber/TestObserver be available publicly | Yes, both (79.4%), No (8.8%), TS only (2.9%), TO only (8.8%) | Keep both
Should RxJava support testing flows out of box | Yes (100%), No (0%) | Keep supporting
Opinions about test methods | ... | ...
assertComplete | Keep: using it (41.2%), Keep: using it a lot (11.8%), Can live without (14.7%), Confusing: rename it (2.9%), Not using can stay (20.6%), Redundant: remove it (0%), No need: remove it (0%), No opinion (5.9%) | Keep
assertEmpty | Keep: using it (35.3%), Keep: using it a lot (11.8%), Can live without (14.7%), Confusing: rename it (11.8%), Not using can stay (20.6%), Redundant: remove it (0%), No need: remove it (0%), No opinion (5.9%) | Keep & Rename
assertError(Class) | Keep: using it (32.4%), Keep: using it a lot (14.7%), Can live without (14.7%), Confusing: rename it (0%), Not using can stay (20.6%), Redundant: remove it (2.9%), No need: remove it (2.9%), No opinion (11.8%) | Keep
assertError(Predicate) | Keep: using it (38.2%), Keep: using it a lot (8.8%), Can live without (8.8%), Confusing: rename it (0%), Not using can stay (14.7%), Redundant: remove it (2.9%), No need: remove it (2.9%), No opinion (23.5%) | Keep
assertError(Throwable) | Keep: using it (41.2%), Keep: using it a lot (5.9%), Can live without (14.7%), Confusing: rename it (0%), Not using can stay (8.8%), Redundant: remove it (11.8%), No need: remove it (2.9%), No opinion (14.7%) | Keep
assertErrorMessage | Keep: using it (2.9%), Keep: using it a lot (2.9%), Can live without (14.7%), Confusing: rename it (0%), Not using can stay (44.1%), Redundant: remove it (8.8%), No need: remove it (14.7%), No opinion (11.8%) | Remove
assertFailure(Class, T...) | Keep: using it (5.9%), Keep: using it a lot (5.9%), Can live without (2.9%), Confusing: rename it (8.8%), Not using can stay (35.3%), Redundant: remove it (5.9%), No need: remove it (5.9%), No opinion (29.4%) | Keep & Rename
assertFailure(Predicate, T...) | Keep: using it (5.9%), Keep: using it a lot (2.9%), Can live without (2.9%), Confusing: rename it (8.8%), Not using can stay (38.2%), Redundant: remove it (5.9%), No need: remove it (5.9%), No opinion (29.4%) | Remove
assertFailureAndMessage | Keep: using it (5.9%), Keep: using it a lot (5.9%), Can live without (5.9%), Confusing: rename it (2.9%), Not using can stay (29.4%), Redundant: remove it (8.8%), No need: remove it (17.6%), No opinion (23.5%) | Remove
assertNever(Predicate) | Keep: using it (8.9%), Keep: using it a lot (5.9%), Can live without (0%), Confusing: rename it (8.8%), Not using can stay (41.2%), Redundant: remove it (2.9%), No need: remove it (11.8%), No opinion (20.6%) | Remove
assertNever(T) | Keep: using it (17.6%), Keep: using it a lot (2.9%), Can live without (8.8%), Confusing: rename it (8.8%), Not using can stay (32.4%), Redundant: remove it (5.9%), No need: remove it (8.8%), No opinion (14.7%) | Remove
assertNoErrors | Keep: using it (29.4%), Keep: using it a lot (35.3%), Can live without (8.8%), Confusing: rename it (2.9%), Not using can stay (5.9%), Redundant: remove it (0%), No need: remove it (5.9%), No opinion (11.8%) | Keep
assertNotComplete | Keep: using it (32.4%), Keep: using it a lot (2.9%), Can live without (11.8%), Confusing: rename it (5.9%), Not using can stay (26.5%), Redundant: remove it (0%), No need: remove it (5.9%), No opinion (14.7%) | Keep
assertNoTimeout | Keep: using it (2.9%), Keep: using it a lot (0%), Can live without (14.7%), Confusing: rename it (0%), Not using can stay (38.2%), Redundant: remove it (5.9%), No need: remove it (11.8%), No opinion (26.5%) | Remove
assertNotSubscribed | Keep: using it (5.9%), Keep: using it a lot (2.9%), Can live without (14.7%), Confusing: rename it (0%), Not using can stay (32.4%), Redundant: remove it (0%), No need: remove it (8.8%), No opinion (35.3%) | Remove
assertNotTerminated | Keep: using it (8.8%), Keep: using it a lot (2.9%), Can live without (14.7%), Confusing: rename it (0%), Not using can stay (29.7%), Redundant: remove it (5.9%), No need: remove it (11.8%), No opinion (26.5%) | Remove
assertNoValues | Keep: using it (41.2%), Keep: using it a lot (14.7%), Can live without (11.8%), Confusing: rename it (2.9%), Not using can stay (11.8%), Redundant: remove it (0%), No need: remove it (5.9%), No opinion (11.8%) | Keep
assertResult | Keep: using it (14.7%), Keep: using it a lot (29.4%), Can live without (0%), Confusing: rename it (2.9%), Not using can stay (20.6%), Redundant: remove it (8.8%), No need: remove it (2.9%), No opinion (20.6%) | Keep
assertSubscribed | Keep: using it (2.9%), Keep: using it a lot (2.9%), Can live without (11.8%), Confusing: rename it (0%), Not using can stay (32.4%), Redundant: remove it (5.9%), No need: remove it (14.7%), No opinion (29.4%) | Remove
assertTerminated | Keep: using it (14.7%), Keep: using it a lot (0%), Can live without (5.9%), Confusing: rename it (0%), Not using can stay (26.5%), Redundant: remove it (17.6%), No need: remove it (11.8%), No opinion (23.5%) | Remove
assertTimeout | Keep: using it (5.9%), Keep: using it a lot (5.9%), Can live without (5.9%), Confusing: rename it (2.9%), Not using can stay (44.1%), Redundant: remove it (5.9%), No need: remove it (8.8%), No opinion (26.5%) | Remove
assertValue(Predicate) | Keep: using it (29.4%), Keep: using it a lot (20.6%), Can live without (5.9%), Confusing: rename it (2.9%), Not using can stay (17.6%), Redundant: remove it (5.9%), No need: remove it (2.9%), No opinion (14.7%) | Keep
assertValue(T) | Keep: using it (17.6%), Keep: using it a lot (38.2%), Can live without (5.9%), Confusing: rename it (2.9%), Not using can stay (2.9%), Redundant: remove it (5.9%), No need: remove it (2.9%), No opinion (20.6%) | Keep
assertValueAt(int, Predicate) | Keep: using it (14.7%), Keep: using it a lot (17.6%), Can live without (23.5%), Confusing: rename it (2.9%), Not using can stay (17.6%), Redundant: remove it (5.9%), No need: remove it (8.8%), No opinion (20.6%) | Keep
assertValueAt(int, T) | Keep: using it (14.7%), Keep: using it a lot (17.6%), Can live without (8.8%), Confusing: rename it (2.9%), Not using can stay (20.6%), Redundant: remove it (8.8%), No need: remove it (11.8%), No opinion (14.7%) | Keep
assertValueCount | Keep: using it (17.6%), Keep: using it a lot (38.2%), Can live without (5.9%), Confusing: rename it (2.9%), Not using can stay (2.9%), Redundant: remove it (5.9%), No need: remove it (5.9%), No opinion (20.6%) | Keep
assertValueSequence | Keep: using it (8.8%), Keep: using it a lot (11.8%), Can live without (8.8%), Confusing: rename it (5.9%), Not using can stay (23.5%), Redundant: remove it (8.8%), No need: remove it (5.9%), No opinion (26.5%) | Keep
assertValueSequenceOnly | Keep: using it (5.9%), Keep: using it a lot (5.9%), Can live without (5.9%), Confusing: rename it (8.8%), Not using can stay (17.6%), Redundant: remove it (14.7%), No need: remove it (2.9%), No opinion (38.2%) | Remove
assertValueSet | Keep: using it (2.9%), Keep: using it a lot (5.9%), Can live without (8.8%), Confusing: rename it (5.9%), Not using can stay (23.5%), Redundant: remove it (11.8%), No need: remove it (8.8%), No opinion (32.4%) | Remove
assertValueSetOnly | Keep: using it (0%), Keep: using it a lot (8.8%), Can live without (2.9%), Confusing: rename it (8.8%), Not using can stay (23.5%), Redundant: remove it (11.8%), No need: remove it (5.9%), No opinion (41.2%) | Remove
assertValuesOnly | Keep: using it (8.8%), Keep: using it a lot (11.8%), Can live without (5.9%), Confusing: rename it (8.8%), Not using can stay (17.6%), Redundant: remove it (8.8%), No need: remove it (8.8%), No opinion (32.4%) | Keep & rename
await | Keep: using it (23.5%), Keep: using it a lot (8.8%), Can live without (8.8%), Confusing: rename it (2.9%), Not using can stay (11.8%), Redundant: remove it (2.9%), No need: remove it (17.6%), No opinion (23.5%) | Keep
await(long, TimeUnit) | Keep: using it (26.5%), Keep: using it a lot (8.8%), Can live without (5.9%), Confusing: rename it (2.9%), Not using can stay (17.6%), Redundant: remove it (2.9%), No need: remove it (11.8%), No opinion (26.5%) | Keep
awaitCount(int) | Keep: using it (26.5%), Keep: using it a lot (2.9%), Can live without (2.9%), Confusing: rename it (0%), Not using can stay (14.7%), Redundant: remove it (5.9%), No need: remove it (17.6%), No opinion (29.4%) | Keep
awaitCount(int, Runnable) | Keep: using it (5.9%), Keep: using it a lot (2.9%), Can live without (0%), Confusing: rename it (2.9%), Not using can stay (29.4%), Redundant: remove it (5.9%), No need: remove it (20.6%), No opinion (32.4%) | Remove
awaitCount(int, Runnable, long) | Keep: using it (5.9%), Keep: using it a lot (2.9%), Can live without (0%), Confusing: rename it (2.9%), Not using can stay (26.5%), Redundant: remove it (2.9%), No need: remove it (20.6%), No opinion (38.2%) | Remove
awaitDone | Keep: using it (5.9%), Keep: using it a lot (5.9%), Can live without (5.9%), Confusing: rename it (5.9%), Not using can stay (26.5%), Redundant: remove it (5.9%), No need: remove it (14.7%), No opinion (29.4%) | Keep & rename
awaitTerminalEvent | Keep: using it (8.8%), Keep: using it a lot (5.9%), Can live without (20.6%), Confusing: rename it (0%), Not using can stay (17.6%), Redundant: remove it (2.9%), No need: remove it (11.8%), No opinion (32.4%) | Remove
awaitTerminalEvent(long TimeUnit) | Keep: using it (2.9%), Keep: using it a lot (2.9%), Can live without (11.8%), Confusing: rename it (0%), Not using can stay (26.5%), Redundant: remove it (5.9%), No need: remove it (11.8%), No opinion (38.2%) | Remove
clearTimeout | Keep: using it (2.9%), Keep: using it a lot (2.9%), Can live without (0%), Confusing: rename it (5.9%), Not using can stay (29.4%), Redundant: remove it (0%), No need: remove it (11.8%), No opinion (47.1%) | Remove
completions | Keep: using it (2.9%), Keep: using it a lot (0%), Can live without (2.9%), Confusing: rename it (5.9%), Not using can stay (20.6%), Redundant: remove it (2.9%), No need: remove it (11.8%), No opinion (52.9%) | Remove
errorCount | Keep: using it (2.9%), Keep: using it a lot (2.9%), Can live without (2.9%), Confusing: rename it (0%), Not using can stay (23.5%), Redundant: remove it (2.9%), No need: remove it (14.7%), No opinion (50%) | Remove
errors | Keep: using it (2.9%), Keep: using it a lot (5.9%), Can live without (5.9%), Confusing: rename it (2.9%), Not using can stay (20.6%), Redundant: remove it (0%), No need: remove it (14.7%), No opinion (47.1%) | Remove
getEvents | Keep: using it (5.9%), Keep: using it a lot (5.9%), Can live without (8.8%), Confusing: rename it (2.9%), Not using can stay (14.7%), Redundant: remove it (8.8%), No need: remove it (8.8%), No opinion (44.1%) | Remove
isTerminated | Keep: using it (8.8%), Keep: using it a lot (2.9%), Can live without (11.8%), Confusing: rename it (0%), Not using can stay (23.5%), Redundant: remove it (2.9%), No need: remove it (8.8%), No opinion (41.2%) | Remove
isTimeout | Keep: using it (2.9%), Keep: using it a lot (0%), Can live without (5.9%), Confusing: rename it (0%), Not using can stay (26.5%), Redundant: remove it (8.8%), No need: remove it (11.8%), No opinion (44.1%) | Remove
lastThread | Keep: using it (5.9%), Keep: using it a lot (0%), Can live without (2.9%), Confusing: rename it (0%), Not using can stay (17.6%), Redundant: remove it (0%), No need: remove it (17.6%), No opinion (55.9%) | Remove
valueCount | Keep: using it (14.7%), Keep: using it a lot (5.9%), Can live without (11.8%), Confusing: rename it (0%), Not using can stay (8.8%), Redundant: remove it (17.6%), No need: remove it (8.8%), No opinion (32.4%) | Remove
values | Keep: using it (11.8%), Keep: using it a lot (32.4%), Can live without (14.7%), Confusing: rename it (0%), Not using can stay (8.8%), Redundant: remove it (5.9%), No need: remove it (8.8%), No opinion (17.6%) | Keep
cancel | Keep: using it (14.7%), Keep: using it a lot (2.9%), Can live without (2.9%), Confusing: rename it (0%), Not using can stay (14.7%), Redundant: remove it (2.9%), No need: remove it (2.9%), No opinion (58.8%) | Keep¹
dispose | Keep: using it (23.5%), Keep: using it a lot (14.7%), Can live without (2.9%), Confusing: rename it (0%), Not using can stay (8.8%), Redundant: remove it (0%), No need: remove it (0%), No opinion (50%) | Keep²
isCancelled | Keep: using it (11.8%), Keep: using it a lot (2.9%), Can live without (2.9%), Confusing: rename it (2.9%), Not using can stay (23.5%), Redundant: remove it (2.9%), No need: remove it (2.9%), No opinion (50%) | Keep¹
isDisposed | Keep: using it (20.6%), Keep: using it a lot (8.8%), Can live without (5.9%), Confusing: rename it (2.9%), Not using can stay (14.7%), Redundant: remove it (0%), No need: remove it (2.9%), No opinion (44.1%) | Keep²
assertOf(TestSubscriber) | Keep: using it (5.6%), Keep: using it a lot (8.8%), Can live without (0%), Confusing: rename it (2.9%), Not using can stay (11.8%), Redundant: remove it (5.9%), No need: remove it (11.8%), No opinion (52.9%) | Remove
assertOf(TestObserver) | Keep: using it (5.6%), Keep: using it a lot (8.8%), Can live without (0%), Confusing: rename it (2.9%), Not using can stay (11.8%), Redundant: remove it (5.9%), No need: remove it (8.8%), No opinion (55.9%) | Remove
hasSubscription | Keep: using it (15.2%), Keep: using it a lot (2.9%), Can live without (9.1%), Confusing: rename it (2.9%), Not using can stay (18.2%), Redundant: remove it (2.9%), No need: remove it (2.9%), No opinion (48.5%) | Keep

From the additional comments

• using mainly values()
• rename confusing methods (empty)
• provide additional specific test consumer types for base reactive types³
• focus on testing, provide docs about how to test
• likes the test() API's existence
• likes the test functionality but RxJava should ship the test support separately via helpers
• better AssertJ support via callback having all events provided as parameters

Methods used for testing mainly by RxJava will be moved to the test-only TestHelper class. Some removed features may require an RxJava internal subclass instead to access protected internal state.

Remarks

^{1, 2} these are redundant, may remove dispose from TestSubscriber and remove cancel from TestObserver.
³ would match the event terminology better (assertSuccess) but introduces redundancy.

[akarnokd]

Closing via #6526.