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

Add support for nil pointer return values #53

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

solidDoWant
Copy link

This adds support for returning nil pointers from mocked functions. Currently, if the returned value is nil then the cast to the pointer type will cause a panic.

Here's an example:

Previous:

func (c *MockFilesClient) CopyFiles(ctx context.Context, in *CopyFilesRequest, opts ...grpc.CallOption) (*CopyFilesResponse, error) {
	opts0 := []interface{}{ctx, in}
	for _, opts1 := range opts {
		opts0 = append(opts0, opts1)
	}
	args := c.Called(opts0...)
	return args.Get(0), args.Error(1)
}

New:

func (c *MockFilesClient) CopyFiles(ctx context.Context, in *CopyFilesRequest, opts ...grpc.CallOption) (*CopyFilesResponse, error) {
	opts0 := []interface{}{ctx, in}
	for _, opts1 := range opts {
		opts0 = append(opts0, opts1)
	}
	args := c.Called(opts0...)
	var ret0 *CopyFilesResponse
	if args.Get(0) != nil {
		ret0 = args.Get(0).(*CopyFilesResponse)
	}
	return ret0, args.Error(1)
}

I also ran the makefile target to update the examples, which produced a lot of changes because they're out of date with current protoc tool versions. Let me know if you want me to revert these.

Copy link
Collaborator

@felix-kaestner felix-kaestner left a comment

Choose a reason for hiding this comment

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

@solidDoWant Nice addition. Keep in mind that using nil return values has already been supported through something like

var res *HelloReply
m.OnSayHello(ctx, req).Return(res, nil)

or even

m.OnSayHello(ctx, req).Return((*HelloReply)(nil), nil)

However I think this provides a nicer interface to most users. As a small improvement, could we do something like the following to avoid some of the branching?

diff --git a/internal/framework/testify.go b/internal/framework/testify.go
index d3d0d47..7f31c80 100644
--- a/internal/framework/testify.go
+++ b/internal/framework/testify.go
@@ -276,6 +276,9 @@ func (tm *testifyMocker) generateMethodDefinitions(g *protogen.GeneratedFile, me
 				g.P("if args.Get(", i, ") != nil {")
 				g.P("ret", i, " = args.Get(", i, ").(", r, ")")
 				g.P("}")
+
+				ret[i] = fmt.Sprintf("ret%d", i)
+				continue
 			}
 
 			switch r {
@@ -288,11 +291,7 @@ func (tm *testifyMocker) generateMethodDefinitions(g *protogen.GeneratedFile, me
 			case "error":
 				ret[i] = fmt.Sprintf("args.Error(%d)", i)
 			default:
-				if r.IsPointer() {
-					ret[i] = fmt.Sprintf("ret%d", i)
-				} else {
-					ret[i] = fmt.Sprintf("args.Get(%d).(%v)", i, r)
-				}
+				ret[i] = fmt.Sprintf("args.Get(%d).(%v)", i, r)
 			}
 		}
 		g.P("return ", strings.Join(ret, ", "))

And ideally also a test would be great.

Signed-off-by: solidDoWant <[email protected]>
@solidDoWant
Copy link
Author

And ideally also a test would be great.

Any suggestions on where? I don't see existing tests that I could easily add a test case for.

@felix-kaestner
Copy link
Collaborator

felix-kaestner commented Jan 28, 2025

Any suggestions on where? I don't see existing tests that I could easily add a test case for.

Would be great to add it inside of the examples in https://github.com/lovoo/protoc-gen-go-grpcmock/blob/main/examples/helloworld/testify/helloworld_test.go. I'd like to use that as sort of a playground to test out any changes we make to this project.

So essentially just adding another test that uses nil as return value, which would panic in the current implementation and will work fine after the additions of this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants