-
Notifications
You must be signed in to change notification settings - Fork 436
Split CI script into separate GitHub Actions steps #4357
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
|
👋 Thanks for assigning @tnull as a reviewer! |
ci/ci-tests.sh
Outdated
| } | ||
|
|
||
| # Initialize GitHub Actions Job Summary | ||
| if [ -n "$GITHUB_STEP_SUMMARY" ]; then |
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.
Given this is just nice printing should we always turn it on rather than only for CI?
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.
We can do that, but in which file would you want to save the output? The file is, during CI, in $GITHUB_STEP_SUMMARY
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.
Oh I just skimmed it and thought it printed didn't write to a file.
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.
Wasn't happy with the way github does the step summary. Now pushed a different approach that just splits up the workflow in more granular steps.
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.
Bleh, no, ci-tests.sh exists so that we're not tied to github and so that people can run it locally. Splitting it into a million bits breaks those existing goals.
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.
You can still run ci-tests.sh
13792ed to
03aa9f1
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4357 +/- ##
==========================================
- Coverage 86.08% 86.00% -0.09%
==========================================
Files 156 156
Lines 102428 102641 +213
Branches 102428 102641 +213
==========================================
+ Hits 88179 88277 +98
- Misses 11754 11857 +103
- Partials 2495 2507 +12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Not happy with this yet, exploring other options. |
4f74e2d to
6f524c7
Compare
TheBlueMatt
left a comment
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'm still not really sold on this - we're gonna end up missing a step that we add in CI or something and is more logic in ci-tests.sh really helpful - how often are you actually looking at individual steps to understand the performance profile of ci-tests.sh?
|
It's not so much about performance profile anymore. That was what I had initially. The current shape of it is to make it easy to see which test failed and jump to log files. It's friction that I experience with one big log file. Adding things in CI is not something that happens very often. I think it is worth doing this change. If you add to CI and follow the pattern, it seems unlikely that something is forgotten. |
|
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
tnull
left a comment
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.
Concept ACK.
I like the idea as it makes it easier to just run a specific subset of tests/checks, while still maintaining backwards compat.
Breaking the CI tests into individual steps might also be a good first step before we move some of them into a nightly job eventually?
ci/ci-tests.sh
Outdated
| workspace-tests | ||
| ldk-upgrade-tests | ||
| workspace-member-checks | ||
| lightning-dnssec |
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.
Can we make the crate-related step naming consistent? Currently some are using just an abbreviated prefix (tx-sync, block-sync), while others like this spell out the full crate name. If we intend this to be used by devs regularly, would be good to make the step names predictable, so that we don't have to always look up what the exact step is called.
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.
Yes, good one, done.
Also renamed step IDs to be action-oriented (e.g., test-lightning-block-sync instead of lightning-block-sync-tests), matching how the echo statements and GitHub step names already use verbs like "Testing..." or "Checking...".
e08a604 to
8a620a5
Compare
But even with more discrete steps you're still scrolling up until you find the
Doesn't this happen automatically (after github finishes loading....) I'm really not happy seeing a laundry list of steps copied between the bash script and github config - these are inevitably going to get out of sync and we'll end up having steps skipped in CI. |
Right, I don't disagree, but this doesn't fix that? There are still multiple commands run in many steps and you still have to scoll up and find the one that failed. #4368 should improve things a bit, but if you actually want to fix this problem probably we need something in |
|
Agreed that #4368 improves it a bit, but I still think that also having these github-native sections is better. Even if they sometimes group multiple commands. |
|
Worth noting that this also helps when waiting for CI to complete, which can take a while. With the current single-step approach there's no way to tell how far along a run is. You just see it running. With split steps you can see which step is currently executing, giving a sense of progress. GitHub also shows timing per step, which makes it easier to identify where optimization efforts would have the most impact. |
Hmm, I see the point about things potentially getting out-of-sync at some point. No strong opinion either way, but even if we end up not doing the CI changes, can we at least keep the splittin of |
|
The out of sync risk is addressed in the extra commit that I pushed where completion of all steps is verified. I also think that step selection can be useful in CI other than for reporting purposes. Currently our CI is painfully slow, and there are probably build matrix / step combinations that can for example be moved to a nightly job. |
Refactor ci-tests.sh to support running individual test steps by name, allowing the GitHub Actions workflow to display each step separately. This provides clearer feedback when a specific test fails, as each step now appears as its own workflow step rather than being buried in one monolithic script run. The script now accepts an optional step name argument to run a single step, or runs all steps when no argument is provided. A verification step ensures all expected steps were executed. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
c5aa8da to
d566539
Compare
|
Rebased onto |

Summary
ci-tests.shto accept an optional step name argument, running only that step when providedbuild.yml./ci/ci-tests.shwithout arguments still executes all stepsMotivation
GitHub Actions UI now shows individual test step progress, making it easier to identify which specific test phase failed without scrolling through logs.