TestEnvironment: Fix timeout arithmetic #544
Conversation
|
@viktorklang - are you still merging commits? are there other contributors (do you need more)? |
|
Indeed the original code looks wrong but I don't remember tripping this failure case ever. Also isn't there a TCK test that verifies this timeout case? |
|
The fact that this obvious bug only surfaces now means that this TCK facility is not used correctly anywhere in the code — |
|
If errors are delivered synchronously (like in the TCK tests) and the unit under test is behaving as expected, it is unlikely this code will be hit (e.g. |
|
I added a unit test to the TCK to demonstrate the issue (if you remove the fix the test will likely fail). |
examples/src/test/java/org/reactivestreams/example/unicast/AsyncRangePublisherTest.java
Show resolved
Hide resolved
Scottmitch
force-pushed
the
timeout_fix
branch
2 times, most recently
from
2b7af33 to
b7c8c83
Compare
Scottmitch
force-pushed
the
timeout_fix
branch
4 times, most recently
from
d1e0462 to
f606fa1
Compare
|
@akarnokd - thanks for approval. What are the next steps to getting this merged and released? |
|
I don't know. Most of the original crew have moved on afaik so no idea. |
|
I’m afraid I lost my sonatype credentials, so I can’t publish anymore. If someone else wants to step up and needs me to confirm something, then I’ll happily do that! |
|
No idea. Also no idea how the release infrastructure is working. If I had this annoying issue, after so much time, I'd seriously consider cloning the TCK, applying the fixes and begin using that to verify my projects. Given the amount of development in RS, you probably won't have to worry about becoming incompatible. (If I understood .NET better, I would have done it for the Reactive Streams .NET TCK repo, which practically unusable with newer test frameworks of .NET) |
examples/src/test/java/org/reactivestreams/example/unicast/AsyncRangePublisherTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
AFAIKS this looks fine, LGTM. I guess it "worked ok enough" so that we didn't notice earlier, thanks for the fix @Scottmitch.
As for releasing, I don't think I can help with that nowadays.
|
@ktoso - thx for review. comments addressed.
I'm assuming most tests have publisher and verification on same thread which would make it unlikely to come up. |
Motivation: The timeout calculation decrementing `totalTimeoutRemainingNs` is incorrect which results in premature timeouts and test failures. Signed-off-by: Scott Mitchell <[email protected]>
|
I'd approve and merge but have long since lost merge rights here. @viktorklang @rkuhn are you able to help out here? |
|
I can merge, so I did that, but as I said I can’t publish anymore. |
|
Thank you! It seems I can publish (but not merge) huh, so I’ll give that a try if we want a patch release with this :) |
|
Sure, go ahead! I guess you’ll need another PR to bump the version, right? |
|
version update: #547 anything else required for release? |
Motivation:
The timeout calculation decrementing
totalTimeoutRemainingNsis incorrect which results in premature timeouts and test failures.