Skip to content

refactor: Do not call teardown manually in tests - #3293

Closed
alexandear wants to merge 2 commits into
google:masterfrom
alexandear-org:refactor/setup-t-helper
Closed

refactor: Do not call teardown manually in tests#3293
alexandear wants to merge 2 commits into
google:masterfrom
alexandear-org:refactor/setup-t-helper

Conversation

@alexandear

@alexandearalexandear commented Sep 24, 2024

Copy link
Copy Markdown
Contributor

The PR refactors the setup function by passing *testing.T parameter and calling t.Cleanup(server.Close) inside. So a test that using setup doesn't need to call manually teardown.

This leads to removal of 1319 code lines in tests.

@gmlewisgmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you, @alexandear !
One important change, please, then we should be ready for a second LGTM+Approval from any other contributor to this repo before merging.

Comment threadscrape/scrape_test.go
@codecov

codecovBot commented Sep 25, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@ Coverage Diff @@## master #3293 +/- ##
==========================================
- 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.

Co-authored-by: Glenn Lewis <6598971+gmlewis@users.noreply.github.com>

@gmlewisgmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you, @alexandear !
LGTM.

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

@gmlewisgmlewis added the NeedsReview PR is awaiting a review before merging. label Sep 25, 2024

@tomfeigintomfeigin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

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

Copy link
Copy Markdown
Collaborator

Thank you, @tomfeigin !
Merging.

Sign up for freeto 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.

3 participants

@alexandear@gmlewis@tomfeigin