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

cmd/protoc-gen-go-grpc: add change detector test #7072

Merged
merged 23 commits into from
Apr 5, 2024
Merged

cmd/protoc-gen-go-grpc: add change detector test #7072

merged 23 commits into from
Apr 5, 2024

Conversation

arvindbr8
Copy link
Member

Fixes: #6748

RELEASE NOTES: none

@arvindbr8 arvindbr8 added the Type: Meta Github repo, process, etc label Apr 1, 2024
@arvindbr8 arvindbr8 added this to the 1.64 Release milestone Apr 1, 2024
@arvindbr8 arvindbr8 requested a review from dfawley April 1, 2024 21:03
@codecov Codecov
Copy link

codecov bot commented Apr 1, 2024 •
edited
Loading

Codecov Report

Merging #7072 (cc67268) into master (eb4e411) will decrease coverage by 0.05%.
The diff coverage is n/a.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #7072      +/-   ##
==========================================
- Coverage   81.27%   81.23%   -0.05%     
==========================================
  Files         345      345              
  Lines       33927    33927              
==========================================
- Hits        27574    27560      -14     
- Misses       5191     5198       +7     
- Partials     1162     1169       +7

see 15 files with indirect coverage changes

@arvindbr8 arvindbr8 mentioned this pull request Apr 2, 2024
Closed
dfawley
dfawley reviewed Apr 3, 2024
View reviewed changes
.github/workflows/testing.yml Outdated
Comment on lines 99 to 100
VET_SKIP_PROTO=1
./vet.sh -install && ./vet.sh
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I think these can fit on one line.

Copy link
Member Author

Choose a reason for hiding this comment

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

I dont know of a clean one line way to apply VET_SKIP_PROTO to both ./vet.sh commands

Copy link
Member

Choose a reason for hiding this comment

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

oh ... you need it set for both? :(

If you're interested, I've been meaning to delete VET_SKIP_PROTO and instead make it the default, with VET_CHECK_PROTO=1 set if you actually what to check the proto files.

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 a good idea. I can send a follow for that.

Copy link
Member Author

Choose a reason for hiding this comment

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

#7099

-- I'll actually wait for that to merge before merging this PR

.github/workflows/testing.yml Outdated Show resolved Hide resolved
cmd/protoc-gen-go-grpc/protoc-gen-go-grpc_test.sh Outdated Show resolved Hide resolved
.github/workflows/testing.yml Outdated Show resolved Hide resolved
.github/workflows/testing.yml Outdated Show resolved Hide resolved
cmd/protoc-gen-go-grpc/protoc-gen-go-grpc_test.sh Outdated Show resolved Hide resolved
regenerate.sh Outdated Show resolved Hide resolved
@dfawley dfawley assigned arvindbr8 and unassigned dfawley Apr 3, 2024
@arvindbr8 arvindbr8 requested a review from dfawley April 3, 2024 22:57
@arvindbr8 arvindbr8 assigned dfawley and unassigned arvindbr8 Apr 3, 2024
@arvindbr8
Copy link
Member Author

The ugliness here ^^ is because i was trying to run the Github workflow

@arvindbr8
Copy link
Member Author

vet-proto is expected to fail due to the golden_grpc.pb.go golden file that was added for the test.

@dfawley dfawley assigned arvindbr8 and unassigned dfawley Apr 4, 2024
@dfawley
Copy link
Member

dfawley commented Apr 4, 2024

vet-proto is expected to fail due to the golden_grpc.pb.go golden file that was added for the test.

That doesn't sound right, or are you saying it's because you were doing some temporary testing? Please make sure vet-proto is passing.

arvindbr8
arvindbr8 commented Apr 5, 2024
View reviewed changes
Copy link
Member Author

@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.

You are right. vet-proto is not supposed to fail. Fixed that and addressed your other comments.

@arvindbr8 arvindbr8 assigned dfawley and unassigned arvindbr8 Apr 5, 2024
@arvindbr8 arvindbr8 requested a review from dfawley April 5, 2024 18:02
@arvindbr8 arvindbr8 marked this pull request as draft April 5, 2024 18:03
@arvindbr8 arvindbr8 marked this pull request as ready for review April 5, 2024 22:48
dfawley
dfawley reviewed Apr 5, 2024
View reviewed changes
cmd/protoc-gen-go-grpc/protoc-gen-go-grpc_test.sh Outdated Show resolved Hide resolved
regenerate.sh Outdated Show resolved Hide resolved
@arvindbr8 arvindbr8 merged commit 6fbcd8a into grpc:master Apr 5, 2024
14 checks passed
@arvindbr8 arvindbr8 deleted the adding-protoc-gen-go-test branch May 30, 2024 00:24
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dfawley dfawley dfawley approved these changes

Assignees

@dfawley dfawley

Labels
Type: Meta Github repo, process, etc
Projects
None yet
Milestone
1.64 Release
Development

Successfully merging this pull request may close these issues.

protoc-gen-go-grpc: add tests
2 participants
@arvindbr8 @dfawley