Skip to content

ci: Only run functional tests on native windows in master #28567

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
fanquake merged 1 commit into from
Oct 4, 2023
Merged

ci: Only run functional tests on native windows in master #28567

fanquake merged 1 commit into from
Oct 4, 2023

Conversation

@fjahr
Copy link
Contributor

@fjahr fjahr commented Oct 3, 2023

This idea was discussed here.

@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 3, 2023 •
edited
Loading

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 hebasto
Concept ACK MarcoFalke, fanquake

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

Conflicts

No conflicts as of last run.

@DrahtBot DrahtBot added the Tests label Oct 3, 2023
@fjahr fjahr changed the title ci: Only run functional tests on windows in master ci: Only run functional tests on native windows in master Oct 3, 2023
@maflcko
Copy link
Member

maflcko commented Oct 3, 2023

Concept ACK.

Doesn't seem ideal, but I can't see a better alternative right now.

@maflcko maflcko requested a review from hebasto October 3, 2023 09:51
hebasto
hebasto approved these changes Oct 3, 2023
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK 9eca34224021d5adc80f09555b2e15a81ccd6c76, the diff is correct.

Suggesting to add a comment that explains the reasons of skipping functional tests in pull requests.

@fjahr
Copy link
Contributor Author

fjahr commented Oct 3, 2023

I added a comment as suggested by @hebasto including TODO since that seemed most appropriate.

maflcko
maflcko reviewed Oct 3, 2023
View reviewed changes
@DrahtBot DrahtBot mentioned this pull request Oct 3, 2023
Merged
@fanquake fanquake requested a review from hebasto October 4, 2023 08:55
hebasto
hebasto reviewed Oct 4, 2023
View reviewed changes
@fanquake
Copy link
Member

fanquake commented Oct 4, 2023

Concept ACK. Lets get this merged, because this is just continuing to confuse and annoy contributors.

@fjahr
Copy link
Contributor Author

fjahr commented Oct 4, 2023

Reverted the if and fixed the comment.

hebasto
hebasto approved these changes Oct 4, 2023
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK aba4a58

@fanquake fanquake merged commit 3cd0280 into bitcoin:master Oct 4, 2023
@ismaelsadeeq ismaelsadeeq mentioned this pull request Oct 5, 2023
Merged
Frank-GER pushed a commit to syscoin/syscoin that referenced this pull request Oct 13, 2023
@fanquake
Merge bitcoin#28567: ci: Only run functional tests on native windows …
07f975f 
…in master

aba4a58 ci: Only run functional tests on windows in master (Fabian Jahr)

Pull request description:

  This idea was discussed [here](bitcoin#28509 (comment)).

ACKs for top commit:
  hebasto:
    ACK aba4a58

Tree-SHA512: 89fd6352b585bae3538d5350b0404c216a8225fe356d408c1ebe3394e7b9a190d65639f4eef310056e020909928d7a1f2de25585c97d2ac087d1a9f72af281eb
@amitiuttarwar amitiuttarwar mentioned this pull request Oct 18, 2023
Merged
@hebasto hebasto mentioned this pull request Dec 12, 2023
Merged
fanquake added a commit that referenced this pull request Dec 12, 2023
@fanquake
Merge #29059: Revert "ci: Only run functional tests on windows in mas…
a7484be 
…ter"

7b22cd8 Revert "ci: Only run functional tests on windows in master" (Hennadii Stepanov)

Pull request description:

  This PR reverts commit aba4a58 from #28567.

  The Windows-specific code received [quality](#28486) and [performance](#29045) improvements recently. So there are no reasons to skip functional tests in PRs anymore.

  In my own repo, I've run the GHA Windows job more than 100 times with no failure.

ACKs for top commit:
  maflcko:
    lgtm ACK 7b22cd8
  TheCharlatan:
    ACK 7b22cd8

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

Reviewers

@hebasto hebasto hebasto approved these changes

+1 more reviewer

@maflcko maflcko maflcko approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@fjahr @DrahtBot @maflcko @fanquake @hebasto