ci: Add test-each-commit task #28279

Currently, if a pull request has more than one commit, previous commits may fail to compile, or may fail the tests. This is problematic, because it breaks git-bisect, or worse.

Fix this by adding a CI task for this.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Will squash if there is conceptual agreement (Not sure when this concept last came up, but I think @dergoegge and @achow101 mentioned it?). Review question: Looks like the worst-case runtime is 1 hour per commit, so with GHA at most 6 commits could be tested before a timeout. Maybe a self-hosted worker with a longer timeout would be better?

Concept ACK

Maybe a self-hosted worker with a longer timeout would be better?

Probably but I am wondering a bit if these task could end up clogging our self-hosted worker queue and hinder other jobs from running in their regular time?

Probably but I am wondering a bit if these task could end up clogging our self-hosted worker queue and hinder other jobs from running in their regular time?

Right, that is another reason for self-hosted runners, because it is possible to assign a label to them, so that they are task-specific, and not interfere with other tasks. On GHA "free" runners will stop being spun up after n total parallel running tasks.

This would be a cirrus self-hosted worker though, right?

Concept ACK if our CI infra can handle it. I build and run the unit tests on each commit for more critical pulls, or pulls with commits that aren't clearly separate or that look interactive, or those with many commits, but often without also running the functional tests that take longer to run. Having the CI verify this would alert PR authors more quickly and save developer time for everyone.

often without also running the functional tests that take longer to run

That said, what my build+test checks most often find is a failing build before running any tests. So if infra resources are limited, just build (or build + units) might still catch most of the issues.

I think it is fine to use GHA for now, with a limit of 6 commits. This should cover 99% of all pull requests. If there is a need for a self-hosted runner, it can trivially be added later.

Concept ACK.

Currently, if a pull request has more than one commit, previous commits may fail to compile, or may fail the tests. This is problematic, because it breaks git-bisect, or worse.

I've encountered such cases during my reviews.

On GHA "free" runners will stop being spun up after n total parallel running tasks.

n == 20

On GHA "free" runners will stop being spun up after n total parallel running tasks.

n == 20

Sure, but as I said, we can use self-hosted runners if and when there is a need. This check will take a few seconds on pulls with one commit (I can reduce this, if needed), which should be the most common type of pull request. Also, there is a limit of 6 hours, so this should be fine as well, unless many pull requests with many commits are pushed to at the same time, in which case there likely is a backlog either way.

Approach ACK fac02205ff59467dd061429e469d4fd07fcc5ed5.

What is the way to figure out which commit failed?

What is the way to figure out which commit failed?

Compile each commit locally, I guess? See the existing docs on how to do that.

What is the way to figure out which commit failed?

Compile each commit locally, I guess? See the existing docs on how to do that.

Yes, it is what is expected from a developer in first place before they push their branch.

But if we add a CI task that tests every commit, it is reasonable, at least for me, to expect it prints a commit hash being tested somewhere.

ACK faed0f4ed43e9416eb0620db1943c9ae083a5b50

utACK faed0f4ed43e9416eb0620db1943c9ae083a5b50

Small fixup to remove the unused/disabled --interactive flag. Should be trivial to re-ACK.

utACK fa5356cd49facf195447f0f5921dce1fa53cb25d

ACK fa5356cd49facf195447f0f5921dce1fa53cb25d

rfm or is there anything left to be done here?

Per https://github.com/bitcoin/bitcoin/actions/runs/6075652138/job/16482267917?pr=28279

Running test-one-commit on commit fafcd2e9ef1209d614de5763a2733098537919dd Author: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Wed Aug 16 16:40:42 2023 +0200 ci: Add test-each-commit task

ACK fa5356cd49facf195447f0f5921dce1fa53cb25d

Modulo one question in #28279 (review).

Looks like this is currently failing for new PRs: https://github.com/bitcoin/bitcoin/actions/runs/6184007676/job/16786866270?pr=28476

Run git rebase --exec "echo Running test-one-commit on \$( git log -1 ) && ./autogen.sh && CC=clang CXX=clang++ ./configure && make clean && make -j $(nproc) check && ./test/functional/test_runner.py -j $(( $(nproc) * 2 ))" "$( git log '^'$( git log --merges -1 --format='%H' ) HEAD --format='%H' ${MAX_COUNT} | tail -1 )~1"
fatal: invalid upstream '~1'
Error: Process completed with exit code 128.

I guess the issue is that ( git log '^'$( git log --merges -1 --format='%H' ) HEAD --format='%H' ${MAX_COUNT} | tail -1 ) prints nothing for some reason when there is only one commit in the pull request?

Not sure why, because locally it works:

$ git --version 
git version 2.41.0
$ git log -1 --format='%H'
3c99f66f8a10aa4dfc1e85c3a240cc144442eac7
$ ( git log '^'$( git log --merges -1 --format='%H' ) HEAD --format='%H' ${MAX_COUNT} | tail -1 ) 
3c99f66f8a10aa4dfc1e85c3a240cc144442eac7