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

ci: tone down ci some more #492

Merged
merged 3 commits into from
May 17, 2023
Merged

ci: tone down ci some more #492

merged 3 commits into from
May 17, 2023

Conversation

PaulRBerg
Copy link
Member

We didn't tone CI down enough in #474. Some workflows are still taking a long time to run:

https://github.com/sablierhq/v2-core/actions/runs/4984025835/jobs/8921863702?pr=490

Copy link
Member

@andreivladbrg andreivladbrg left a comment

Choose a reason for hiding this comment

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

It looks like the time for the invariant tests has decreased significantly, but not so much for the fork and coverage. I believe this issue is caused by the foundry.

https://github.com/sablierhq/v2-core/actions/runs/4984576283

@PaulRBerg
Copy link
Member Author

PaulRBerg commented May 16, 2023

That's odd .. the need to communicate to the RPC might explain the fork tests, but the coverage job should have been faster because of the --no-match-path flag:

$ forge coverage --no-match-path "test/fork/**/*.sol" --report lcov

I'll investigate some more before merging this PR.

@PaulRBerg PaulRBerg force-pushed the ci/tone-down branch 4 times, most recently from 989075a to b055ddc Compare May 16, 2023 21:42
@PaulRBerg
Copy link
Member Author

Following up with my investigation on why the coverage job is taking so long (>1 hour) still.

  • It's not the unit tests (I ran them with --match-path)
  • Not the fuzz tests (same as above)
  • Not the fork tests (because --no-match-patch works)
  • It's the invariant tests (I ran them with --match-path)

I proceeded to run the coverage command on my local machine, and noticed the same slow behavior. This has led me to conclude that there is a bug in Foundry, which I reported here:

foundry-rs/foundry#4952

In our PR, I updated the coverage job to include only unit and fuzz tests, like this:

$ forge coverage --match-path "test/{fuzz,unit}/**/*.sol" --report lcov

@andreivladbrg - let me know if this is good to merge now.

@PaulRBerg
Copy link
Member Author

PaulRBerg commented May 17, 2023

I've also investigated why the fork tests take so long (>1 hour). Here are my findings:

  • They take 50% less time to run locally compared to CI, which suggests that the slow execution time in CI is in part due to using the standard GitHub runners, which have 2 cores on Linux
  • The issue may be that we are getting rate limited by the RPC node provider (see my tweet about this). Some web socket connections may stall, temporarily, until the rate limit is lifted.

We might be able to speed up the CI run time by upgrading to larger GitHub runners with more cores, although this would require us to upgrade to a paid GitHub plan (e.g. Team). Similarly, we might get rate limited less often if we upgraded to a higher plan on Alchemy and/ or ChainNodes.

Anyway. In the meantime, I will simply lower the number of fuzz runs in the test-fork job. We get enough test coverage via the ci-deep workflow.

Update: it is also unclear how Foundry handles RPC retries. See foundry-rs/foundry#4958.

PaulRBerg added 3 commits May 17, 2023 12:59
ci: disable fork tests in coverage job
test: remove "fuzz" and "invariant" config for "test-optimized" profile
@PaulRBerg
Copy link
Member Author

I will simply lower the number of fuzz runs in the test-fork job

Seems like this approach is working: the fork tests take ~15 minutes to run now:

https://github.com/sablierhq/v2-core/actions/runs/5002102460/jobs/8962236042?pr=492

Merging.

@PaulRBerg PaulRBerg merged commit 729e171 into main May 17, 2023
@PaulRBerg PaulRBerg deleted the ci/tone-down branch May 18, 2023 11:06
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