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

Attempt to fix timing test CI failure. #66

Merged
merged 1 commit into from
Aug 13, 2024

Conversation

michael-grunder
Copy link
Collaborator

@michael-grunder michael-grunder commented Aug 12, 2024

  • Reduce sleep time from 3s to 1s. No need to sleep so long.
  • Relax tolerances so we can be pretty sure the server has awakened when we make the next attempt.

@michael-grunder michael-grunder requested a review from bjosv August 12, 2024 22:23
@michael-grunder michael-grunder force-pushed the fix-timing-test branch 2 times, most recently from f5fab5a to e352bb8 Compare August 13, 2024 06:58
Copy link
Collaborator

@bjosv bjosv left a comment

Choose a reason for hiding this comment

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

Great finding.
I ran the target separately and your change seems to fix it.
When running the tests 20 times in a row, within the same job, it failed 10 of 20 without the change.
With this fix it failed 1 of 20. This is good enough, but extending the time just a bit more to 1.5s would be ok for me. Then it was 0 of 20 fails when I ran a couple of times.

tests/client_test.c Show resolved Hide resolved
tests/client_test.c Outdated Show resolved Hide resolved
* Reduce sleep time from 3s to 1s. No need to sleep so long.
* Sleep for additional time to account for timing/preemption issues in
  the CI runner.

Signed-off-by: michael-grunder <[email protected]>
@michael-grunder michael-grunder merged commit 9188dbd into valkey-io:main Aug 13, 2024
15 of 41 checks passed
@michael-grunder michael-grunder deleted the fix-timing-test branch August 13, 2024 15:02
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.

2 participants