Fix missing cancel() in tests that don't consume the entire source #346
Conversation
akarnokd
changed the title
| sub.expectNone(String.format("Publisher %sproduced unrequested ", pub)); | ||
| } finally { | ||
| sub.cancel(); | ||
| } |
| }); | ||
| onSubscribeLatch.expectClose("Should have received onSubscribe"); | ||
| env.verifyNoAsyncErrorsNoDelay(); | ||
| } |
| if (fail) { | ||
| throw new AssertionError("Cancellations were missing:" + b); | ||
| } | ||
| } |
| } | ||
|
|
||
| public RangePublisherTest() { | ||
| super(new TestEnvironment(25)); |
There was a problem hiding this comment.
the default one should be fine, isn't it?
There was a problem hiding this comment.
We use 25 ms for most of the synchronous tests in RxJava because otherwise it takes minutes to work through the 100+ TCK tests. Since this Publisher is synchronous, there is little to no chance for a timeout in my opinion.
There was a problem hiding this comment.
This isn't the Rx suite nor does it impact the Rx suite, so that does not really matter :-)
It does, however, break the configurability of the timeouts for how we use them in this repo: since the setting is given via env value: https://github.com/reactive-streams/reactive-streams-jvm/blob/master/.travis.yml#L14 and settings in code win over settings from env (documented here: https://github.com/reactive-streams/reactive-streams-jvm/tree/master/tck#timeout-configuration ).
Please note that timeout does not hurt suite time all that much, the more painful one is (as explained here: https://github.com/reactive-streams/reactive-streams-jvm/tree/master/tck#timeout-configuration
) contributes much more to "actually waiting doing nothing time", so if anything the other one is more influential AFAIR.
Please stick to the default here so the .travis.yml configuration continues to work in the project, thanks.
|
TCK fixes look correct, thanks for catching this. |
|
Updated to default |
|
:+1 |
| s.onSubscribe(parent); | ||
| } | ||
|
|
||
| static final class RangeSubscription extends AtomicLong implements Subscription { |
There was a problem hiding this comment.
Would it be of greater utility to have it be an IteratorSubscription?
There was a problem hiding this comment.
The main purpose was to track cancellation/completion in response to the TCK and this was the simplest multi-valued Publisher I could think of.
Working with an iterator requires a lot of try-catches around the method calls to Iterator because they may throw (for example, I saw libraries that hid JDBC calls behind plain iterators).
There was a problem hiding this comment.
@akarnokd Fair enough, we can leave that to another time. :)
There was a problem hiding this comment.
I guess you meant an example IteratorSubscription. How about I post a PR to the examples submodule with a few basic subscription types?
There was a problem hiding this comment.
@akarnokd No, not an example. I thought it might be iteresting for the TCK to have a sync publisher which can be supplied an Iterable :)
|
In case my previous comment was not clear enough: LGTM, thanks 😉 |
|
@reactive-streams/contributors let me know if we can merge (or add approved in review menu) |
|
@reactive-streams/contributors Ok, merging. |
Some of the
PublisherVerificationtests didn't cancel theSubscriptionwhen they stopped consuming the source.