Skip to content

Conversation

@joostjager
Copy link
Contributor

@joostjager joostjager commented Jan 28, 2026

Summary

  • Modifies ci-tests.sh to accept an optional step name argument, running only that step when provided
  • Splits the single "Run CI script" workflow step into 18 individual steps in build.yml
  • Preserves backward compatibility: running ./ci/ci-tests.sh without arguments still executes all steps

Motivation

GitHub Actions UI now shows individual test step progress, making it easier to identify which specific test phase failed without scrolling through logs.

image

@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Jan 28, 2026

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@joostjager joostjager changed the title ci: Add GitHub Job Summary reporting to ci-tests.sh Add GitHub Job Summary reporting to ci-tests.sh Jan 28, 2026
ci/ci-tests.sh Outdated
}

# Initialize GitHub Actions Job Summary
if [ -n "$GITHUB_STEP_SUMMARY" ]; then
Copy link
Collaborator

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?

Copy link
Contributor Author

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

Copy link
Collaborator

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.

Copy link
Contributor Author

@joostjager joostjager Jan 29, 2026

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.

Copy link
Collaborator

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.

Copy link
Contributor Author

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

@codecov
Copy link

codecov bot commented Jan 28, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.00%. Comparing base (9e91b2e) to head (c5aa8da).
⚠️ Report is 61 commits behind head on main.

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     
Flag Coverage Δ
tests 86.00% <ø> (-0.09%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@joostjager
Copy link
Contributor Author

Not happy with this yet, exploring other options.

@joostjager joostjager changed the title Add GitHub Job Summary reporting to ci-tests.sh Split CI script into separate GitHub Actions steps Jan 29, 2026
@joostjager joostjager marked this pull request as ready for review January 30, 2026 11:09
@joostjager joostjager requested a review from tnull January 30, 2026 11:09
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a 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?

@joostjager
Copy link
Contributor Author

joostjager commented Jan 30, 2026

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.

@ldk-reviews-bot
Copy link

🔔 1st Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

Copy link
Contributor

@tnull tnull left a 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
Copy link
Contributor

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.

Copy link
Contributor Author

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...".

This allows GitHub Actions to show individual test step progress in the
UI, making it easier to identify which specific test phase failed
without scrolling through logs.

The ci-tests.sh script now accepts an optional step name argument. When
provided, it runs only that step. When omitted, it runs all steps as
before, preserving backward compatibility for local testing.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@TheBlueMatt
Copy link
Collaborator

The current shape of it is to make it easy to see which test failed

But even with more discrete steps you're still scrolling up until you find the + ... line to see which command specifically failed? I'm not sure how this helps.

and jump to log files. It's friction that I experience with one big log file.

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.

Adds a --verify-complete flag to ci-tests.sh that checks all steps
defined in ALL_STEPS were actually run. Each step logs its completion
to GITHUB_ENV, and a final verification step ensures nothing was
missed. This prevents CI steps from being silently skipped if the
workflow and script get out of sync.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@joostjager
Copy link
Contributor Author

joostjager commented Feb 2, 2026

But even with more discrete steps you're still scrolling up until you find the + ... line to see which command specifically failed? I'm not sure how this helps.

I always find it hard to scroll up in the long log output to find the last command executed, and not scroll further up to an earlier command and incorrectly think that that is the failing one. I think the following is an improvement to that:

image

Doesn't this happen automatically (after github finishes loading....)

It jumps to the very end indeed, but I cannot see directly what it was doing.

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.

I may hope the set of tests is relatively stable, and getting out of sync is therefore very unlikely. But pushed a commit that does a verification at the end to catch it anyway.

@TheBlueMatt
Copy link
Collaborator

I always find it hard to scroll up in the long log output to find the last command executed, and not scroll further up to an earlier command and incorrectly think that that is the failing one

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 ci-tests.sh that simply catches errors and prints the command that errored.

@joostjager
Copy link
Contributor Author

joostjager commented Feb 2, 2026

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. ci-test.sh can print the command that failed, but then it's still required to scroll up to get to the start of the log.

@joostjager
Copy link
Contributor Author

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.

@tnull
Copy link
Contributor

tnull commented Feb 3, 2026

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.

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 ci-tests.sh? That would be pretty handy for local testing, IMO.

@joostjager
Copy link
Contributor Author

joostjager commented Feb 3, 2026

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.

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.

4 participants