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

refactor: add try catch to reviewer #279

Merged
merged 9 commits into from
Apr 10, 2024

Conversation

alicelovescake
Copy link
Member

This PR:

  • Adds a try catch block so we can log the error and not stop the process if requestReviewer fails

@alicelovescake alicelovescake requested a review from a team as a code owner April 9, 2024 22:19
src/utils.ts Outdated Show resolved Hide resolved
Copy link
Member

@dsanders11 dsanders11 left a comment

Choose a reason for hiding this comment

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

I think the changes to tests should mostly be backed out and put in a new spec/utils.spec.ts which tests tagBackportReviewers directly. The changes to spec/operations.spec.ts get too into the implementation details of tagBackportReviewers, it was better when it was checking just if tagBackportReviewers was called, and we can put testing of error handling inside tagBackportReviewers in the spec/utils.spec.ts file.

@alicelovescake
Copy link
Member Author

I think the changes to tests should mostly be backed out and put in a new spec/utils.spec.ts which tests tagBackportReviewers directly. The changes to spec/operations.spec.ts get too into the implementation details of tagBackportReviewers, it was better when it was checking just if tagBackportReviewers was called, and we can put testing of error handling inside tagBackportReviewers in the spec/utils.spec.ts file.

Ah yes, agreed that for the updateManualBackport function, it was testing the implementation details of tagBackportReviewers more. Made a new test file with similar changes + a test checking that users is added to reviewers correctly

Copy link
Member

@dsanders11 dsanders11 left a comment

Choose a reason for hiding this comment

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

Thanks @alicelovescake!

@dsanders11 dsanders11 merged commit 3b3824a into electron:main Apr 10, 2024
3 checks passed
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