Skip to content

Conversation

@hebasto
Copy link
Member

@hebasto hebasto commented Aug 28, 2023

This PR is intended to speed up the CI feedback for pull requests:

  • a PR opened against the current master branch:
    image

  • this PR:
    image

Suggested in #28173 (comment):

An alternative would be to run them on non-pr pushes only. Failures should be rare enough to deal with them post-merge.

@hebasto hebasto added the Tests label Aug 28, 2023
@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 28, 2023

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.

@hebasto hebasto marked this pull request as draft August 29, 2023 15:33
@hebasto hebasto marked this pull request as ready for review August 30, 2023 09:21
@hebasto
Copy link
Member Author

hebasto commented Aug 30, 2023

cc @MarcoFalke


- name: Run functional tests
env:
TEST_RUNNER_EXTRA: ${{ github.event_name != 'pull_request' && '--extended --exclude feature_dbcrash' || '' }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
TEST_RUNNER_EXTRA: ${{ github.event_name != 'pull_request' && '--extended --exclude feature_dbcrash' || '' }}
TEST_RUNNER_EXTRA: ${{ github.event_name != 'pull_request' && '--extended' || '' }}

If there is a reason to exclude this, it should be mentioned

Copy link
Member Author

@hebasto hebasto Aug 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we OK with an increased runtime:

image
?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

feature_dbcrash.py                                     | ✓ Passed  | 7086 s

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any downside?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there are any downsides, you can skip the functional tests on the master branch completely on job_id % n == 0, where n is the down-scaling factor, but then run them completely otherwise?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It takes one runner busy approximately twice longer than other runners. Of course, not a big deal, considering we are provided with 20 runners.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there are any downsides, you can skip the functional tests on the master branch completely on job_id % n == 0, where n is the down-scaling factor, but then run them completely otherwise?

Let's left it as is and see how it will go.

This change is intended to speed up the CI feedback for pull requests.
@maflcko
Copy link
Member

maflcko commented Aug 30, 2023

lgtm ACK 62ab3e9 if https://github.com/hebasto/bitcoin/actions/runs/6023862001/job/16341417883 is green

@maflcko
Copy link
Member

maflcko commented Aug 30, 2023

It is green

@hebasto
Copy link
Member Author

hebasto commented Aug 30, 2023

cc @fanquake

@fanquake fanquake merged commit 13e169a into bitcoin:master Aug 30, 2023
@hebasto hebasto deleted the 230828-gha-win branch August 30, 2023 15:16
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 26, 2023
…s for pull requests

62ab3e9 ci, windows: Do not run extended functional tests for pull requests (Hennadii Stepanov)

Pull request description:

  This PR is intended to speed up the CI feedback for pull requests:

  - a [PR](https://github.com/bitcoin/bitcoin/actions/runs/6019964104?pr=28196) opened against the current master branch:
  ![image](https://github.com/bitcoin/bitcoin/assets/32963518/481a70eb-13f3-40c9-8f6a-ca2f06350158)

  - this PR:
  ![image](https://github.com/bitcoin/bitcoin/assets/32963518/2582307f-7b72-4816-b5be-e84d5e4a3016)

  Suggested in bitcoin#28173 (comment):

  > An alternative would be to run them on non-pr pushes only. Failures should be rare enough to deal with them post-merge.

ACKs for top commit:
  MarcoFalke:
    lgtm ACK 62ab3e9 if https://github.com/hebasto/bitcoin/actions/runs/6023862001/job/16341417883 is green

Tree-SHA512: e937efc5c03290f5d246ce1b0638dc72f39ef1d509ba5d2063f92bfe9157c602e6a952a9558dfc6413bbc5209fe55280b3278a0e4079773b8cc9ff236c8f9246
@bitcoin bitcoin locked and limited conversation to collaborators Aug 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants