Skip to content
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.

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

Make test-fixtures-windows required for PR auto-merge #1793

Merged
merged 1 commit into from
Jan 23, 2025

Conversation

EliahKagan
Copy link
Member

@EliahKagan EliahKagan commented Jan 22, 2025

Since #1663, the test-fixtures-windows CI job checks actual failures against a list of specific tests that are known to fail on Windows when GIX_TEST_IGNORE_ARCHIVES=1. It is therefore capable of providing useful information about new failures, or newly passing tests that should be removed from the list, if the job ever does fail.

The job also seems not to fail. This is to say that while #1358 is not fixed, the test-fixtures-windows job has a very low rate of failure and, if it does fail, something new and interesting would be happening such that we would want to know about it and probably not immediately merge a PR that caused it without checking how and why that happened.

This PR adds test-fixtures-windows to the list of jobs that are dependencies of a required check for branch protection based PR auto-merge.

I know of two possible reasons not to do this now, which should be considered:

  • Maybe we don't want PR-blocking job to be able to fail due to an expected failure not failing anymore (though if that happens we would want to take note of it).
  • This is one of the longer-running jobs, taking approximately as long as the other Windows test job. Usually they will run in parallel on separate runners, but on occasion when many checks are running on Windows runners in the repository or organization, they might effectively run in series. Maybe we don't want such a long-running job to be a PR-blocking job.

Overall it seems to me that it makes sense to do this (as argued above), but I did want to acknowledge those two counterpoints.

@EliahKagan EliahKagan marked this pull request as ready for review January 22, 2025 10:48
@Byron
Copy link
Member

Byron commented Jan 23, 2025

Alright, thank you, let's try it.
I have fixed the merge conflict in the Web editor and am not 100% confident I did it correctly. If you give your OK we can merge this PR.

@EliahKagan
Copy link
Member Author

EliahKagan commented Jan 23, 2025

I think the merge commit may be okay, but that the situation might be less confusing with a rebase. I can do that, but please let me know if you don't want a rebase here.

Edit: I've done the rebase.

Since GitoxideLabs#1663, the `test-fixtures-windows` CI job checks actual
failures against a list of specific tests that are known to fail on
Windows when `GIX_TEST_IGNORE_ARCHIVES=1`. It is therefore capable
of providing useful information about new failures, or newly
passing tests that should be removed from the list, if the job ever
does fail.

The job also seems not to fail. This is to say that while GitoxideLabs#1358 is
not fixed, the `test-fixtures-windows` job has a very low rate of
failure and, if it does fail, something new and interesting would
be happening such that we would want to know about it and probably
not immediately merge a PR that caused it without checking how and
why that happened.

This adds `test-fixtures-windows` to the list of jobs that are
dependencies of a required check for branch protection based PR
auto-merge.
@EliahKagan EliahKagan force-pushed the test-fixtures-windows-required branch from 8275c27 to e97c4f1 Compare January 23, 2025 07:42
@Byron Byron enabled auto-merge January 23, 2025 07:44
@Byron Byron merged commit 7c803fa into GitoxideLabs:main Jan 23, 2025
21 checks passed
@EliahKagan EliahKagan deleted the test-fixtures-windows-required branch January 23, 2025 07:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants