Skip to content

ci: run build and test for each commit in pushed range#1040

Open
Vedd-Patel wants to merge 1 commit into
getfloresta:masterfrom
Vedd-Patel:ci(#708)/run-checks-for-each-commit
Open

ci: run build and test for each commit in pushed range#1040
Vedd-Patel wants to merge 1 commit into
getfloresta:masterfrom
Vedd-Patel:ci(#708)/run-checks-for-each-commit

Conversation

@Vedd-Patel
Copy link
Copy Markdown

Description and Notes

FIXES #708

this PR adds a new CI workflow that runs cargo build and cargo test on every intermediate commit in a push batch to ensure PR history remains fully compilable

notes:

  • uses a manual git checkout --detach loop instead of git rebase --exec for better reliability in headless CI
  • tests run in --release mode to prevent the floresta-rpc tests from hitting the 30-second node startup timeout
  • integrates the existing bi-weekly cache strategy so the per-commit checks run quickly

How to verify the changes you have done?

push a branch with multiple commits to your fork, the new per-commit tests action will run alongside the standard CI, logging each commit it tests and cleanly failing if an intermediate commit is broken

screenshots

below are the screenshot of the result of the per-commit tests

Screenshot 2026-05-09 at 00 49 35

Screenshot 2026-05-09 at 01 32 40

minor note

the below change is intentional(not ai generated), to make the start of every commit check visible

Screenshot 2026-05-09 at 01 28 15

@Vedd-Patel
Copy link
Copy Markdown
Author

@jaoleal @luisschwab take a look at this when you get a moment!

@jaoleal jaoleal requested review from jaoleal and luisschwab May 8, 2026 21:01
@jaoleal jaoleal added the CI A change or issue related to CI label May 8, 2026
@jaoleal jaoleal added this to Floresta May 8, 2026
@github-project-automation github-project-automation Bot moved this to Backlog in Floresta May 8, 2026
@jaoleal jaoleal added this to the Q2/2026 milestone May 8, 2026
@moisesPompilio
Copy link
Copy Markdown
Collaborator

This isn’t good, because it should be reproducible locally. In this case, you basically wrote everything directly in the CI, but the right approach would be to put it all in a shell script so it can be run locally and tested, since this is now part of the CI flow.

Also, I think the lint should run on every commit too, and if possible, the functional tests as well.

@Vedd-Patel
Copy link
Copy Markdown
Author

Vedd-Patel commented May 9, 2026

@moisesPompilio thanks for the review but here are my thoughts on it,

on the shell script concern, youre right that the logic should be reproducible locally, and it actually is ci-test-each-commit-exec.sh is the script that does all the actual work(build + test), and you can run it direclty on your locall machine, the workflow just calls it via a git checkout loop, which is the same thing you'd do locally(see below)

BASE=$(git merge-base HEAD origin/master)
for SHA in $(git rev-list --reverse "$BASE..HEAD"); do
  git checkout --detach "$SHA"
  bash .github/ci-test-each-commit-exec.sh
done
git checkout -

so the reproducibility is there, the workflow itself is just the glue that computes the push range(github.event.before-> github.sha) and feeds it to the same script but that part is inherently CI-specific sicne it depends on push event metadata that doesnt exist locally

i've took reference from this links you can check it out - https://github.com/bitcoin/bitcoin/blob/master/.github/workflows/ci.yml https://github.com/bitcoin/bitcoin/blob/master/.github/ci-test-each-commit-exec.py

on lint and functional test per commit,

  • lint already runs on the latest commit in the existing CI jobs, running it on each commit add real cost, run teh clippy on full workspace isnt fast for limited benefit, a lint failure on any intermediate commit thats fine on the latest one doesnt actually block the PR, it just add noise in my opinion, if goal of this feature is bisectibility, then complie or test failure is what breaks bisect, a lint warning doesnt

  • functional tests hit almost 30-second node startup timeout per run(i struggled bcz of this in build and test also), with letssay 5 commits in a push thats 5 full node startups just for this job, which would make it the slowest in the entire CI by a long shot, thats why i use --release builds and unit tests only here, it keeps the per-commit runs fast enough to be practical

P.S. - if you think lint is worth adding despite the cost tradeoff, happy to include it, just wanted to be transparent about why it was left out rather than it being oversight

@jaoleal
Copy link
Copy Markdown
Member

jaoleal commented May 9, 2026

P.S. - if you think lint is worth adding despite the cost tradeoff, happy to include it, just wanted to be transparent about why it was left out rather than it being oversight

Relax, bill gates is paying for our CI

@Vedd-Patel
Copy link
Copy Markdown
Author

P.S. - if you think lint is worth adding despite the cost tradeoff, happy to include it, just wanted to be transparent about why it was left out rather than it being oversight

Relax, bill gates is paying for our CI

then i'll add both lint and functional tests :)

@jaoleal
Copy link
Copy Markdown
Member

jaoleal commented May 9, 2026

Also, we still have a lot to improve on the functional tests side.

About lint, If the problem is rust build time, on floresta-nix i think can speed that up, ill go after it when we stop having more emergent priorities. For sure we can talk about that for Q3 and Q4.

@Vedd-Patel
Copy link
Copy Markdown
Author

Also, we still have a lot to improve on the functional tests side.

About lint, If the problem is rust build time, on floresta-nix i think can speed that up, ill go after it when we stop having more emergent priorities. For sure we can talk about that for Q3 and Q4.

But it's better to implement the current lint and functional tests, as the improvement is on hold for now.
(Also, let me know about the improvements on functional tests side, I would love to assist)

@jaoleal
Copy link
Copy Markdown
Member

jaoleal commented May 9, 2026

lint already runs on the latest commit in the existing CI jobs, running it on each commit add real cost, run teh clippy on full workspace isnt fast for limited benefit, a lint failure on any intermediate commit thats fine on the latest one doesnt actually block the PR, it just add noise in my opinion, if goal of this feature is bisectibility, then complie or test failure is what breaks bisect, a lint warning doesnt

Even if its noisy, what we seek here are atomic commits with concise and meaningfull changes. So the tradeoff is worth it, even if the CI takes 1 hour to run, its worth it... The idea is not to delegate for CI to catch problems, but only to publicly attest that things are correct for reviewers

on the shell script concern, youre right that the logic should be reproducible locally, and it actually is ci-test-each-commit-exec.sh is the script that does all the actual work(build + test), and you can run it direclty on your locall machine, the workflow just calls it via a git checkout loop, which is the same thing you'd do locally(see below)

Well ,partially, the script, as it is now on e2154c5, is very simplistic given what we run on our current CI, perhaps the ideal would be to delegate that to our just pcc recipe? On the other side, the CI job is appears to be a little too much to just run a simple check on every commit, theres a lot to dry out just by a superfical look.

But it's better to implement the current lint and functional tests, as the improvement is on hold for now.

Yes, besides, only when we have things running that we will know the weak points to resolve.

(Also, let me know about the improvements on functional tests side, I would love to assist)

Take a read on what i wrote on #1038, its just a sketch on what more to improve but theres already somethings being addressed... These times i also been thinking that literally running the nodes are too much, perhaps for some utreexo related stuff we coul directly use rustreexo-python-ffi ? For bitcoin core stuff we could use bitcoin kernel ? We should have a brainstorm about that.

@luisschwab
Copy link
Copy Markdown
Member

I'd rather have this done when I start the CI overhaul.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI A change or issue related to CI

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

CI: Run checks for each commit, not only the top one

4 participants