Skip to content
This repository was archived by the owner on Dec 18, 2018. It is now read-only.

Fix failing keep-alive timeout tests #1090

Merged
merged 1 commit into from
Sep 9, 2016
Merged

Conversation

cesarblum
Copy link
Contributor

@@ -91,23 +91,27 @@ public class KeepAliveTimeoutTests
{
using (var connection = new TestConnection(server.Port))
{
for (var i = 0; i < 10; i++)
for (var i = 0; i < 11; i++)
Copy link
Contributor

Choose a reason for hiding this comment

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

11? Why not 10?

Copy link
Contributor

@benaadams benaadams Sep 8, 2016

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pakrym Ha, that's not necessary. I had it in my head that the keep-alive timeout I was using for the tests was 30s, but it's 10s. With the short delay being 3s I'd need 11 iterations to make sure I was getting past the timeout.

@cesarblum cesarblum force-pushed the cesarbs/fix-keepalive-tests branch from e63ba38 to 5b20652 Compare September 8, 2016 03:58
@cesarblum
Copy link
Contributor Author

Updated.

@cesarblum
Copy link
Contributor Author

Travis failure is unrelated.

@cesarblum
Copy link
Contributor Author

@halter73
Copy link
Member

halter73 commented Sep 9, 2016

:shipit:

@cesarblum cesarblum merged commit 5b20652 into dev Sep 9, 2016
@cesarblum cesarblum deleted the cesarbs/fix-keepalive-tests branch September 9, 2016 21:02
@mikeharder
Copy link
Contributor

@CesarBS: I thought we decided the keepalive should apply to both requests and responses, so the original test is actually better. Is this fix temporary until the keepalive behavior can be changed?

@cesarblum
Copy link
Contributor Author

That's right, it's temporary. The timeout implementation needs to be revisited, but this keeps the CI good for now.

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

Successfully merging this pull request may close these issues.

6 participants