Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

3.x: review features of the test consumers #6153

Closed
akarnokd opened this issue Aug 10, 2018 · 22 comments
Closed

3.x: review features of the test consumers #6153

akarnokd opened this issue Aug 10, 2018 · 22 comments
Milestone

Comments

@akarnokd
Copy link
Member

  • Review available assertions.
  • Remove redundant assertions.
  • Resolve ambiguities about the assertions.
  • Rething naming of assertions.
@vanniktech
Copy link
Collaborator

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
Copy link
Contributor

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
Copy link
Member Author

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

@JakeWharton
Copy link
Contributor

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

@sircelsius
Copy link
Contributor

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

@akarnokd
Copy link
Member Author

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

@JakeWharton
Copy link
Contributor

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

@akarnokd
Copy link
Member Author

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

@JakeWharton
Copy link
Contributor

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

@akarnokd
Copy link
Member Author

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
Copy link
Collaborator

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

@akarnokd
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Member Author

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

@JakeWharton
Copy link
Contributor

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
Copy link
Member Author

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

@akarnokd
Copy link
Member Author

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

@arturdryomov
Copy link
Contributor

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
Copy link
Contributor

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

@akarnokd
Copy link
Member Author

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%) Keep1
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%) Keep2
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%) Keep1
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%) Keep2
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 types3
  • 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.
3 would match the event terminology better (assertSuccess) but introduces redundancy.

@akarnokd
Copy link
Member Author

Closing via #6526.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants