-
Notifications
You must be signed in to change notification settings - Fork 130
ci(l1): improve snapsync smoketest #5598
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR enhances the snapsync smoketest by increasing its frequency and expanding test coverage to include multiple consensus clients. The changes run the smoketest every 3 hours instead of daily and test against both Lighthouse and Prysm consensus clients.
- Increases test frequency from daily (once at 03:00 UTC) to every 3 hours
- Adds parallel testing with two consensus clients: Lighthouse and Prysm
- Parameterizes the consensus layer Docker image in the reusable action
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
.github/workflows/daily_snapsync.yaml |
Updates cron schedule to run every 3 hours and splits the sync job into two parallel jobs for Lighthouse and Prysm consensus clients |
.github/actions/snapsync-run/action.yml |
Adds cl_image input parameter to allow specifying different consensus client images, making the action reusable for multiple clients |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Every 3 hours | ||
| - cron: "0 */3 * * *" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this work? as far as i Understand this 2 jobs will run sequentially (i might be wrong) taking nearly 2hs each. Also I'm not sure were the timeout is set i se the timeout true but missed the actual number.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's set in a somewhat complicated way in the prepare step. I'm not sure why it was done like that when strategy.matrix.include would have done the trick AFAICT, but it predates this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked and running hoodi+sepolia took <1h30m, so 2x that should be right on time. We can bump this to 4h, but I feel it's better to try with 3h first and bump it if we see the need for it.
| network: ${{ matrix.network }} | ||
| timeout: ${{ matrix.timeout }} | ||
| cl_type: lighthouse | ||
| cl_image: "sigp/lighthouse:v8.0.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's the exact same workflow except for this, consider setting these in the strategy matrix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered it, but it's being specified dynamically in another job. I feel this was easier and more clear, and we have plans to move this outside GHA anyways.
Motivation
We found the smoketest lacking.
Description
This PR changes the snapsync smoketest job to run every 3 hours, with both Lighthouse and Prysm consensus clients.