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 client function to return ping latency #201

Merged
merged 6 commits into from
Sep 27, 2024
Merged

Conversation

douglascdev
Copy link
Contributor

Thought it would be useful to have a client function that returns ping latency like dank-twitch-irc does.

@coveralls
Copy link

coveralls commented Sep 24, 2024

Coverage Status

coverage: 98.036% (+0.04%) from 98.0%
when pulling 2519e47 on douglascdev:latency
into 81d96cf on gempir:master.

@gempir
Copy link
Owner

gempir commented Sep 24, 2024

Nice idea. But I'd like a test.

@douglascdev
Copy link
Contributor Author

Nice idea. But I'd like a test.

Added some tests, let me know if you'd like any changes :)

client_test.go Outdated
return diff
}()

if latencyDiff > time.Millisecond*3 {
Copy link
Owner

Choose a reason for hiding this comment

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

This is so flakey... I don't like this at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah true but I don't see an alternative, I was getting 0ms locally and it worked consistently, but when actions runs the test it fails. Do you have a better idea?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could try reverting the last 2 commits and using returnedLatency.Truncate instead ofreturnedLatency.Round, but even if that works now I'm not sure it won't eventually cause your tests to fail because actions ran too slowly.

@gempir gempir merged commit 477f613 into gempir:master Sep 27, 2024
6 checks passed
@gempir
Copy link
Owner

gempir commented Sep 27, 2024

Thanks for the contribution!

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