Skip to content

fix(test): force test failure#74

Merged
oscard0m merged 2 commits intomainfrom
force-test-pipeline-failing
Aug 31, 2022
Merged

fix(test): force test failure#74
oscard0m merged 2 commits intomainfrom
force-test-pipeline-failing

Conversation

@oscard0m
Copy link
Copy Markdown
Member

@oscard0m oscard0m commented Aug 16, 2022 •
edited
Loading

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 oscard0m force-pushed the force-test-pipeline-failing branch from 14eef82 to c4a2c50 Compare August 16, 2022 22:28
@oscard0m
Copy link
Copy Markdown
Member Author

oscard0m commented Aug 16, 2022 •
edited
Loading

@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
Copy Markdown

timrogers commented Aug 18, 2022

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

@wolfy1339
Copy link
Copy Markdown
Member

wolfy1339 commented Aug 18, 2022

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
Copy Markdown

timrogers commented Aug 18, 2022

Sounds reasonable to me 🤷🏻

@wolfy1339
Copy link
Copy Markdown
Member

wolfy1339 commented Aug 18, 2022

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
Copy Markdown
Member Author

oscard0m commented Aug 19, 2022

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
Copy Markdown
Member

wolfy1339 commented Aug 19, 2022

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
View reviewed changes
Copy link
Copy Markdown
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
View reviewed changes
@gr2m
Copy link
Copy Markdown
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 added a commit to octoherd/create-octoherd-script that referenced this pull request Aug 31, 2022
@oscard0m
ci(test.yml): assure 'test' workflow fails if one of its dependencies…
778ce15 
… fail

context here: octokit/auth-oauth-device.js#74
@oscard0m oscard0m mentioned this pull request Aug 31, 2022
Merged
@oscard0m oscard0m dismissed stale reviews from wolfy1339 and gr2m via bfafa4f August 31, 2022 20:25
@oscard0m oscard0m force-pushed the force-test-pipeline-failing branch from 8d93278 to bfafa4f Compare August 31, 2022 20:25
@oscard0m oscard0m changed the title [🛑 DO NOT MERGE] fix(test): force test failure fix(test): force test failure Aug 31, 2022
This was referenced Sep 1, 2022
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
wolfy1339 pushed a commit to octokit/create-octokit-project.js that referenced this pull request Sep 1, 2022
@oscard0m
feat(test-action): add fix to create-test-action 'test' job (#189)
5e4d598 
Context: octokit/auth-oauth-device.js#74
gr2m pushed a commit to octoherd/create-octoherd-script that referenced this pull request Sep 6, 2022
@oscard0m
ci(test.yml): assure 'test' workflow fails if one of its dependencies…
94f9402 
… fail (#61)

context here: octokit/auth-oauth-device.js#74
@gr2m gr2m mentioned this pull request Sep 6, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wolfy1339 wolfy1339 wolfy1339 approved these changes

@timrogers timrogers Awaiting requested review from timrogers

@nickfloyd nickfloyd Awaiting requested review from nickfloyd

@gr2m gr2m Awaiting requested review from gr2m

Assignees

No one assigned

Labels

Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@oscard0m @timrogers @wolfy1339 @gr2m