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

miledxz · 2024-01-08T21:35:35Z

Heey !
Im adding this pull request for this issue: #6748

I will add more detailed explanation, thank you a lot for the time taken !

little gentle ping for @dfawley
and one for @arvindbr8

NOTICE: there's a little issue with added test_unary.proto file maybe in vet.sh

I can update what's required

The goal of this pr is to add test coverage for using
protoc-gen-go-grpc internaly and compare results with usage of actual protoc —go-grpc=.out test.proto
from google.golang.org/protobuf/compiler/protogen

A)

I have added tests for unary examples, but maybe it's good idea to make sure is this even good approach,
including my approach in tests, function naming, const naming, too much init functions ?
Is this code in tests reusable, for adding future examples of unary rpc proto file test examples

Can something be better in tests ?

Also I have added a little example of same test, but in other format.
If you maybe prefer more this way.

B)

One thing thats not the best, is initPlugin Compiler versions,
Im using latest one, with v4.24.3 but have no clue how to use or update to current ones in time,
will take a closer look, so every time theres an update we don't have to update tests

C) Added Small Update on this since vet.sh is greping these comments , Will fix error handling better

Also I have not included “authors” comments that are generated above in pb.go files from proto files,
since generateFile function, does not do that, and if I want to do that
I can create helper function, that's gonna be called inside generateFile to do that job for me.
If this is even important.

D)

By the way, we are only using here protoc —go-grpc_out=. test.proto, command that is used
in wrapper function generateFile, that is using protogen, from protobuf/compiler/protogen

Is there maybe a plan of implementing protoc —go_out=. test.proto in other wrapper function,
for generating remaining pb.go file ?
Does grpc.pb.go feels uncomplete without other pb.go file ?

My plan is also to add additional tests, for all of helper functions that are being used.

E)

Also I have already tested with sever streaming proto file, and seems to work,
I can add that in this pull request as separated commit,
or as totaly separated pull request ?

Then we can add client and bidirectional streaming examples

F)

Btw, do u prefer more to compare files that are being written to, and remove them with cleanup,
or just to compare files content ?

G)

Other unary rpc test example:
Do u prefer more naming of const with: dummySomething, mockSomething, or just smthng ?

func TestGenerateFileUnaryRPC(t *testing.T) {
    testGeneratedGoProtoPath := "test_unary_grpc.pb.go"

    requireUnimplemented = proto.Bool(true)

    msg := []*descriptorpb.DescriptorProto{
        {
            Name: proto.String("EventRequest"),
            Field: []*descriptorpb.FieldDescriptorProto{
                {
                    Name:   proto.String("content"),
                    Type:   descriptorpb.FieldDescriptorProto_TYPE_BYTES.Enum(),
                    Number: proto.Int32(1),
                },
            },
        },
        {
            Name: proto.String("EventResponse"),
            Field: []*descriptorpb.FieldDescriptorProto{
                {
                    Name:   proto.String("content"),
                    Type:   descriptorpb.FieldDescriptorProto_TYPE_BYTES.Enum(),
                    Number: proto.Int32(1),
                },
            },
        },
    }

    mthd := []*descriptorpb.MethodDescriptorProto{
        {
            Name:       proto.String("addBlogPost"),
            InputType:  proto.String("EventRequest"),
            OutputType: proto.String("EventResponse"),
        },
    }

    svc := []*descriptorpb.ServiceDescriptorProto{
        {
            Name:   proto.String("BlogPostService"),
            Method: mthd,
        },
    }

    protoFile := []*descriptorpb.FileDescriptorProto{
        {
            Name:    proto.String("test_unary.proto"),
            Package: proto.String("main"),
            Options: &descriptorpb.FileOptions{
                GoPackage: proto.String("main/proto"),
            },
            MessageType: msg,
            Service:     svc,
        },
    }

    req := &pluginpb.CodeGeneratorRequest{
        ProtoFile:      protoFile,
        FileToGenerate: []string{"test_unary.proto"},
        CompilerVersion: &pluginpb.Version{
            Major: proto.Int32(4),
            Minor: proto.Int32(24),
            Patch: proto.Int32(3),
        },
    }

    plgn, err := protogen.Options{}.New(req)
    if err != nil {
        t.Errorf("Error while creating Plugin object: %v", plgn)
    }

    for _, f := range plgn.Files {
        if !f.Generate {
            continue
        }

        file := generateFile(plgn, f)
        fileContent, _ := file.Content()
        filePathUnary := strings.Replace(*f.Proto.Name, ".proto", "_grpc.pb.go", 1)

        err := os.WriteFile(filePathUnary, fileContent, 0644)
        if err != nil {
            t.Errorf("Error while writing go stub content to file: %s", err)
        }
    }

    testPluginContent, err := os.ReadFile(testGeneratedGoProtoPath)
    if err != nil {
        t.Errorf("Error while reading proto file")
    }

    protocPluginContent, err := os.ReadFile("testdata/proto/" + testGeneratedGoProtoPath)
    if err != nil {
        t.Errorf("Error while reading proto file")
    }

    if !reflect.DeepEqual(testPluginContent, protocPluginContent) {
        t.Errorf("Error generated go stubs are not equal")
    }

    t.Cleanup(func() {
        os.Remove(testGeneratedGoProtoPath)
    })
}

RELEASE NOTES: none

…oto command

codecov · 2024-01-08T21:38:40Z

Codecov Report

Merging #6910 (5b17419) into master (76a23bf) will decrease coverage by 0.22%.
Report is 31 commits behind head on master.
The diff coverage is n/a.

❗ Current head 5b17419 differs from pull request most recent head 2ce2a54. Consider uploading reports for the commit 2ce2a54 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6910      +/-   ##
==========================================
- Coverage   82.46%   82.25%   -0.22%     
==========================================
  Files         296      296              
  Lines       31465    31465              
==========================================
- Hits        25947    25880      -67     
- Misses       4463     4515      +52     
- Partials     1055     1070      +15

see 18 files with indirect coverage changes

miledxz · 2024-01-10T21:44:47Z

cmd/protoc-gen-go-grpc/grpc_test.go

+	req := &pluginpb.CodeGeneratorRequest{
+		ProtoFile:      initProtoFile(),
+		FileToGenerate: []string{"test_unary.proto"},
+		CompilerVersion: &pluginpb.Version{


This maybe can be an issue with up to date versions

arvindbr8 · 2024-01-10T22:37:42Z

Thanks for sending us this PR @zedGGs.

At a high level, we want to be testing the protoc-gen-go-grpc binary and not really unit test the behavior of generateFile(). There are a couple of ways of going about it,

go test - call cmd.Command() from a go test to run protoc-gen-go-grpc with the input proto and check the output
shell script - to build and exec the binary from the shell script. However inorder for the test to be run during CD we need to be able to call the shell script from a go test or create a new github workflow to call this shell script.

I'm more inclined for (1).

Let me know when you have the PR ready for review again and we can take another pass at it.

Also please find my responses inline for the questions asked.

Feel free to reach out if you have more questions.

Heey ! Im adding this pull request for this issue: #6748

I will add more detailed explanation, thank you a lot for the time taken !

little gentle ping for @dfawley and one for @arvindbr8

NOTICE: there's a little issue with added test_unary.proto file maybe in vet.sh

I can update what's required

arvindbr8: vet is complaining because the grpc_test.go file that was added is missing the copyright message at the top.

The goal of this pr is to add test coverage for using protoc-gen-go-grpc internaly and compare results with usage of actual protoc —go-grpc=.out test.proto from google.golang.org/protobuf/compiler/protogen

A)

I have added tests for unary examples, but maybe it's good idea to make sure is this even good approach, including my approach in tests, function naming, const naming, too much init functions ? Is this code in tests reusable, for adding future examples of unary rpc proto file test examples

arvindbr8: I'm not sure if the code in this test might be reused in the future. Although inflating the init functions inplace might be better for readability, but I have no strong preference. Although this might not be required if we go with the new approach

Can something be better in tests ?

arvindbr8: we should definitely add protos for all types of RPCs and tests as well. It might be easier to add this in this PR itself.

Also I have added a little example of same test, but in other format. If you maybe prefer more this way.

arvindbr8: Could you link to that please?

B)

One thing thats not the best, is initPlugin Compiler versions, Im using latest one, with v4.24.3 but have no clue how to use or update to current ones in time, will take a closer look, so every time theres an update we don't have to update tests

arvindbr8: Not relevant if we go with the new approach

C)

Also I have not included “authors” comments that are generated above in pb.go files from proto files, since generateFile function, does not do that, and if I want to do that I can create helper function, that's gonna be called inside generateFile to do that job for me. If this is even important.

arvindbr8: Not relevant if we go with the new approach

D)

By the way, we are only using here protoc —go-grpc_out=. test.proto, command that is used in wrapper function generateFile, that is using protogen, from protobuf/compiler/protogen

Is there maybe a plan of implementing protoc —go_out=. test.proto in other wrapper function, for generating remaining pb.go file ? Does grpc.pb.go feels uncomplete without other pb.go file ?

My plan is also to add additional tests, for all of helper functions that are being used.

arvindbr8: protoc uses our protoc-gen-go-grpc when we the flag --go-grpc-out is being used. protoc-gen-go-grpc is the only binary we own in our repo. We do not have to test the behavior of other plugins here.

We only want to be able to test changes made to protoc-gen-go-grpc binary.

E)

Also I have already tested with sever streaming proto file, and seems to work, I can add that in this pull request as separated commit, or as totaly separated pull request ?

Then we can add client and bidirectional streaming examples

arvindbr8: can we add all of that in this same PR if thats not too much effort. Seems like we can reuse the same message and only add additional rpcs to the service.

F)

Btw, do u prefer more to compare files that are being written to, and remove them with cleanup, or just to compare files content ?

arvindbr8: Let's remove them with cleanup

G)

Other unary rpc test example: Do u prefer more naming of const with: dummySomething, mockSomething, or just smthng ?

func TestGenerateFileUnaryRPC(t *testing.T) {
    testGeneratedGoProtoPath := "test_unary_grpc.pb.go"

    requireUnimplemented = proto.Bool(true)

    msg := []*descriptorpb.DescriptorProto{
        {
            Name: proto.String("EventRequest"),
            Field: []*descriptorpb.FieldDescriptorProto{
                {
                    Name:   proto.String("content"),
                    Type:   descriptorpb.FieldDescriptorProto_TYPE_BYTES.Enum(),
                    Number: proto.Int32(1),
                },
            },
        },
        {
            Name: proto.String("EventResponse"),
            Field: []*descriptorpb.FieldDescriptorProto{
                {
                    Name:   proto.String("content"),
                    Type:   descriptorpb.FieldDescriptorProto_TYPE_BYTES.Enum(),
                    Number: proto.Int32(1),
                },
            },
        },
    }

    mthd := []*descriptorpb.MethodDescriptorProto{
        {
            Name:       proto.String("addBlogPost"),
            InputType:  proto.String("EventRequest"),
            OutputType: proto.String("EventResponse"),
        },
    }

    svc := []*descriptorpb.ServiceDescriptorProto{
        {
            Name:   proto.String("BlogPostService"),
            Method: mthd,
        },
    }

    protoFile := []*descriptorpb.FileDescriptorProto{
        {
            Name:    proto.String("test_unary.proto"),
            Package: proto.String("main"),
            Options: &descriptorpb.FileOptions{
                GoPackage: proto.String("main/proto"),
            },
            MessageType: msg,
            Service:     svc,
        },
    }

    req := &pluginpb.CodeGeneratorRequest{
        ProtoFile:      protoFile,
        FileToGenerate: []string{"test_unary.proto"},
        CompilerVersion: &pluginpb.Version{
            Major: proto.Int32(4),
            Minor: proto.Int32(24),
            Patch: proto.Int32(3),
        },
    }

    plgn, err := protogen.Options{}.New(req)
    if err != nil {
        t.Errorf("Error while creating Plugin object: %v", plgn)
    }

    for _, f := range plgn.Files {
        if !f.Generate {
            continue
        }

        file := generateFile(plgn, f)
        fileContent, _ := file.Content()
        filePathUnary := strings.Replace(*f.Proto.Name, ".proto", "_grpc.pb.go", 1)

        err := os.WriteFile(filePathUnary, fileContent, 0644)
        if err != nil {
            t.Errorf("Error while writing go stub content to file: %s", err)
        }
    }

    testPluginContent, err := os.ReadFile(testGeneratedGoProtoPath)
    if err != nil {
        t.Errorf("Error while reading proto file")
    }

    protocPluginContent, err := os.ReadFile("testdata/proto/" + testGeneratedGoProtoPath)
    if err != nil {
        t.Errorf("Error while reading proto file")
    }

    if !reflect.DeepEqual(testPluginContent, protocPluginContent) {
        t.Errorf("Error generated go stubs are not equal")
    }

    t.Cleanup(func() {
        os.Remove(testGeneratedGoProtoPath)
    })
}

arvindbr8: Not application if we take the new approach

arvindbr8

Please take a look at my comment above and let me know when its ready for review again.

miledxz · 2024-01-12T06:24:08Z

Please take a look at my comment above and let me know when its ready for review again.

@arvindbr8 thank you a lot for the awesome review and all the explanation and advices in general and in all inline comments

I will go with first option as u advise, add all protos for different types of rpcs, use cleanup, all in same pr

arvindbr8 · 2024-01-12T21:41:00Z

@zedGGs -- I guess your changes were not pushed to your branch? Please take a look

github-actions · 2024-01-18T22:45:46Z

This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

miledxz · 2024-01-24T06:05:06Z

@arvindbr8 yes, something similar ! and then I went on vacation,
I will add an update soon, thank you !

github-actions · 2024-01-30T06:45:50Z

This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

arvindbr8 · 2024-02-21T18:08:02Z

Since @dfawley has more context I'll reassign it to him

arvindbr8 · 2024-02-22T17:44:36Z

@zedGGs - I would be cautious when I do a force push to your branch. Github UI handles that weirdly and sometimes have gotten rid of the previous conversations on the PR.

Also, Doug is on vacation and will be back next week.

miledxz · 2024-02-22T20:15:02Z

@arvindbr8 sure, can agree on that, I think it's all ready and good at this point,
maybe I can put some of our most important conversation here so its all at one place

an update with most important conversation, I think this could cover all of the idea:

here: #6910 (comment)
here: #6910 (comment)

arvindbr8

I remember approving it already. Let's wait on dfawley's review before we can merge it

dfawley

Thanks for the PR.. I have a few comments but overall this looks really good!

dfawley · 2024-03-15T15:31:41Z

.github/workflows/testing.yml

@@ -37,8 +37,6 @@ jobs:
  # Run the main gRPC-Go tests.
  tests:
    # Proto checks are run in the above job.


This comment should be moved with the env lines and updated to something like in the vet-proto job.

make sense, I can do that

dfawley · 2024-03-15T15:33:50Z

.github/workflows/testing.yml

+        env:
+          VET_BUILD_GO_PROTOC: 1
        run: |
+          ./vet.sh -install
+          protoc --version


This doesn't want to be run for every version of Go that we test, presumably.

Let's test it in extras instead, below?

sure, that's reasonable

dfawley · 2024-03-15T15:37:13Z

cmd/protoc-gen-go-grpc/grpc_test.go

+)
+
+func TestProtocBinary(t *testing.T) {
+	workingDir, err := os.Getwd()


I don't think we need this workingDir at all. os.MkdirTemp uses the default temp directory if the first parameter is "" which seems perfectly fine and command.Dir defaults to the current directory already.

make sense, will do

dfawley · 2024-03-15T15:41:47Z

cmd/protoc-gen-go-grpc/grpc_test.go

+			protoFile: "test_bidirectional_streaming.proto",
+		},
+	} {
+		protoFilePath := "./proto/" + spec.protoFile


Please use path.Join instead.

sounds like a plan

dfawley · 2024-03-15T15:44:54Z

cmd/protoc-gen-go-grpc/grpc_test.go

+		if !reflect.DeepEqual(testPluginContent, protocPluginContent) {
+			t.Errorf("error generated go test stubs are not equal")
+		}


Please use cmp.Diff and show the results so if things change we can see exactly what.

sounds like an awesome plan

dfawley · 2024-03-15T15:45:26Z

cmd/protoc-gen-go-grpc/grpc_test.go

+
+		pbgoFile := strings.Replace(spec.protoFile, ".proto", "_grpc.pb.go", 1)
+
+		testPluginContent, err := os.ReadFile(tmpDir + "/testdata/proto/" + pbgoFile)


Same comment re: path.Join and below.

dfawley · 2024-03-15T15:48:36Z

regenerate.sh

+# excluding grpc_testing_not_regenerate and cmd/protoc-gen-go-grpc directories
+rm -f $(find . -name '*.pb.go' | grep -v -e 'grpc_testing_not_regenerate' -e 'cmd/protoc-gen-go-grpc')


Actually, I have renamed the files in grpc_test_not_regenerate to not be .pb.go anymore, so you can delete this part.

github-actions · 2024-03-26T04:45:46Z

This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

github-actions · 2024-04-01T08:45:45Z

This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

arvindbr8 · 2024-04-02T17:24:35Z

Since this PR is going stale, I decided to open #7072 to attempt to fix it.

miledxz · 2024-04-02T17:30:49Z

Heey @arvindbr8 , is there still possibility for me to finalise this pr ? I happen to be on vacation so I haven't reply on time, but Im available now,
of course if @dfawley agrees on this, I understand you already added another pr

arvindbr8 · 2024-04-02T23:13:59Z

Sorry about that @zedGGs. Please feel free to take up another issue in the repo. We would love to take a look at your PR.

miledxz added 2 commits January 8, 2024 22:01

adding inital files generated by protoc --go-grpc_out=. test_unary.pr…

2d88560

…oto command

adding tests for generating unary rpc grpc files

a42eb89

Merge branch 'grpc:master' into adding-protoc-gen-go-grpc-tests

9e8e728

dfawley assigned arvindbr8 Jan 10, 2024

dfawley requested a review from arvindbr8 January 10, 2024 18:22

dfawley added the Type: Testing label Jan 10, 2024

dfawley added this to the 1.61 Release milestone Jan 10, 2024

adding additional update for handling proto author comments

cb67398

miledxz commented Jan 10, 2024

View reviewed changes

arvindbr8 removed their assignment Jan 10, 2024

arvindbr8 suggested changes Jan 10, 2024

View reviewed changes

arvindbr8 added the Status: Requires Reporter Clarification label Jan 10, 2024

arvindbr8 assigned miledxz Jan 10, 2024

arvindbr8 added Status: Requires Reporter Clarification and removed Status: Requires Reporter Clarification labels Jan 12, 2024

github-actions bot added the stale label Jan 18, 2024

zasweq modified the milestones: 1.61 Release, 1.62 Release Jan 23, 2024

github-actions bot removed the stale label Jan 23, 2024

github-actions bot added stale and removed stale labels Jan 30, 2024

miledxz and others added 2 commits January 31, 2024 07:56

Merge branch 'grpc:master' into adding-protoc-gen-go-grpc-tests

19b18c8

adding required tests and proto files updates

2aeaffd

arvindbr8 assigned dfawley and unassigned zasweq Feb 21, 2024

Merge branch 'grpc:master' into adding-protoc-gen-go-grpc-tests

cac7cce

miledxz force-pushed the adding-protoc-gen-go-grpc-tests branch from bcde830 to 5b17419 Compare February 21, 2024 19:26

adding update for regenerate.sh

2ce2a54

miledxz force-pushed the adding-protoc-gen-go-grpc-tests branch from 5b17419 to 2ce2a54 Compare February 21, 2024 19:27

arvindbr8 requested a review from dfawley February 26, 2024 21:29

arvindbr8 changed the title ~~Adding protoc gen go grpc tests~~ cmd/protoc-gen-go-grpc: add protoc change detector test Feb 26, 2024

miledxz requested a review from arvindbr8 February 27, 2024 18:05

arvindbr8 approved these changes Feb 28, 2024

View reviewed changes

dfawley reviewed Mar 15, 2024

View reviewed changes

dfawley assigned miledxz and unassigned dfawley Mar 15, 2024

dfawley added the Status: Requires Reporter Clarification label Mar 15, 2024

zasweq modified the milestones: 1.63 Release, 1.64 Release Mar 20, 2024

github-actions bot added stale and removed stale labels Mar 26, 2024

arvindbr8 mentioned this pull request Mar 26, 2024

scripts: improve regenerate.sh to use the correct proto compiler version #7064

Merged

github-actions bot added the stale label Apr 1, 2024

arvindbr8 closed this Apr 2, 2024

github-actions bot locked as resolved and limited conversation to collaborators Sep 30, 2024


		pbgoFile := strings.Replace(spec.protoFile, ".proto", "_grpc.pb.go", 1)

		testPluginContent, err := os.ReadFile(tmpDir + "/testdata/proto/" + pbgoFile)

		# excluding grpc_testing_not_regenerate and cmd/protoc-gen-go-grpc directories
		rm -f $(find . -name '*.pb.go' \| grep -v -e 'grpc_testing_not_regenerate' -e 'cmd/protoc-gen-go-grpc')

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

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

Uh oh!

Conversation

miledxz commented Jan 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jan 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arvindbr8 commented Jan 10, 2024

Uh oh!

arvindbr8 left a comment

Choose a reason for hiding this comment

Uh oh!

miledxz commented Jan 12, 2024

Uh oh!

arvindbr8 commented Jan 12, 2024

Uh oh!

github-actions bot commented Jan 18, 2024

Uh oh!

miledxz commented Jan 24, 2024

Uh oh!

github-actions bot commented Jan 30, 2024

Uh oh!

arvindbr8 commented Feb 21, 2024

Uh oh!

arvindbr8 commented Feb 22, 2024

Uh oh!

miledxz commented Feb 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arvindbr8 left a comment

Choose a reason for hiding this comment

Uh oh!

dfawley left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 26, 2024

Uh oh!

github-actions bot commented Apr 1, 2024

Uh oh!

arvindbr8 commented Apr 2, 2024

Uh oh!

miledxz commented Apr 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arvindbr8 commented Apr 2, 2024

Uh oh!

miledxz commented Jan 8, 2024 •

edited

Loading

codecov bot commented Jan 8, 2024 •

edited

Loading

miledxz commented Feb 22, 2024 •

edited

Loading

miledxz commented Apr 2, 2024 •

edited

Loading