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

Reduce test suite time #656

Merged
merged 4 commits into from
Sep 5, 2024
Merged

Conversation

yuandrew
Copy link
Contributor

@yuandrew yuandrew commented Sep 5, 2024

What was changed

Set cluster cache refresh rate to seconds instead of minutes. Now our test doesn't need to wait 60+ seconds for the cache to refresh

TestOperator_Cluster_Operations goes from ~118.29 seconds to ~4.4 seconds
Overall CI runtime goes from 10m32s to 7m26s (sample size 1)

Why?

Speed up tests

Checklist

  1. Closes [Feature Request] Reduce test suite time #645
  2. How was this tested:

Ran test locally

  1. Any docs updates needed?

@CLAassistant
Copy link

CLAassistant commented Sep 5, 2024

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Member

@cretz cretz left a comment

Choose a reason for hiding this comment

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

This is great, thanks! Ideally we have no sleeps in tests, but even this is a big win. Any flakiness concern on sleeping exactly the amount of the refresh impl? Should we sleep 2s to be sure or reduce the refresh interval (maybe only for this test)?

@yuandrew
Copy link
Contributor Author

yuandrew commented Sep 5, 2024

This is great, thanks! Ideally we have no sleeps in tests, but even this is a big win. Any flakiness concern on sleeping exactly the amount of the refresh impl? Should we sleep 2s to be sure or reduce the refresh interval (maybe only for this test)?

Changing refresh rate to milliseconds takes another second off of the test, having a buffer of 5 millisecond sleep seems to be good, no issues after running test locally 80 times

@cretz
Copy link
Member

cretz commented Sep 5, 2024

Can you confirm with server any harm in changing this to 100ms just for our tests? Meaning, is this max expiry before refetch or is this some kind of loop interval. If no prob, lets do that.

@yuandrew
Copy link
Contributor Author

yuandrew commented Sep 5, 2024

YX confirmed this is fine, only side effect is more DB IO

Jotting down an observation, setting refresh rate to 1 ms causes some tests to fail due to max QPS being reached, but 100 ms seems fine.

@yuandrew yuandrew merged commit 9a7224d into temporalio:main Sep 5, 2024
7 checks passed
@yuandrew yuandrew deleted the reduce-test-time branch September 5, 2024 20:38
@dandavison
Copy link
Contributor

Nice.

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.

[Feature Request] Reduce test suite time
4 participants