Cancel the subscription after receiving all of the pertinent emissions (#259) #260
Conversation
|
@ktoso can you provide some guidance on how you want this tested please. |
|
I think adding a test that has a simple synchronous publisher that checks for the cancellation signal while spinning would be good enough here - we just want to show that such test will properly terminate - right? |
|
Got a few mins more time, more hints which should help you get started :-) Put the test inside In our case I'd go with: @Test
public void required_spec317_mustNotSignalOnErrorWhenPendingAboveLongMaxValue_forSynchronousPublisher() throws Throwable { |
|
👍 |
…axValue' completes in a timely manner for fully synchronous publishers (reactive-streams#259).
|
@ktoso ping. |
| }).required_spec317_mustNotSignalOnErrorWhenPendingAboveLongMaxValue(); | ||
|
|
||
| // 11 due to the implementation of this particular TCK test (see impl) | ||
| Assert.assertEquals(sent.get(), 11); |
There was a problem hiding this comment.
I think I'd skip this check, the test "passes" if it doesn't end up infinitely publishing - it's true that it is 11 signals but I wouldn't test for this number.
There was a problem hiding this comment.
Since it's the verification test, isn't it fine to be specific?
There was a problem hiding this comment.
No strong feelings about it, can stay as is :)
There was a problem hiding this comment.
If it is wrong I'd want to see it removed, but from a cursory inspection I don't see that it could be problematic, but if I am wrong, please do tell me!
There was a problem hiding this comment.
It's definitely not wrong, just depending on the internally+arbitrarily chosen value inside of required_spec317_mustNotSignalOnErrorWhenPendingAboveLongMaxValue.
You're right though that these are very specific tests so keeping as-is may be a good idea.
There was a problem hiding this comment.
If we change the underlying TCK impl then we'd also have to change this assertion, I think that's good.
|
Yup good one, LGTM // @viktorklang @smaldini I think we'd like to release an RC with this fix for final validation before 1.0, sounds like a plan? |
|
I've already done the copyright waiver thing: https://github.com/reactive-streams/reactive-streams-jvm/blob/master/CopyrightWaivers.txt#L29 |
|
@reactive-streams/contributors Let's merge this TCK fix in and issue a new RC immediately to get feedback. |
|
Merging |
Cancel the subscription after receiving all of the pertinent emissions (#259)
See #259.