Replaced TestUntil by AccessSafely#11
Conversation
|
@eliezio Since @alexguzun was working in wire most recently, I prefer that he review your code. He is working near London this week and won't be back to Portugal until the weekend. I will do a review but I won't merge. I want Alex to approve and merge. |
VaughnVernon
left a comment
There was a problem hiding this comment.
@eliezio Looks good to me. Will await @alexguzun to approve and merge.
VaughnVernon
left a comment
There was a problem hiding this comment.
@eliezio I think that the common base class test makes sense. It seems like a good idea to prove that the interfaces are appropriately implemented across different transport types. I approve but will await @alexguzun to approve and merge.
|
@eliezio @alexguzun The Travis-CI build is failing on |
| import static org.junit.Assert.assertEquals; | ||
|
|
||
| public abstract class AbstractServerChannelActorTest { | ||
| private static AtomicInteger TEST_PORT = new AtomicInteger(49560); |
There was a problem hiding this comment.
Each test, extending from this class, should have their own port range to avoid collision when running parallel tests .
There was a problem hiding this comment.
Got it. Is it possible to use random ports on all tests, allowing a definitive solution for problems like this?
There was a problem hiding this comment.
Even a single static AtomicInteger and using incrementAndGet() in the abstract base will work for all extenders. I don't know why random would be better. You would still have to set ranges to different tests don't collide. I don't think there could be a central static because it could get recreated if multiple JVMs are employed in a given test run.
There was a problem hiding this comment.
When I say random I mean letting the OS choose a random local port and configuring the client accordingly. This is a common failsafe practice on testing server applications.
There was a problem hiding this comment.
I could not find any reliable way to do what you are suggesting with raw java.nio.channels.SocketChannel and io.rsocket.transport.netty.server.TcpServerTransport at the time. I think most of the frameworks simply loop through sockets until find one that's open. We could replicate the approach but it's prone to race conditions when the loop identifies a free socket, closes it, and the consumer did not yet attached to it.
There was a problem hiding this comment.
This automatic port allocation is provided by every ServerSocket implementation. See https://docs.oracle.com/javase/8/docs/api/java/net/ServerSocket.html#ServerSocket-int-
Moreover, the test at https://github.com/rsocket/rsocket-java/blob/88ee909b8bfb78b5c53fa25bc39e16f6d893f0df/rsocket-transport-netty/src/test/java/io/rsocket/transport/netty/server/TcpServerTransportTest.java#L85 suggests that this mechanism is also available on rsocket.
There was a problem hiding this comment.
Great find! Can you test this?
No description provided.