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

Fix the ping test by adding a delay to the write operation #41

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Matthias247
Copy link

The TestPing unit test failed on my machine since it always reported a rtt of 0. I guess that is since the Go timers are not very accurate on windows and writing through pipes is really fast.

I fixed that now by introducing a delay/sleep to the write.
It's a little hacky since I grab and modify the connection from the session which would be unsafe if it sends anything while not externally called. But I think it doesn't do that as long as there are not streams, pings or keepalives running and works for that test.

The other alternative is to introduce a general delay option to pipeConn which I did first. But there unit tests need to be modified in lots of places to set that new parameter.

@hashicorp-cla
Copy link

hashicorp-cla commented Dec 6, 2019

CLA assistant check

Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement

Learn more about why HashiCorp requires a CLA and what the CLA includes


1 out of 2 committers have signed the CLA.

  • evanphx
  • Matthias247

Have you signed the CLA already but the status is still pending? Recheck it.

@evanphx
Copy link
Contributor

evanphx commented Jul 28, 2022

This is a good fix for the tests, @Matthias247 I realize this is quite old, did you want to sign the CLA so I can merge this? Thanks!

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.

3 participants