Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

refactor: Simplify temp file creation in tests #3290

Merged
merged 1 commit into from
Oct 1, 2024

Conversation

alexandear
Copy link
Contributor

The PR simplifies the openTestFile function logic by leveraging T.TempDir.

Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Thank you, @alexandear !
Now that you are passing t here, what do you think about making this a t.Helper?

github/github_test.go Show resolved Hide resolved
Copy link

codecov bot commented Sep 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.97%. Comparing base (2b8c7fa) to head (b5b123e).
Report is 126 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3290      +/-   ##
==========================================
- Coverage   97.72%   92.97%   -4.75%     
==========================================
  Files         153      171      +18     
  Lines       13390    11715    -1675     
==========================================
- Hits        13085    10892    -2193     
- Misses        215      729     +514     
- Partials       90       94       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@alexandear alexandear force-pushed the refactor/simplify-test branch from d4fa389 to b5b123e Compare September 24, 2024 16:02
@alexandear alexandear requested a review from gmlewis September 24, 2024 16:03
Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Thank you, @alexandear .
LGTM. (In the future, please don't force-push in this repo as we always squash-and-merge anyway... it makes reviewing changes easier since the last review. Thanks.)

Awaiting second LGTM+Approval from any other contributor to this repo before merging.

@gmlewis gmlewis added the NeedsReview PR is awaiting a review before merging. label Sep 25, 2024
Copy link
Contributor

@tomfeigin tomfeigin left a comment

Choose a reason for hiding this comment

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

LGTM

@gmlewis gmlewis removed the NeedsReview PR is awaiting a review before merging. label Oct 1, 2024
@gmlewis
Copy link
Collaborator

gmlewis commented Oct 1, 2024

Thanks you, @tomfeigin !
Merging.

@gmlewis gmlewis merged commit 8faa07c into google:master Oct 1, 2024
6 of 7 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants