ci: Add test-each-commit task

maflcko commented at 2:23 pm on August 16, 2023: member

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.

DrahtBot commented at 2:23 pm on August 16, 2023: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	dergoegge, hebasto, jonatack

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

DrahtBot added the label Tests on Aug 16, 2023

maflcko force-pushed on Aug 16, 2023

maflcko force-pushed on Aug 17, 2023

maflcko commented at 8:45 am on August 17, 2023: member

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?

dergoegge commented at 8:53 am on August 17, 2023: member

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?

maflcko commented at 9:02 am on August 17, 2023: member

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.

dergoegge commented at 12:09 pm on August 17, 2023: member

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

jonatack commented at 4:26 pm on August 17, 2023: contributor

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.

jonatack commented at 4:28 pm on August 17, 2023: contributor

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.

maflcko marked this as ready for review on Aug 21, 2023

maflcko force-pushed on Aug 21, 2023

maflcko commented at 11:49 am on August 21, 2023: member

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.

maflcko force-pushed on Aug 21, 2023

hebasto commented at 2:51 pm on August 21, 2023: member

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.

hebasto commented at 2:53 pm on August 21, 2023: member

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

n == 20

maflcko commented at 12:27 pm on August 22, 2023: member

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.

in .github/workflows/ci.yml:40 in fac02205ff outdated

35+        with:
36+          ref: ${{ github.event.pull_request.head.sha }}
37+          fetch-depth: '8'  # Two more than $MAX_COUNT
38+      - run: git checkout HEAD~  # Skip the top commit, because it is already checked by the other tasks.
39+      - run: sudo apt install ccache build-essential libtool autotools-dev automake pkg-config bsdmainutils python3-zmq libevent-dev libboost-dev libsqlite3-dev libdb++-dev systemtap-sdt-dev libminiupnpc-dev libnatpmp-dev libqt5gui5 libqt5core5a libqt5dbus5 qttools5-dev qttools5-dev-tools qtwayland5 libqrencode-dev -y
40+      - run: EDITOR=true git rebase --interactive --exec "./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"

hebasto commented at 2:42 pm on August 22, 2023:

Out of curiosity, why clang? To catch -Wthread-safety?

maflcko commented at 2:53 pm on August 22, 2023:

It is jammy clang.

It doesn’t catch compile warnings, I think. Only compile errors.

hebasto commented at 2:56 pm on August 22, 2023:

Right, it is configured without --enable-werror.

Why did you choose clang over gcc then?

maflcko commented at 2:59 pm on August 22, 2023:

Why did you chose clang over gcc then?

Clang is generally faster and uses less memory

hebasto commented at 2:44 pm on August 22, 2023: member

Approach ACK fac02205ff59467dd061429e469d4fd07fcc5ed5.

What is the way to figure out which commit failed?

maflcko commented at 2:52 pm on August 22, 2023: member

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.

hebasto commented at 3:00 pm on August 22, 2023: member

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.

maflcko force-pushed on Aug 23, 2023

hebasto approved

hebasto commented at 9:41 am on August 23, 2023: member

ACK faed0f4ed43e9416eb0620db1943c9ae083a5b50

maflcko requested review from jonatack on Aug 28, 2023

maflcko requested review from dergoegge on Aug 28, 2023

jonatack commented at 5:55 pm on August 30, 2023: contributor

utACK faed0f4ed43e9416eb0620db1943c9ae083a5b50

DrahtBot added the label CI failed on Sep 4, 2023

DrahtBot removed the label CI failed on Sep 4, 2023

ci: Add test-each-commit task fafcd2e9ef

ci: Limit test-each-commit to --max-count=6 fa5356cd49

maflcko force-pushed on Sep 4, 2023

maflcko commented at 4:06 pm on September 4, 2023: member

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

dergoegge approved

dergoegge commented at 11:05 am on September 11, 2023: member

utACK fa5356cd49facf195447f0f5921dce1fa53cb25d

DrahtBot requested review from jonatack on Sep 11, 2023

DrahtBot requested review from hebasto on Sep 11, 2023

in .github/workflows/ci.yml:34 in fa5356cd49

29+    if: github.event_name == 'pull_request'
30+    timeout-minutes: 360  # Use maximum time, see https://docs.github.com/en/actions/using-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.
31+    env:
32+      MAX_COUNT: '--max-count=6'
33+    steps:
34+      - uses: actions/checkout@v3

hebasto commented at 12:17 pm on September 11, 2023:

fafcd2e9ef1209d614de5763a2733098537919dd

Suggesting to rebase on top of the #28402 and

0      - uses: actions/checkout@v4

maflcko commented at 12:24 pm on September 11, 2023:

Yes, can be done on the next force push or in the other pull.

jonatack commented at 9:22 pm on September 13, 2023:

#28402 has been merged; it’s not clear to me if this ought to be updated to v4.

maflcko commented at 6:19 am on September 14, 2023:

Ah sorry, I forgot it was already merged.

I don’t think v3 or v4 are any different, so I’ll follow-up with this style nit later.

in .github/workflows/ci.yml:41 in fa5356cd49

40       - run: git checkout HEAD~  # Skip the top commit, because it is already checked by the other tasks.
41-      - run: sudo apt install ccache build-essential libtool autotools-dev automake pkg-config bsdmainutils python3-zmq libevent-dev libboost-dev libsqlite3-dev libdb++-dev systemtap-sdt-dev libminiupnpc-dev libnatpmp-dev libqt5gui5 libqt5core5a libqt5dbus5 qttools5-dev qttools5-dev-tools qtwayland5 libqrencode-dev -y
42-      - run: EDITOR=true git rebase --interactive --exec "./autogen.sh && CC=clang CXX=clang++ ./configure && make clean && make -j $(nproc) check && ./test/functional/test_runner.py -j $(( $(nproc) * 2 ))" $( git log --merges -1 --format='%H' )
43+      - run: sudo apt install clang ccache build-essential libtool autotools-dev automake pkg-config bsdmainutils python3-zmq libevent-dev libboost-dev libsqlite3-dev libdb++-dev systemtap-sdt-dev libminiupnpc-dev libnatpmp-dev libqt5gui5 libqt5core5a libqt5dbus5 qttools5-dev qttools5-dev-tools qtwayland5 libqrencode-dev -y
44+      # Use clang++, because it is a bit faster and uses less memory than g++
45+      - 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"

hebasto commented at 12:19 pm on September 11, 2023:

fa5356cd49facf195447f0f5921dce1fa53cb25d

Why don’t combine unrelated to MAX_COUNT changes into the previous commit?

maflcko commented at 12:25 pm on September 11, 2023:

For review, it may be better to just look at the overall changes. There are two commits, so that the CI can be observed working.

hebasto approved

hebasto commented at 12:19 pm on September 11, 2023: member

ACK fa5356cd49facf195447f0f5921dce1fa53cb25d

maflcko commented at 11:43 am on September 12, 2023: member

rfm or is there anything left to be done here?

jonatack commented at 9:39 pm on September 13, 2023: contributor

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

0Running 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).

DrahtBot removed review request from jonatack on Sep 13, 2023

fanquake merged this on Sep 14, 2023

fanquake closed this on Sep 14, 2023

maflcko deleted the branch on Sep 14, 2023

fanquake commented at 10:18 am on September 14, 2023: member

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

0Run 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"
1fatal: invalid upstream '~1'
2Error: Process completed with exit code 128.

maflcko commented at 12:07 pm on September 14, 2023: member

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:

0$ git --version 
1git version 2.41.0
2$ git log -1 --format='%H'
33c99f66f8a10aa4dfc1e85c3a240cc144442eac7
4$ ( git log '^'$( git log --merges -1 --format='%H' ) HEAD --format='%H' ${MAX_COUNT} | tail -1 ) 
53c99f66f8a10aa4dfc1e85c3a240cc144442eac7

maflcko referenced this in commit fa2cb2f5d3 on Sep 14, 2023

fanquake referenced this in commit f5c5ddafbc on Sep 14, 2023

Frank-GER referenced this in commit d61b597dc5 on Sep 19, 2023

Frank-GER referenced this in commit 53fbb5ebc0 on Sep 19, 2023

fanquake referenced this in commit 737aac8cc8 on Sep 19, 2023

russeree referenced this in commit a5ba314ff6 on Sep 20, 2023

Frank-GER referenced this in commit 3c1de58e24 on Sep 25, 2023

sidhujag referenced this in commit 0592813fca on Sep 26, 2023

Retropex referenced this in commit 54d8fcdef0 on Oct 4, 2023

Retropex referenced this in commit 5f62ed90f6 on Oct 4, 2023

ci: Add test-each-commit task #28279

Reviews