Reactor-core: Hide TestSubscriber constructors

[rstoyanchev]

Currently TestSubscriber requires code like this:

TestSubscriber<Foo> subscriber = new TestSubscriber<>();
subscriber.bindTo(publisher);
subscriber.assertValues(foo1, foo2);

As an API it's less than ideal that you can forget to bind and start using assert methods. It leaves you wondering why no values are coming. I've done it more than once.

If we make the constructors private and bindTo effectively a static factory, then this kind of mistake would not be possible and it would be more readable.

[akarnokd]

I don't get why bindTo even exists, what's wrong with Publisher.subscribe()?

[rstoyanchev]

Indeed bindTo could go entirely.

That said what I find disconcerting still is that I can start using assert methods before subscribing and those methods are completely silent and not proactive about such erroneous usage.

What could be useful is to separate out the assert-related methods. The TestSubscriber would not automatically expose them but would return an implementation of some interface or class with the assert methods from a call to bindTo.

Short form:

TestSubscriber.bindTo(publisher).assertValues(foo1, foo2);

Long form:

TestSubscriber<Foo> subscriber = new TestSubsciber<>();
subscriber.configureValuesTimeout(Duration.ofSeconds(5));
SubscriberAssert assert = subscriber.bindTo(publisher);
assert.assertValues(foo1, foo2);

The names could also be shortened if placed on a dedicated interface or class (e.g. hasValues or values instead instead of assertValues).

[smaldini]

For TestSubscriber we could have create() and bindTo() static methods hiding pre-subscribing or not indeed.

[sdeleuze]

+1 for @rstoyanchev proposal as well, with an additional create() method as proposed by @smaldini in order to still be able to use TestSubscriber in tests where Publisher.subscribe() is used.

[sdeleuze]

Another idea in order to avoid a very common error, maybe we could provide something like TestSubscriber.bindToAndAwait() (not sure about the name) method useful to wait a terminal signal (error or complete) to perform assertions.

[mp911de]

+1 @rstoyanchev, happened also to me that I did not subscribe and wondered about the results.

Having a common form like

TestSubscriber<Foo> subscriber = TestSubscriber.create();

would be consistent with Mono and Flux creation (create is just a placeholder for a better name).

Some tests just require a subscription and assertion, without further configuration. These require also more boilerplate that could be omitted. Having a shortcut is beneficial:

TestSubscription<Foo> subscriber = TestSubscriber.subscribe(flux);

[sdeleuze]

I am starting to work on that and will submit a PR for validating the API design.

[artembilan]

Let me share my five cents.

I don't understand your concern with that ctor. The TestSubscriber is exactly Reactive Streams Subscriber. So, making it to work I definitely should do something like Publisher.subscriber(Subscriber).

If you do that change here, you'll make TestSubscriber useless for me:

TestSubscriber<Message<?>> testSubscriber = new TestSubscriber<>();

ReactiveEndpoint reactiveEndpoint = new ReactiveEndpoint(testChannel, testSubscriber);

[sdeleuze]

@mp911de Are you proposing to use TestSubscriber.subscribe(flux) method name instead of TestSubscriber.bindTo(flux) ?

@artembilan Maybe the advantage of using static methods instead of constructors allows to have some more meaningful "named constructors", but I share your concern and I would like TestSubscriber keep being a regular Subscriber. I will try to see what I can do in the PR I am working on.

[smaldini]

I would just use TestSubscriber.subscribe() and keep TestSubscriber.create() to hide the constructor and forcing the question do you want to defer or not with 2 static.

[artembilan]

Thank you, @sdeleuze , for your attention!
Maybe instead of create() just raw() to warn end-user that he must subscribe it before interactions.
But create() with the good JavaDocs would be fine, too.

[smaldini]

I think if you can only do subscribe() or create() statically to create a TestSubscriber, the behavior difference with a good javadoc should be alright.

[mp911de]

@sdeluze Don't want to change bindTo(…), my TestSubscriber.create(…) proposal was intended for a static factory method that creates a subscribed TestSubscriber.

Either way, create(flux) and raw() read well.

[sdeleuze]

Feel free to have a look to https://github.com/reactor/reactor-core/pull/97 before @smaldini merge it to master ;-)