Skip to content

Conversation

@easwars
Copy link
Contributor

@easwars easwars commented Aug 19, 2024

RELEASE NOTES: none

@easwars easwars requested a review from arvindbr8 August 19, 2024 20:40
@easwars easwars added the Type: Internal Cleanup Refactors, etc label Aug 19, 2024
@easwars easwars added this to the 1.67 Release milestone Aug 19, 2024
@codecov
Copy link

codecov bot commented Aug 19, 2024

Codecov Report

Attention: Patch coverage is 30.76923% with 9 lines in your changes missing coverage. Please review.

Project coverage is 81.86%. Comparing base (86135c3) to head (dbf5688).
Report is 9 commits behind head on master.

Files Patch % Lines
internal/xds/bootstrap/bootstrap.go 18.18% 8 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7537      +/-   ##
==========================================
+ Coverage   81.82%   81.86%   +0.04%     
==========================================
  Files         360      360              
  Lines       27533    27508      -25     
==========================================
- Hits        22528    22520       -8     
+ Misses       3804     3793      -11     
+ Partials     1201     1195       -6     
Files Coverage Δ
internal/testutils/xds/e2e/bootstrap.go 64.00% <100.00%> (ø)
internal/xds/bootstrap/bootstrap.go 70.47% <18.18%> (-1.88%) ⬇️

... and 28 files with indirect coverage changes

Copy link
Contributor

@arvindbr8 arvindbr8 left a comment

Choose a reason for hiding this comment

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

Overall LGTM, unless you think the stringer method might benefit from unit tests.

Also on a side note: Should we modify NewContentsForTesting to Unmarshal to ServerConfigs directly?

@arvindbr8 arvindbr8 assigned easwars and unassigned arvindbr8 Aug 19, 2024
@easwars
Copy link
Contributor Author

easwars commented Aug 20, 2024

Also on a side note: Should we modify NewContentsForTesting to Unmarshal to ServerConfigs directly?

That's a good idea. Did that.

@easwars
Copy link
Contributor Author

easwars commented Aug 20, 2024

Overall LGTM, unless you think the stringer method might benefit from unit tests.

Thought about it. It will mostly be used only for logging and/or debugging. So. I'm not too worried about not having unit tests for that. In fact, we don't want anything to rely on this output being stable.

@easwars easwars merged commit 7e12068 into grpc:master Aug 20, 2024
1 check passed
@easwars easwars deleted the xdsclient_connectivity_state branch August 20, 2024 21:08
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 17, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants