Adding protoc gen go grpc tests

[miledxz]

Heey ! Im adding this pull request for this issue: https://github.com/grpc/grpc-go/issues/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

[codecov[bot]]

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.

:exclamation: 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

[arvindbr8]

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,

1. go test - call cmd.Command() from a go test to run protoc-gen-go-grpc with the input proto and check the output
2. 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

[miledxz]

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]

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

[github-actions[bot]]

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]

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

[github-actions[bot]]

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]

Heeya @arvindbr8 ! I have updated pr, tests are going well locally, but is there possibility that "protoc" plugin is not installed on these machines in pipeline or something similar to that ?

Of course I can update tests again, I appreciate all your time for reviews ! Also, I apologise for late update

[dfawley]

I have updated pr, tests are going well locally, but is there possibility that "protoc" plugin is not installed on these machines in pipeline or something similar to that ?

You can call /vet.sh -install to make our existing vet script install it, or you can do something similar elsewhere. I'd prefer to have only one place where we download/install protoc, or at least one place where we define the version to download.

[miledxz]

@dfawley , thank you for the feedback and explanation, I will do that

[miledxz]

@arvindbr8 thank you for more detailed explanation, I have added few updates, also here's little ping for @dfawley

thank you guys !

btw do u prefer more current update of pipeline with:

        if: matrix.type == 'tests'
        env:
          VET_BUILD_GO_PROTOC: 1

and vet.sh with:

      if [[ -n "${VET_BUILD_GO_PROTOC}" ]]; then
        pushd cmd/protoc-gen-go-grpc
        go build -o /home/runner/go/bin .
        popd
      fi

or u want to directly update only pipeline with:

    pushd cmd/protoc-gen-go-grpc
    go build -o /home/runner/go/bin .
    popd

[miledxz]

@arvindbr8 sure, feel free to add your commits, no worries u are not wasting my time at all

[arvindbr8]

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

[arvindbr8]

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

@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: https://github.com/grpc/grpc-go/pull/6910#discussion_r1474887421 here: https://github.com/grpc/grpc-go/pull/6910#discussion_r1483600630

[github-actions[bot]]

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[bot]]

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]

Since this PR is going stale, I decided to open https://github.com/grpc/grpc-go/pull/7072 to attempt to fix it.

[miledxz]

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]

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.