ci: Reintroduce fixed “test-each-commit” job #28497

pull hebasto wants to merge 1 commits into bitcoin:master from hebasto:230915-each-commit changing 1 files +22 −0
  1. hebasto commented at 7:59 pm on September 15, 2023: member

    This is a fixed version of #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.

    The new job checks at most 6 commits of a pull request, excluding the top one.

    The maximum number of tested commits is 6, which derives from the time constrains.

    For historical context, please see:

    A note for reviewers: To test scripts locally, ensure that you works with a shallow copy of the repo.

  2. DrahtBot commented at 7:59 pm on September 15, 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 MarcoFalke

    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 Sep 15, 2023
  4. maflcko commented at 9:30 am on September 17, 2023: member

    Can you explain what was wrong in my implementation and how you fixed it?

    lgtm ACK either way. Thanks!

  5. hebasto commented at 12:25 pm on September 18, 2023: member

    Can you explain what was wrong in my implementation and how you fixed it?

    Sure.

    First, let’s reproduce the problem locally. Consider https://github.com/bitcoin/bitcoin/actions/runs/6184348331?pr=26711:

    0$ git init bitcoin
1$ cd bitcoin
2$ git remote add origin https://github.com/bitcoin/bitcoin
3$ git fetch --depth=8 origin 0b98391134811cf0cb809566d67174cd4148364a
4$ git checkout 0b98391134811cf0cb809566d67174cd4148364a

    Now git log --merges -1 --format='%H' produces an empty string, because there are no merge commits indeed:

    0$ git log --oneline 
10b98391 (HEAD) [refactor] use CheckPackageMempoolAcceptResult in previous tests
24c42a23 [unit test] package validation subsets and subpackages
3baf475d [test util] CheckPackageMempoolAcceptResult
42143915 [policy] allow any ancestor package, not just child-with-unconfirmed-parents
56ffdadc [validation] submit ancestor subpackages
6af6eb62 [validation] add a PreChecks loop to linearize by fee and find obvious errors
7f00b0ec [refactor] specify what transactions were too low feerate
805f1e5f (grafted) [validation] fully abort package validation for non-TX_SINGLE_FAILURE failures

    We need to fetch N + 2 commits, where N represents the number of commits in the pull request.

  6. maflcko commented at 3:52 pm on September 18, 2023: member
    Ah thanks, so that was another bug. Though, my approach also failed for pull requests with a single commit.
  7. hebasto commented at 7:28 am on September 19, 2023: member

    Though, my approach also failed for pull requests with a single commit.

    The https://github.com/bitcoin/bitcoin/actions/runs/6184007676?pr=28476 is such a case.

    Reproducing it locally:

    • checkout:
    0$ git init bitcoin
1$ cd bitcoin
2$ git remote add origin https://github.com/bitcoin/bitcoin
3$ git fetch --depth=8 origin a241d6069cf0542acdd8ec6be63724da19f10720
4$ git checkout a241d6069cf0542acdd8ec6be63724da19f10720
    • the following step:
    0$ git checkout HEAD~

    git log --merges -1 --format='%H' returns 1e9d367d0d39f9252bf8f738c216c0fe4c64a898, which is correct.

    However, git log ^1e9d367d0d39f9252bf8f738c216c0fe4c64a898 HEAD --format='%H' produces an empty line, because

    0$ git rev-parse HEAD
11e9d367d0d39f9252bf8f738c216c0fe4c64a898

    and git log is effectively pointed at the empty set of commits.

    Fixed by removing the previous git checkout HEAD~ command.

  8. maflcko commented at 8:10 am on September 19, 2023: member

    Fixed by removing the previous git checkout HEAD~ command.

    Seems wasteful, because the HEAD commit is already checked by all other CI tasks, so I’d say it would make sense to skip it.

  9. hebasto commented at 8:14 am on September 19, 2023: member

    Fixed by removing the previous git checkout HEAD~ command.

    Seems wasteful, because the HEAD commit is already checked by all other CI tasks, so I’d say it would make sense to skip it.

    0if: github.event.pull_request.commits != 1

    ?

  10. maflcko commented at 8:17 am on September 19, 2023: member
    The general idea is that if a pull request has N commits, this check is run min(6, N-1) times.
  11. hebasto commented at 8:19 am on September 19, 2023: member

    The general idea is that if a pull request has N commits, this check is run max(6, N-1) times.

    min(6, N-1) ?

  12. hebasto commented at 8:21 am on September 19, 2023: member

    The general idea is that if a pull request has N commits, this check is run min(6, N-1) times.

    Then it holds, that the check might be skipped if a pull request has the only commit, right?

  13. hebasto commented at 8:23 am on September 19, 2023: member

    The general idea is that if a pull request has N commits, this check is run min(6, N-1) times.

    Then it holds, that the check might be skipped if a pull request has the only commit, right?

    OK, I’ll update the code.

  14. maflcko commented at 8:23 am on September 19, 2023: member
    Oh, I see. You are suggesting to add the check if: github.event.pull_request.commits != 1 and re-add the git checkout HEAD~? Then yes, this should work.
  15. ci: Reintroduce fixed "test-each-commit" job 
    The new job checks at most 6 commits of a pull request, excluding the
top one.
    27b636a921
  16. hebasto force-pushed on Sep 19, 2023
  17. hebasto commented at 8:40 am on September 19, 2023: member

    Updated 57ee0b5bf12f02b1fb18adcbf14fb13166c76011 -> 27b636a92199d2d47db5e6049de3c924d1f634f9 (pr28497.01 -> pr28497.02, diff):

  18. maflcko commented at 8:49 am on September 19, 2023: member
    lgtm ACK 27b636a92199d2d47db5e6049de3c924d1f634f9
  19. maflcko commented at 10:32 am on September 19, 2023: member
    Windows CI failure seems unrelated
  20. maflcko commented at 4:04 pm on September 19, 2023: member
    rfm?
  21. fanquake merged this on Sep 19, 2023
  22. fanquake closed this on Sep 19, 2023
  23. hebasto deleted the branch on Sep 19, 2023
  24. Frank-GER referenced this in commit 3c1de58e24 on Sep 25, 2023
  25. sidhujag referenced this in commit 0592813fca on Sep 26, 2023
  26. ryanofsky commented at 5:13 pm on October 2, 2023: contributor

    This job is failing for me on #25722 (https://github.com/bitcoin/bitcoin/actions/runs/6303163392/job/17147055820?pr=25722) with 1b2a5f12b42543d13a4bcafb9585e3d1df488914.

    The problem is that the PR only contains a one commit after the last merge, so the job should be skipped, but the pull_request.commits != 1 check is not smart enough to skip it because the PR is based on another PR and has merge ancestor commits. So specifically what happens is that after HEAD~ is checked out, the new HEAD 91328ffed79ad2c8866631fbf9fe37643fb1c31d is a merge commit, so the range $(git log --merges -1 --format=%H)..HEAD is equivalent to HEAD..HEAD, which is empty, so the COMMIT_AFTER_LAST_MERGE variable is empty and the rebase command fails.

    I think a nice way to fix this would be instead of figuring out the commit after the last merge and rebasing on that with a ~1 suffix, just figure out the last merge commit directly and rebase on it. This way, if HEAD happens to be a merge commit, the rebase just succeeds immediately without blank variables or errors. I tested the following command line and it seem to work well in all cases:

    0git rebase --exec "echo Running test-one-commit on \$( git log -1 ); echo" $(git rev-list -n7 --reverse HEAD ^$(git rev-list -n1 --merges HEAD)^@ | head -1)

    Test cases:

    • When checking out d898e962d7f7a3a084158530ae0ec135212cd872 which contains more than 6 non-merge commits it executes on the last 6 commits.
    • When checking out 1b2a5f12b42543d13a4bcafb9585e3d1df488914 which contains 1 non-merge commit, it executes only just that commit.
    • When checking out 91328ffed79ad2c8866631fbf9fe37643fb1c31d which is a merge commit, it succeeds right away and executes nothing.

    To decipher the $(git rev-list -n7 --reverse HEAD ^$(git rev-list -n1 --merges HEAD)^@ | head -1) expression:

    • The git rev-list -n7 --reverse HEAD parts lists HEAD and up to 6 of its most recent ancestor commits in chronological order.
    • The extra ^$(git rev-list -n1 --merges HEAD)^@ argument is just there to exclude ancestors of merge commits:
      • $(git rev-list -n1 --merges HEAD) finds the most recent merge commit
      • The ^@ suffix after it expands that merge commit into a list of the parent commits it merges
      • The ^ prefix before it excludes all these merge parents and their ancestors

    I’d be happy to make or review a PR with this fix if it seems right

  27. maflcko commented at 8:26 am on October 3, 2023: member

    I’d be happy to make or review a PR with this fix if it seems right

    No objection if it makes the code clearer or is an improvement otherwise.

    However, I don’t think merge commits are supported in mergeable pull requests, unless it is a subtree bump. So unless the bug also affects subtree bumps, I don’t think we need to spend time fixing it.

  28. ryanofsky referenced this in commit 3e578cec91 on Oct 3, 2023
  29. ryanofsky referenced this in commit 8fb638cf92 on Oct 3, 2023
  30. ryanofsky referenced this in commit b5da8a4111 on Oct 3, 2023
  31. ryanofsky referenced this in commit 3447796bf5 on Oct 3, 2023
  32. ryanofsky commented at 7:39 pm on October 3, 2023: contributor

    No objection if it makes the code clearer or is an improvement otherwise.

    Thanks, implemented the change in #28573

  33. ryanofsky referenced this in commit a327191399 on Oct 4, 2023
  34. ryanofsky referenced this in commit 88c8e3a0e4 on Oct 5, 2023
  35. fanquake referenced this in commit 4e7442e743 on Oct 9, 2023
  36. Frank-GER referenced this in commit 10177a0093 on Oct 13, 2023
  37. janus referenced this in commit 81709dca12 on Apr 1, 2024


hebasto DrahtBot maflcko ryanofsky Sjors

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-09-29 01:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me