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

fix(test): force test failure #74

Merged
merged 2 commits into from
Aug 31, 2022
Merged

fix(test): force test failure #74

merged 2 commits into from
Aug 31, 2022

Conversation

oscard0m
Copy link
Member

@oscard0m oscard0m commented Aug 16, 2022

Description

Assure 'test' Check Run is always executed and fails/succeeds accordingly to the results of its dependency jobs.

  • Example commit with all tests passing ✅: c4a2c50
  • Example commit with tests failing 🔴: 8d93278

Context

When using job.needs, by default only runs when the dependency jobs succeed. If one or more of them fail, the dependent job is skipped

@oscard0m oscard0m added the Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR label Aug 16, 2022
@oscard0m oscard0m marked this pull request as ready for review August 16, 2022 21:52
@oscard0m oscard0m changed the title fix(test): force test failure [🛑 DO NOT MERGE] fix(test): force test failure Aug 16, 2022
@oscard0m oscard0m force-pushed the force-test-pipeline-failing branch 4 times, most recently from 10092bf to 14eef82 Compare August 16, 2022 22:27
@oscard0m
Copy link
Member Author

oscard0m commented Aug 16, 2022

@gr2m @wolfy1339 @timrogers

I think I found the issue. I'm surprised this did not appear before.

What do you think about my solution proposal? Do you agree with solving this in the context of octokit/.github#13?

@timrogers
Copy link

What is your proposed solution? I'm not sure I understand from this PR.

@wolfy1339
Copy link
Member

What is your proposed solution? I'm not sure I understand from this PR.

The solution proposed is to always run the test job that depends on the test_matrix jobs regardless of if they fail or not, in order to avoid having GitHub Actions skip it, and resulting in the skipped status.
That skipped status counts as if the tests passed for branch protection and allows us to merge PRs when they shouldn't.

In the test job, we check if the test_matrix jobs failed and then return an exit code of 1 in order to have it fail

@timrogers
Copy link

Sounds reasonable to me 🤷🏻

@wolfy1339
Copy link
Member

What do you think about my solution proposal? Do you agree with solving this in the context of octokit/.github#13?

This will work for now, until we sort out the reusable workflows

@oscard0m
Copy link
Member Author

This will work for now, until we sort out the reusable workflows

@wolfy1339
Do you mean you want me to apply this change to all our repositories until octokit/.github#13 is addressed?

@wolfy1339
Copy link
Member

This will work for now, until we sort out the reusable workflows

@wolfy1339 Do you mean you want me to apply this change to all our repositories until octokit/.github#13 is addressed?

Yes. That would be great

gr2m
gr2m previously approved these changes Aug 31, 2022
Copy link
Contributor

@gr2m gr2m left a comment

Choose a reason for hiding this comment

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

Change looks good, I think we did something similar in a different project.

wolfy1339
wolfy1339 previously approved these changes Aug 31, 2022
@gr2m
Copy link
Contributor

gr2m commented Aug 31, 2022

Here is the discussion we had in @semantic-release, we arrived at the same conclusion, but did not yet implement the change

semantic-release/semantic-release#2229

@oscard0m oscard0m dismissed stale reviews from wolfy1339 and gr2m via bfafa4f August 31, 2022 20:25
This was referenced Sep 1, 2022
wolfy1339 pushed a commit to octokit/create-octokit-project.js that referenced this pull request Sep 1, 2022
gr2m pushed a commit to octoherd/create-octoherd-script that referenced this pull request Sep 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants