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 select label override #14

Merged
merged 11 commits into from
Mar 28, 2023
Merged

Fix select label override #14

merged 11 commits into from
Mar 28, 2023

Conversation

benCoomes
Copy link
Contributor

@benCoomes benCoomes commented Mar 27, 2023

Fixes bug introduced in #12 where label matching override checks for CI and reviews.

Also:

  • checks labels first so that if labels don't match, API calls for PR status can be skipped.
  • Creates test helper functions to reduce repeated object creation

@benCoomes benCoomes marked this pull request as ready for review March 27, 2023 20:09
@benCoomes
Copy link
Contributor Author

I was scratching my head for a while because I couldn't figure out why tests were failing after reordering the CI/label checks - turns out it was because when the CI checks were skipped for one PR, the graphql mocks got out of order.

Is there a way to make mocks return a value based on a parameter, rather than setting them up to be returned values in a sequence?

src/main.js Outdated Show resolved Hide resolved
src/main.js Outdated Show resolved Hide resolved
@GrantBirki
Copy link
Member

I was scratching my head for a while because I couldn't figure out why tests were failing after reordering the CI/label checks - turns out it was because when the CI checks were skipped for one PR, the graphql mocks got out of order.

Is there a way to make mocks return a value based on a parameter, rather than setting them up to be returned values in a sequence?

This is probably a side effect of two things: 1. Me using jest 2. Me not really understanding jest. I wired up tests in this repository so that we would have them, but they really are not setup in the best or the most maintainable manner. I'm all for any improvements you may have!

PS: I left some comments to use async / await in the new function you created just to be on the safe side and ensure that we are always awaiting for a value to return out of that function

@GrantBirki GrantBirki added bug Something isn't working enhancement New feature or request labels Mar 28, 2023
@GrantBirki
Copy link
Member

@benCoomes When this PR merges, will it fully resolve #11?

@benCoomes
Copy link
Contributor Author

This is probably a side effect of two things: 1. Me using jest 2. Me not really understanding jest.

Ha, we are in the same boat then! I'll spend some time to see what I can do.

When this PR merges, will it fully resolve #11?

It will give a workaround for it, so I'd be okay to close the issue. Dependabot needs to be configured per branch anyways, so setting a branch-specific label and then having a combine-pr config to select on that branch label is not hard to set up.

It'd still be nice to have it separate PRs by target branch by default, but I think the config would need to change to support either:

  • creation of multiple PRs & branches in a single run
  • or, limit each run to a single target branch

@benCoomes
Copy link
Contributor Author

@GrantBirki, I think this is ready for another look!

}),
graphql: jest.fn().mockImplementation((_query, params) => {
switch (params.pull_number) {
case 1:
Copy link
Member

Choose a reason for hiding this comment

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

I love this! TIL

Copy link
Member

@GrantBirki GrantBirki left a comment

Choose a reason for hiding this comment

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

Amazing! 🎉 Thank you @benCoomes, you have significantly improved the test suite here as well and I learned a few new things following through your commits as well 🙇

@GrantBirki GrantBirki merged commit 558e62d into main Mar 28, 2023
@GrantBirki GrantBirki deleted the fix-select-label-override branch March 28, 2023 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants