Skip to content
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

Add unit test for server timeout. #277

Merged
merged 1 commit into from
Nov 17, 2021
Merged

Add unit test for server timeout. #277

merged 1 commit into from
Nov 17, 2021

Conversation

luca-schlecker
Copy link
Collaborator

Signed-off-by: Luca Schlecker luca.schlecker@hotmail.com

This is a preparation for my work on dumb_timer_queue (#264 and thus #273).
It tries to test the timeout value by checking if the tcp socket reaches eof in the specified amount of time indicating that the server closed the connection.

@luca-schlecker luca-schlecker marked this pull request as draft November 17, 2021 11:25
@The-EDev
Copy link
Member

I made a mental note a couple days ago about the fact that timeout isn't being tested, but you beat me to it 😂

@luca-schlecker
Copy link
Collaborator Author

Any ideas why it's not working? I suspect the problem in these lines:

status = receive_future.wait_for(chrono::seconds(2));
CHECK(status == future_status::ready);

Could it be that the CI takes longer than 2 seconds to transfer the message and thus the receive call didn't yet return?

@The-EDev
Copy link
Member

I'm looking into it now

@The-EDev
Copy link
Member

image

welp :|

@luca-schlecker
Copy link
Collaborator Author

That's what I've thought too... Works on my machine locally.

Signed-off-by: Luca Schlecker <luca.schlecker@hotmail.com>
@luca-schlecker
Copy link
Collaborator Author

Interesting, I've made the timings looser and now the test passes...

Copy link
Member

@The-EDev The-EDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, tests are working, only part of timer not being tested is a case where io_service doesn't exist. The only thing I have to complain about is styling, but that should be fixed soon with clang-format. Great work!

@luca-schlecker
Copy link
Collaborator Author

Sorry about the style, Crow's written in a different style to mine (which is based on Google's style), and thus autoformat can't really help me. Yes, should be fixed with clang-format.

@luca-schlecker
Copy link
Collaborator Author

luca-schlecker commented Nov 17, 2021

And I honestly still can't figure out what that io_service object is doing there. It just fails when it's not there, but it's not being used...

I am tempted to just leave it out in my small rework of the dumb_timer_queue.

@luca-schlecker luca-schlecker marked this pull request as ready for review November 17, 2021 19:54
@The-EDev
Copy link
Member

I'm guessing it's for redundancy, so that the timer doesn't execute any timeout callbacks if there is no io_service (since they'll probably always depend on it)

@The-EDev
Copy link
Member

Sorry about the style, Crow's written in a different style to mine (which is based on Google's style), and thus autoformat can't really help me. Yes, should be fixed with clang-format.

No worries, I've already written a new clang-format file, I'm just struggling with automated messages showing formatting errors when running CI.

@The-EDev The-EDev merged commit 22d3918 into master Nov 17, 2021
@luca-schlecker
Copy link
Collaborator Author

Well, functions get executed by asking a dumb_timer_queue to do its work over and over again. But I guess that's a discussion for my upcoming PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants