ci: Add test-each-commit task #28279

pull maflcko wants to merge 2 commits into bitcoin:master from maflcko:2308-ci-test-each-commit- changing 1 files +17 −0
  1. 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.

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

  3. DrahtBot added the label Tests on Aug 16, 2023
  4. maflcko force-pushed on Aug 16, 2023
  5. maflcko force-pushed on Aug 16, 2023
  6. maflcko force-pushed on Aug 16, 2023
  7. maflcko force-pushed on Aug 17, 2023
  8. maflcko force-pushed on Aug 17, 2023
  9. 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?
  10. 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?

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

  12. dergoegge commented at 12:09 pm on August 17, 2023: member
    This would be a cirrus self-hosted worker though, right?
  13. jonatack commented at 4:26 pm on August 17, 2023: member
    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.
  14. jonatack commented at 4:28 pm on August 17, 2023: member

    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.

  15. maflcko marked this as ready for review on Aug 21, 2023
  16. maflcko force-pushed on Aug 21, 2023
  17. 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.
  18. maflcko force-pushed on Aug 21, 2023
  19. maflcko force-pushed on Aug 21, 2023
  20. maflcko force-pushed on Aug 21, 2023
  21. 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.

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

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

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

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

    Approach ACK fac02205ff59467dd061429e469d4fd07fcc5ed5.

    What is the way to figure out which commit failed?

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

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

  28. maflcko force-pushed on Aug 23, 2023
  29. hebasto approved
  30. hebasto commented at 9:41 am on August 23, 2023: member
    ACK faed0f4ed43e9416eb0620db1943c9ae083a5b50
  31. maflcko requested review from jonatack on Aug 28, 2023
  32. maflcko requested review from dergoegge on Aug 28, 2023
  33. jonatack commented at 5:55 pm on August 30, 2023: member
    utACK faed0f4ed43e9416eb0620db1943c9ae083a5b50
  34. DrahtBot added the label CI failed on Sep 4, 2023
  35. DrahtBot removed the label CI failed on Sep 4, 2023
  36. ci: Add test-each-commit task fafcd2e9ef
  37. ci: Limit test-each-commit to --max-count=6 fa5356cd49
  38. maflcko force-pushed on Sep 4, 2023
  39. 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.
  40. dergoegge approved
  41. dergoegge commented at 11:05 am on September 11, 2023: member
    utACK fa5356cd49facf195447f0f5921dce1fa53cb25d
  42. DrahtBot requested review from jonatack on Sep 11, 2023
  43. DrahtBot requested review from hebasto on Sep 11, 2023
  44. 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.

  45. 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.
  46. hebasto approved
  47. hebasto commented at 12:19 pm on September 11, 2023: member
    ACK fa5356cd49facf195447f0f5921dce1fa53cb25d
  48. maflcko commented at 11:43 am on September 12, 2023: member
    rfm or is there anything left to be done here?
  49. jonatack commented at 9:39 pm on September 13, 2023: member

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

  50. DrahtBot removed review request from jonatack on Sep 13, 2023
  51. fanquake merged this on Sep 14, 2023
  52. fanquake closed this on Sep 14, 2023
  53. maflcko deleted the branch on Sep 14, 2023
  54. 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.
  55. 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
  56. maflcko referenced this in commit fa2cb2f5d3 on Sep 14, 2023
  57. fanquake referenced this in commit f5c5ddafbc on Sep 14, 2023
  58. Frank-GER referenced this in commit d61b597dc5 on Sep 19, 2023
  59. Frank-GER referenced this in commit 53fbb5ebc0 on Sep 19, 2023
  60. fanquake referenced this in commit 737aac8cc8 on Sep 19, 2023
  61. russeree referenced this in commit a5ba314ff6 on Sep 20, 2023
  62. Frank-GER referenced this in commit 3c1de58e24 on Sep 25, 2023
  63. sidhujag referenced this in commit 0592813fca on Sep 26, 2023
  64. Retropex referenced this in commit 54d8fcdef0 on Oct 4, 2023
  65. Retropex referenced this in commit 5f62ed90f6 on Oct 4, 2023
  66. janus referenced this in commit 3136750d64 on Apr 1, 2024
  67. bitcoin locked this on Dec 5, 2024


maflcko DrahtBot dergoegge jonatack hebasto fanquake

Labels
Tests

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-22 15:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me