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

status: modify TestStatus_ErrorDetails_Fail to replace protoimpl package #6953

Merged
merged 5 commits into from
Feb 1, 2024

Conversation

arvindbr8
Copy link
Member

@arvindbr8 arvindbr8 commented Jan 31, 2024

The protoimpl package is being currently used in a test (which got introduced in #6919). But this package cannot be accessed directly in google3, hence we need a different way of testing the case.

RELEASE NOTES: none

Copy link

codecov bot commented Jan 31, 2024

Codecov Report

Merging #6953 (81957e0) into master (8d735f0) will decrease coverage by 0.02%.
Report is 3 commits behind head on master.
The diff coverage is 66.66%.

❗ Current head 81957e0 differs from pull request most recent head 5ef777c. Consider uploading reports for the commit 5ef777c to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6953      +/-   ##
==========================================
- Coverage   83.71%   83.69%   -0.02%     
==========================================
  Files         287      287              
  Lines       30926    30929       +3     
==========================================
- Hits        25889    25886       -3     
- Misses       3972     3979       +7     
+ Partials     1065     1064       -1     
Files Coverage Δ
balancer/grpclb/grpclb.go 80.47% <ø> (ø)
balancer/grpclb/grpclb_remote_balancer.go 83.69% <ø> (ø)
balancer/rls/config.go 83.22% <100.00%> (ø)
encoding/proto/proto.go 62.50% <ø> (ø)
internal/binarylog/method_logger.go 87.31% <100.00%> (ø)
internal/binarylog/sink.go 11.42% <ø> (ø)
internal/pretty/pretty.go 57.89% <100.00%> (+15.78%) ⬆️
internal/status/status.go 90.52% <100.00%> (ø)
internal/transport/handler_server.go 85.91% <ø> (ø)
internal/transport/http2_server.go 89.84% <ø> (+0.28%) ⬆️
... and 14 more

... and 12 files with indirect coverage changes

got := tc.s.Details()
if !cmp.Equal(got, tc.i, cmp.Comparer(proto.Equal), cmp.Comparer(equalError)) {
t.Errorf("(%v).Details() = %+v, want %+v", str(tc.s), got, tc.i)
details := tc.s.Details()
Copy link
Member

Choose a reason for hiding this comment

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

You should first ensure len(details) == len(tc.want) here or it could panic.

Copy link
Member Author

Choose a reason for hiding this comment

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

that's right! thanks for catching that

t.Errorf("(%v).Details() = %+v, want %+v", str(tc.s), got, tc.i)
details := tc.s.Details()
for i, d := range details {
// s.Deatils can either contain an error or a proto message. We
Copy link
Member

Choose a reason for hiding this comment

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

Spelling

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@arvindbr8 arvindbr8 added this to the 1.62 Release milestone Jan 31, 2024
@arvindbr8 arvindbr8 added the Type: Dependencies Updating/adding/removing dependencies label Jan 31, 2024
@arvindbr8 arvindbr8 merged commit 8da3e23 into grpc:master Feb 1, 2024
12 checks passed
@arvindbr8 arvindbr8 deleted the fix_import branch February 1, 2024 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Dependencies Updating/adding/removing dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants