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.

Sign up for GitHub

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

Jump to bottom

scripts: add linter rule for using context.WithTimeout on tests #7342

Merged
merged 38 commits into from
Jul 3, 2024
Merged

scripts: add linter rule for using context.WithTimeout on tests #7342

merged 38 commits into from
Jul 3, 2024

Conversation

hasson82
Copy link
Contributor

@hasson82 hasson82 commented Jun 22, 2024 •
edited
Loading

This PR is adding a linter rule for checking each context usage in tests is using context.WithTimeout.
Based on #7304.

The PR grew a lot, so decided to ignore some packages in the linter rule and will deal with them in upcoming PRs.

RELEASE NOTES: none

@hasson82
add context with timeout check to vet script
58ff83f
@hasson82
expand lint rule to avoid dealing with context.WithCancel cases
7da7920
@hasson82
add context.WithTimeout to handeshaker tests
d47051e
@hasson82
avoid context_test.go in linting
5acc6e3
@hasson82
Change the lint rule to avoid context_test.go file
c47300f
@hasson82
add context with timeout to observability tests
d595842
@hasson82
ignore credentials/google package as this package doesnt have default…
a1e7035 
… timeout defined
@hasson82
Change all context in bundle_ext_test.go to context with timeout
4a7ec24
@hasson82
Change all context in bundle_test.go to context with timeout
912de44
@hasson82
Change rbac_engine_test context to context with timeout
150515f
@hasson82
add context with timeout to method_logger_test in binary log
9cb967d
@hasson82
adding internal/transport package to avoid list in linter rule
53cc270
@hasson82
reduce rule in linter to avoid catching cases were context.WithTimeou…
befaa3d 
…t is coming before metadata
@hasson82
change all context usage in metadata_test to context.WithTimeout
eaccdda
@hasson82
add xds/internal to avoid list of linter rule
a4e7742
@hasson82
add context.WithTimeout to peer_test
aa71ff0
@hasson82
change all context usage in picker_wrapper_test to context.WithTimeout
d1aefe7
@hasson82
add security/advancedtls to avoid list in linter rule
40a54fb
@hasson82
change all usage of context in server_test.go to context.WithTimeout
96345b1
@hasson82
change missing context usage in observability_test to context.WithTim…
59a3855 
…eout
@hasson82
change all context usage to context.WithTimeout in gracefulstop_test
cfc815b
@hasson82
remove added lint from vet.sh
c33cbef
@hasson82
add lint rule of all context should have timeouts back
e7192b1
@hasson82
change lint rule in all avoided packages to not grep
689ae5b
@hasson82
change lint rule to not to enforce that the grep returns nothing
6a51d0d
@hasson82
change lint rule to not to enforce that the grep returns nothing
c29c45c
@hasson82
Merge branch 'hasson82/add-context-with-timeout-checks-to-lint' of gi…
7f744cd 
…thub.com:hasson82/grpc-go into hasson82/add-context-with-timeout-checks-to-lint
@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented Jun 22, 2024 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@codecov Codecov
Copy link

codecov bot commented Jun 22, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.47%. Comparing base (4dd7f55) to head (136b4ba).
Report is 23 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #7342      +/-   ##
==========================================
+ Coverage   80.58%   81.47%   +0.89%     
==========================================
  Files         349      348       -1     
  Lines       34056    26744    -7312     
==========================================
- Hits        27445    21791    -5654     
+ Misses       5431     3770    -1661     
- Partials     1180     1183       +3

see 307 files with indirect coverage changes

@aranjans aranjans added this to the 1.66 Release milestone Jun 24, 2024
@atollena atollena removed their assignment Jun 28, 2024
@hasson82 hasson82 requested a review from atollena June 28, 2024 20:48
@atollena atollena assigned atollena and unassigned hasson82 Jul 1, 2024
@dfawley dfawley assigned dfawley and unassigned atollena Jul 2, 2024
dfawley
dfawley reviewed Jul 3, 2024
View reviewed changes
Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

Thanks for doing this. It looks great generally, but I have a nit about creating contexts with timeout only once per test in several of the tests changed.

internal/binarylog/method_logger_test.go Outdated Show resolved Hide resolved
internal/xds/rbac/rbac_engine_test.go Outdated Show resolved Hide resolved
peer/peer_test.go Outdated Show resolved Hide resolved
picker_wrapper_test.go Outdated Show resolved Hide resolved
@dfawley dfawley assigned hasson82 and unassigned dfawley Jul 3, 2024
dfawley
dfawley reviewed Jul 3, 2024
View reviewed changes
server_test.go Outdated Show resolved Hide resolved
@hasson82 hasson82 requested a review from dfawley July 3, 2024 18:52
dfawley
dfawley approved these changes Jul 3, 2024
View reviewed changes
Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

That looks good, thank you.

@zasweq zasweq changed the title Linter rule for using context.WithTimeout on tests Add linter rule for using context.WithTimeout on tests Jul 3, 2024
@zasweq zasweq changed the title Add linter rule for using context.WithTimeout on tests scripts: add linter rule for using context.WithTimeout on tests Jul 3, 2024
@zasweq zasweq merged commit bdd707e into grpc:master Jul 3, 2024
13 checks passed
printchard pushed a commit to printchard/grpc-go that referenced this pull request Jul 8, 2024
@hasson82 @printchard
scripts: add linter rule for using context.WithTimeout on tests (grpc…
608e03b 
…#7342)
printchard pushed a commit to printchard/grpc-go that referenced this pull request Jul 8, 2024
@hasson82 @printchard
scripts: add linter rule for using context.WithTimeout on tests (grpc…
46a6225 
…#7342)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zasweq zasweq zasweq approved these changes

@dfawley dfawley dfawley approved these changes

@atollena atollena atollena approved these changes

Assignees

@zasweq zasweq

Labels
Type: Internal Cleanup Refactors, etc
Projects
None yet
Milestone
1.66 Release
Development

Successfully merging this pull request may close these issues.
5 participants
@hasson82 @atollena @dfawley @zasweq @aranjans