ci: Make the max number of commits tested explicit

hodlinator commented at 9:45 am on November 19, 2025: contributor

Gives less of a false sense of security.

DrahtBot added the label Tests on Nov 19, 2025

DrahtBot commented at 9:45 am on November 19, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33909.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	maflcko, rkrux, janb84
Concept ACK	l0rinc

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

maflcko commented at 10:49 am on November 19, 2025: member

Not sure, this will bloat the title, so that it can’t be fully read anymore:

Also, the title seems the wrong place to list all the shortcomings of each CI task. For example, the win-cross does not build the gui tests, should it say ’no-gui-tests’? Or the arm test has -Wno-error=maybe-uninitialized, so should the title say ’no-error-uninit’? Some tasks do not have CI_LIMIT_STACK_SIZE set, some tasks don’t run the previous releases, or the extended functional tests, some have the gui disabled, no task is using gcc-15, …

Maybe it makes sense to document this better, but I think the title is the wrong approach.

hodlinator commented at 11:09 am on November 19, 2025: contributor

Thanks for the feedback! “test max 6 ancestor commits of H…” is still pretty informative IMO. When not bolded, on larger resolutions, one can still see the whole name. And when selecting a job one sees the full name (as in this “macOS native, no depends” example to the right).

I think documenting at least some limitations of tests in this way makes knowledge less tribal. (The first time I heard of the 6 commit limit was an out-of-band conversion).

Another option might be to emit a message when the number of ancestors are above 6 using https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-commands#setting-a-warning-message

But ideally I want this important limitation to be clear in the list of checks in the main PR tab:

in .github/workflows/ci.yml:59 in d14d1b52b5

55@@ -56,7 +56,7 @@ jobs:
56           fi
57 
58   test-each-commit:
59-    name: 'test each commit'
60+    name: 'test max 6 ancestor commits of HEAD'

l0rinc commented at 11:37 am on November 19, 2025:

6 is an implementation detail, the high-level point is to signal that not every commit is checked, but that the last few commits in the current PR are. Also, the job’s name should also likely be adjusted, maybe ci-test-each-commit-exec.py as well:

0  test-latest-commits:
1    name: 'test latest commits'

maflcko commented at 12:17 pm on November 19, 2025:

The last commit isn’t checked either. Maybe ’test some mid commits'

l0rinc commented at 12:19 pm on November 19, 2025:

But that’s just an optimization, since that’s checked elsewhere, no need to point it out here, it’s not part of reducing “false sense of security”

maflcko commented at 12:23 pm on November 19, 2025:

Someone could introduce an inverse silent merge conflict, so in theory that is a “false sense of security”

l0rinc commented at 12:40 pm on November 19, 2025:

Isn’t that checked by another CI job already?

maflcko commented at 12:43 pm on November 19, 2025:

Which one CI job tests for inverse silent merge conflicts, where the merged result is passing tests, but the raw commit itself is not?

l0rinc commented at 12:47 pm on November 19, 2025:

ah, since this is the only one that does a merge with master? So why are we even skipping HEAD if other jobs aren’t doing it?

maflcko commented at 1:22 pm on November 19, 2025:

ah, since this is the only one that does a merge with master?

Ah, actually all of them do it, and no task is checking for inverse silent merge conflicts.

So why are we even skipping HEAD if other jobs aren’t doing it?

CI can never catch all issues. So at least for issues that are not essential, like silent inverse merge conflicts, it seems ok to not check them.

Same for testing each commit. I mean, it is always great if all commits can be bisected, but test-each-commit is only checking one compiler and not running the fuzz tests. There are many shortcomings and it is never possible to even list all of them (let alone run them).

hodlinator commented at 9:25 pm on November 27, 2025:

I’ve spent more time than I care to admit together with an underpowered LLM and StackOverflow to come up with ways to avoid duplicating the number 6 in both the job’s name and the MAX_COUNT constant.

Haven’t found a way to access env.* when setting name, or to access name when setting env.* (github.job returns “test-each-commit”). I’ve also explored doing it through YAML node aliases, but that doesn’t seem to work either.

I want to specify the 6 in the name in order to make it immediately explicit when looking at the bottom of a PR Conversation tab. (See lower screenshot: #33909 (comment)).

At this point I think keeping them in sync is worth the trouble. Added comment after MAX_COUNT in latest push to that affect.

In hindsight I don’t think the “of HEAD”-suffix adds much (and makes the name harder to fit, see #33909 (comment)), so removed it.

l0rinc commented at 9:51 pm on November 27, 2025:

Why is it better to duplicate than to abstract the above to what we should pay attention to? It’s not the 6 that matters, but that not all of them, so why not focus on that instead?

hodlinator commented at 9:55 pm on November 27, 2025:

Hm.. I guess a shorter version of “test a limited number of ancestor commits” could work. “test a few ancestor commits”?

l0rinc commented at 10:03 pm on November 27, 2025:

It should signal that it’s not exhaustive, something slightly ambiguous like “test latest commits” or “test latest few commits”?

hodlinator commented at 8:05 am on November 28, 2025:

“test latest commits” is incrementally better than master. But it is a bit ambiguous - especially for a newcomer who didn’t know it by its old name, almost like if it were to be testing on master without any commits from the PR. Curious what others prefer.

janb84 commented at 8:40 am on November 28, 2025:

“test latest commits” would indicate to me that it would test all the commits in the PR (even if >6) , “test latest few commits” would be better imho.

Or even more contrarian, lean into curiosity and make it “test some latest commits”

l0rinc commented at 10:40 am on November 28, 2025:

I don’t think this one is better than the version on master, newcomers could still be confused by what happens if they only have two commits…

l0rinc commented at 11:40 am on November 19, 2025: contributor

I didn’t realize this, so concept ACK.

I wouldn’t include the value of a variable in the variable’s name, though, and I think the job’s name should likely also be updated - left a suggestion

janb84 commented at 11:46 am on November 19, 2025: contributor

Also, the title seems the wrong place to list all the shortcomings of each CI task. For example, the win-cross does not build the gui tests, should it say ’no-gui-tests’? Or the arm test has -Wno-error=maybe-uninitialized, so should the title say ’no-error-uninit’? Some tasks do not have CI_LIMIT_STACK_SIZE set, some tasks don’t run the previous releases, or the extended functional tests, some have the gui disabled, no task is using gcc-15, …

Maybe it makes sense to document this better, but I think the title is the wrong approach.

PR #33824 changes the title of some of the CI runs which (maybe) address some of your concerns (as a side note)

in .github/workflows/ci.yml:64 in d14d1b52b5

kevkevinpal commented at 11:56 am on November 19, 2025:

is this the reason for the limitation? or are there other reasons?

if its just this couldn’t we just get how many commits are in this branch and use it here?

l0rinc commented at 12:07 pm on November 19, 2025:

See the explanation above:

timeout-minutes: 360  # Use maximum time, see https://docs.github.com/en/actions/writing-workflows/workflow-syntax-for-github-actions#jobsjob_idtimeout-minutes. Assuming a worst case time of 1 hour per commit, this leads to a --max-count=6 below.

in .github/workflows/ci.yml:58 in d14d1b52b5 outdated

55@@ -56,7 +56,7 @@ jobs:
56           fi
57 
58   test-each-commit:

rkrux commented at 11:01 am on November 24, 2025:

This or something similar?

0- test-each-commit:
1+ test-last-few-ancestor-commits:

hodlinator commented at 9:16 pm on November 27, 2025:

Explored that now, but we still have the ci-test-each-commit-exec.py script…

https://github.com/bitcoin/bitcoin/blob/d14d1b52b51c4e8e2388af7e54c854e8a133e088/.github/workflows/ci.yml#L113

…and unlike the name, this job “id” is not externally facing, so I’d rather not touch it.

rkrux commented at 11:11 am on November 24, 2025: contributor

ACK d14d1b52b51c4e8e2388af7e54c854e8a133e088

I vaguely recall noticing this limit some time back. Makes sense to me that it is highlighted in the job name.

DrahtBot requested review from l0rinc on Nov 24, 2025

ci: Make the max number of commits tested explicit

Gives less of a false sense of security.

b5a7a685bb

hodlinator force-pushed on Nov 27, 2025

maflcko commented at 7:13 am on November 28, 2025: member

lgtm ACK b5a7a685bba312a780eddcb4a53ce2c26a937854

DrahtBot requested review from rkrux on Nov 28, 2025

rkrux approved

rkrux commented at 8:02 am on November 28, 2025: contributor

crACK b5a7a685bba312a780eddcb4a53ce2c26a937854

janb84 commented at 9:15 am on November 28, 2025: contributor

ACK b5a7a685bba312a780eddcb4a53ce2c26a937854

The PR removes incorrectness in the title of the the task “test-each-commit”, which doesn’t checks each commit, to make it more explicit.

This will help (starting) contributors, in my opinion, to have a better mental model of what is checked in the CI and what isn’t. Therefor this is an valuable improvement and worthy of the code churn.

ci: Make the max number of commits tested explicit #33909

Code Coverage & Benchmarks

Reviews