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

xds: add unit tests for HandshakeInfo.Equal #7638

Merged
merged 7 commits into from
Sep 26, 2024
Merged

xds: add unit tests for HandshakeInfo.Equal #7638

merged 7 commits into from
Sep 26, 2024

Conversation

janardhanvissa
Copy link
Contributor

@janardhanvissa janardhanvissa commented Sep 17, 2024 •
edited
Loading

Addresses: #6568

RELEASE NOTES: None

@codecov Codecov
Copy link

codecov bot commented Sep 17, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.81%. Comparing base (11c44fb) to head (3a63bdb).
Report is 10 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #7638      +/-   ##
==========================================
- Coverage   81.93%   81.81%   -0.12%     
==========================================
  Files         361      361              
  Lines       27816    27821       +5     
==========================================
- Hits        22790    22761      -29     
- Misses       3837     3860      +23     
- Partials     1189     1200      +11

see 26 files with indirect coverage changes

@purnesh42H purnesh42H self-assigned this Sep 17, 2024
@purnesh42H purnesh42H changed the title Handshakeinfo equal xds/test: add tests for handshakeinfo equal Sep 17, 2024
@purnesh42H
Copy link
Contributor

@janardhankrishna-sai please add rel notes and as mentioned before please follow convention : for PR titles

@janardhanvissa
Copy link
Contributor Author

@purnesh42H Added relevant notes as before.

purnesh42H
purnesh42H approved these changes Sep 17, 2024
View reviewed changes
Copy link
Contributor

@purnesh42H purnesh42H left a comment

Choose a reason for hiding this comment

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

lgtm

@purnesh42H purnesh42H requested a review from easwars September 17, 2024 15:08
@purnesh42H
Copy link
Contributor

@easwars for second review

@purnesh42H purnesh42H added the Area: Testing Includes tests and testing utilities that we have for unit and e2e tests within our repo. label Sep 17, 2024
@purnesh42H purnesh42H added this to the 1.68 Release milestone Sep 17, 2024
@purnesh42H purnesh42H changed the title xds/test: add tests for handshakeinfo equal internal/credentials/xds: add unit test for HandshakeInfo.Equal Sep 17, 2024
@easwars easwars changed the title internal/credentials/xds: add unit test for HandshakeInfo.Equal xds: add unit tests for HandshakeInfo.Equal Sep 24, 2024
easwars
easwars reviewed Sep 24, 2024
View reviewed changes
@easwars easwars assigned janardhanvissa and unassigned easwars Sep 24, 2024
@janardhanvissa
Copy link
Contributor Author

@easwars Addressed the above mentioned comments and pushed the changes, please let me know if any changes need to be made.

@purnesh42H purnesh42H changed the title xds: add unit tests for HandshakeInfo.Equal internal/credentials/xds: add unit tests for HandshakeInfo.Equal Sep 24, 2024
@purnesh42H purnesh42H requested a review from easwars September 24, 2024 16:18
@purnesh42H purnesh42H assigned easwars and unassigned janardhanvissa Sep 24, 2024
easwars
easwars reviewed Sep 25, 2024
View reviewed changes
internal/credentials/xds/handshake_info_test.go
wantMatch: true,
},
{
desc: "same providers, different SAN matchers",
Copy link
Contributor

Choose a reason for hiding this comment

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

This test case seems to be subset of the next one. So, we can remove this test case.

@easwars easwars merged commit 9affdbb into grpc:master Sep 26, 2024
14 checks passed
@easwars easwars changed the title internal/credentials/xds: add unit tests for HandshakeInfo.Equal xds: add unit tests for HandshakeInfo.Equal Sep 26, 2024
@easwars easwars mentioned this pull request Sep 26, 2024
Closed
@purnesh42H
Copy link
Contributor

@easwars Addressed the above mentioned comments and pushed the changes, please let me know if any changes need to be made.

@janardhanvissa for future, please respond to each of reviewer's comment.

  • For accepted suggestions: Reply "Done" directly within the comment.
  • For explanations without code changes: Respond with an explanation directly within the comment.
  • For alternate solutions: Describe your changes directly within the comment.

This keeps the conversation organized and makes it easier for reviewers to track changes.

@janardhanvissa
Copy link
Contributor Author

@purnesh42H Ok, understood. I'll follow this approach in future responses. Thank you!

janardhanvissa added a commit to janardhanvissa/grpc-go that referenced this pull request Oct 1, 2024
@janardhanvissa @janardhankrishna-sai
internal/credentials/xds: add unit tests for HandshakeInfo.Equal (g…
8bc9074 
…rpc#7638)
@tbg tbg mentioned this pull request Nov 27, 2024
Closed
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@easwars easwars easwars approved these changes

@purnesh42H purnesh42H purnesh42H approved these changes

Assignees

@easwars easwars

Labels
Area: Testing Includes tests and testing utilities that we have for unit and e2e tests within our repo. Status: Requires Reporter Clarification Type: Testing
Projects
None yet
Milestone
1.68 Release
Development

Successfully merging this pull request may close these issues.
4 participants
@janardhanvissa @purnesh42H @easwars @janardhankrishna-sai