-
Notifications
You must be signed in to change notification settings - Fork 463
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
JAMES-3715 Add TCP transport level tests for IMAP IDLE #910
Conversation
In the face of Netty4 migration, pipeline modifications (which IDLE does) deserves to be seriously tested.
- A field can be static - Apply boolean expression simplifications
IDLE listener sends unsolicited notification setted up by the SelectedMailbox listener, thus IDLE listener needs to run after the SelectedMailbox listener. James EventBus provides no such guaranty thus we can run in a data race where IDLE is called first, causing it to never send changes. As a fix, we make SelectedMailbox calling synchronously IDLE listener if needed. IDLE listener no longer need its own registration on the event bus as it is "attached" to the one of SelectedMailbox.
|
||
@Test | ||
void idleShouldSendInitialContinuation() throws Exception { | ||
SocketChannel server = SocketChannel.open(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is more like a client than a server? BTW should we put this SocketChannel.open() into BeforeEach and close it in AfterEach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is more like a client than a server?
Yes we are opening a TCP client connection to test the server.
BTW should we put this SocketChannel.open() into BeforeEach and close it in AfterEach?
👍
} | ||
|
||
@Test | ||
void idleShouldResponsesShouldBeOrdered() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
void idleShouldResponsesShouldBeOrdered() throws Exception { | |
void idleResponsesShouldBeOrdered() throws Exception { |
imapServer.destroy(); | ||
clientConnection.close(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we close the client connection before destroying the server?
In the face of Netty4 migration, pipeline modifications (which IDLE does)
deserves to be seriously tested.