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

Block merge of PRs with failing tests #4730

Closed
pete-watters opened this issue Dec 19, 2023 · 6 comments
Closed

Block merge of PRs with failing tests #4730

pete-watters opened this issue Dec 19, 2023 · 6 comments
Labels
bug Functionality broken bug-p2 Critical functionality broken for few users, with no clear workarounds for:devops needs:refactoring Tech debt, developer experience, testing, etc. needs:tests

Comments

@pete-watters
Copy link
Contributor

We are able to click Merge when ready on PRs that have failing tests which can introduce problems like:

I'm not sure if we can stop this happening but we should try

@pete-watters pete-watters added needs:tests needs:refactoring Tech debt, developer experience, testing, etc. for:devops labels Dec 19, 2023
@kyranjamie
Copy link
Collaborator

kyranjamie commented Dec 19, 2023

I'm confused by Github's UI

image

I want all statuses to pas, yet it gives me the option to specify individual ones?

Gosh, it says here that yeah, you have to select them individually....

@pete-watters
Copy link
Contributor Author

Gosh, it says here that yeah, you have to select them individually....

Yeah its a bit confusing. I think we may also need to add dev as a protected branch but I'm not sure if that would introduce other headaches?

@kyranjamie
Copy link
Collaborator

It already is, these are the protected branch rules. Added them manually for now, does this change your merge UI?
image

@pete-watters
Copy link
Contributor Author

pete-watters commented Dec 19, 2023

It has added these checks and is now blocking merge if checks aren't passing. Thanks for the quick action on this 👍

Checks show as required:
Screenshot 2023-12-19 at 10 17 24

You can click Merge when ready but its not green anymore
Screenshot 2023-12-19 at 10 18 52

When you click it, if the tests aren't passing then you won't be able to merge
Screenshot 2023-12-19 at 10 19 07

@pete-watters
Copy link
Contributor Author

We might need to do some more work for external contributor PRs as some of the now required checks don't pass. You can see on this PR: #4751

The test's can't pass on their PR as they don't have the required key but it would be handy if lint / prettier / typecheck can run.

We should probably change the test code back so the key is available to them. That way we could run the full suite of checks properly on their PR.

I'm happy to help do this, just wanted to add a note here

@pete-watters pete-watters reopened this Dec 22, 2023
@markmhendrickson markmhendrickson added the bug Functionality broken label Jan 2, 2024
@markmhendrickson markmhendrickson added this to the Fix urgent bugs milestone Jan 2, 2024
@markmhendrickson markmhendrickson added the bug-p2 Critical functionality broken for few users, with no clear workarounds label Jan 2, 2024
@pete-watters
Copy link
Contributor Author

Kyran helped me understand the process for merging external PRs better so we don't need to do this. They won't be able to run all CI actions for security reasons but we have made a good improvement here blocking PRs with failing tests merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Functionality broken bug-p2 Critical functionality broken for few users, with no clear workarounds for:devops needs:refactoring Tech debt, developer experience, testing, etc. needs:tests
Projects
None yet
Development

No branches or pull requests

3 participants