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

Removing testservice implementation and using stubserver #12

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

janardhanvissa
Copy link
Owner

No description provided.

Copy link

@easwars easwars left a comment

Choose a reason for hiding this comment

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

Changes look fine overall. But I think in all tests, if possible we should let the stubserver register the test service instead of explicitly doing it from the test.

server, err := xds.NewGRPCServer(grpc.Creds(insecure.NewCredentials()), modeChangeOpt, xds.BootstrapContentsForTesting(bootstrapContents))
if err != nil {
t.Fatalf("Failed to create an xDS enabled gRPC server: %v", err)
}
t.Cleanup(server.Stop)
testgrpc.RegisterTestServiceServer(server, &testService{})
testgrpc.RegisterTestServiceServer(server, stub)
Copy link

Choose a reason for hiding this comment

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

I think we should be doing something like:

	stub := &stubserver.StubServer{
		EmptyCallF: func(ctx context.Context, in *testpb.Empty) (*testpb.Empty, error) {
			return &testpb.Empty{}, nil
		},
		UnaryCallF: func(ctx context.Context, in *testpb.SimpleRequest) (*testpb.SimpleResponse, error) {
			return &testpb.SimpleResponse{}, nil
		},
	}
	// Create the xDS-enabled gRPC server like how we are doing today
	stub.S = server
	stubserver.StartTestService(t, stub) // instead of manually registering the test service

@@ -161,7 +162,7 @@ func (s) TestServerSideXDS_WithNoCertificateProvidersInBootstrap_Failure(t *test
if err != nil {
t.Fatalf("Failed to create an xDS enabled gRPC server: %v", err)
}
testgrpc.RegisterTestServiceServer(server, &testService{})
testgrpc.RegisterTestServiceServer(server, &stubserver.StubServer{})
Copy link

Choose a reason for hiding this comment

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

Same comment as above.

server, err := xds.NewGRPCServer(grpc.Creds(creds), modeChangeOpt, xds.BootstrapContentsForTesting(bootstrapContents))
if err != nil {
t.Fatalf("Failed to create an xDS enabled gRPC server: %v", err)
}
testgrpc.RegisterTestServiceServer(server, &testService{})
testgrpc.RegisterTestServiceServer(server, stub)
Copy link

Choose a reason for hiding this comment

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

And here too.

@easwars
Copy link

easwars commented Oct 7, 2024

I see that while some tests create an empty stub server like stub := &stubserver.StubServer{}, set the S field and call StartTestService, and some tests actually provide custom implementation of some of the RPC handlers when creating the stubserver. Both of these are fine. But there are some tests which provide an implementation of some of the RPC handlers, but these implementations don't seem to be doing anything significant like the one in test/xds/xds_client_ignore_resource_deletion_test.go. Can we instead use the first option of creating an empty stub server, setting the S field and calling StartTestService for tests like these too?

@janardhanvissa
Copy link
Owner Author

An empty stub server will give invalid memory address or nil pointer dereference right? How to approach these, please suggest.

@easwars
Copy link

easwars commented Oct 7, 2024

An empty stub server will give invalid memory address or nil pointer dereference right? How to approach these, please suggest.

Ah, there is a check for if server == nil { ... } in StartTestService before setting up the RPC handlers. Then, what you have is good I guess. Maybe you can send them out for review on the grpc repo. Thanks.

@janardhanvissa
Copy link
Owner Author

Yes, there is a check for that. Sure will raise a PR on the main repo. Thanks for the confirmation.

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.

3 participants