authz: modify the tests to use stubserver instead of testservice implementations #7888

janardhanvissa · 2024-12-02T14:00:35Z

Partially Addresses: #7291

RELEASE NOTES: None

codecov · 2024-12-02T14:07:21Z

Codecov Report

Attention: Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 82.22%. Comparing base (724f450) to head (136cc82).

Files with missing lines	Patch %	Lines
internal/stubserver/stubserver.go	50.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #7888      +/-   ##
==========================================
+ Coverage   82.05%   82.22%   +0.17%     
==========================================
  Files         381      381              
  Lines       38539    38543       +4     
==========================================
+ Hits        31622    31691      +69     
+ Misses       5602     5552      -50     
+ Partials     1315     1300      -15

Files with missing lines	Coverage Δ
internal/stubserver/stubserver.go	`80.00% <50.00%> (-0.92%)`	⬇️

... and 29 files with indirect coverage changes

purnesh42H

1 comment otherwise lgtm. Good work!

authz/audit/audit_logging_test.go

purnesh42H · 2024-12-06T06:59:52Z

internal/stubserver/stubserver.go

+	EmptyCallF           func(ctx context.Context, in *testpb.Empty) (*testpb.Empty, error)
+	UnaryCallF           func(ctx context.Context, in *testpb.SimpleRequest) (*testpb.SimpleResponse, error)
+	FullDuplexCallF      func(stream testgrpc.TestService_FullDuplexCallServer) error
+	StreamingInputCallF  func(stream testgrpc.TestService_StreamingInputCallServer) error


@janardhanvissa i think we don't need to create extra functions for input and output. Can you just use the FullDuplexCallF for streaming calls

grpc-go/authz/audit/audit_logging_test.go

Line 250 in 66ba4b2

FullDuplexCallF: func(stream testgrpc.TestService_FullDuplexCallServer) error {

?

Discussed offline. We need to use stream.CloseAndRecv() so we need a client streaming handler. We should rename it to ClientStreamingInputCall and ClientStreamingOutputCall though

internal/stubserver/stubserver.go

purnesh42H · 2024-12-20T09:26:31Z

internal/stubserver/stubserver.go

+	UnaryCallF                func(ctx context.Context, in *testpb.SimpleRequest) (*testpb.SimpleResponse, error)
+	FullDuplexCallF           func(stream testgrpc.TestService_FullDuplexCallServer) error
+	ClientStreamingInputCall  func(stream testgrpc.TestService_StreamingInputCallServer) error
+	ClientStreamingOutputCall func(req *testpb.StreamingOutputCallRequest, stream testgrpc.TestService_StreamingOutputCallServer) error


@janardhanvissa let's change this StreamingInputCall and StreamingOutputCall and in the comment mention that that input is for ClientStreaming and output is for ServerStreaming

…to stubserver-streamingcall

purnesh42H · 2024-12-21T12:43:21Z

internal/stubserver/stubserver.go

+	EmptyCallF           func(ctx context.Context, in *testpb.Empty) (*testpb.Empty, error)
+	UnaryCallF           func(ctx context.Context, in *testpb.SimpleRequest) (*testpb.SimpleResponse, error)
+	FullDuplexCallF      func(stream testgrpc.TestService_FullDuplexCallServer) error
+	StreamingInputCallF  func(stream testgrpc.TestService_StreamingInputCallServer) error                                          // ClientStreaming


Next to StreamingInputCallF please mention Client-Streaming request

purnesh42H

lgtm. Please address the comment suggestion for Streaming calls

purnesh42H · 2024-12-21T12:43:59Z

internal/stubserver/stubserver.go

+	UnaryCallF           func(ctx context.Context, in *testpb.SimpleRequest) (*testpb.SimpleResponse, error)
+	FullDuplexCallF      func(stream testgrpc.TestService_FullDuplexCallServer) error
+	StreamingInputCallF  func(stream testgrpc.TestService_StreamingInputCallServer) error                                          // ClientStreaming
+	StreamingOutputCallF func(req *testpb.StreamingOutputCallRequest, stream testgrpc.TestService_StreamingOutputCallServer) error // ServerStreaming


Next to StreamingOutputCallF please mention Server-streaming response

purnesh42H · 2025-01-07T13:20:54Z

@easwars for second review

easwars · 2025-01-07T19:50:33Z

authz/audit/audit_logging_test.go

-				t.Fatalf("Error listening: %v", err)
-			}
-			go s.Serve(lis)
+			ss.S = s


While you are here, may I request you to make the following additional changes:

Move the whole section under the comment // Construct the credentials for the tests and the stub server to here. That will make the subtests executed as part of the for loop (via the call to t.Run) be independent of each other.

Pass the arguments to the call to grpc.NewServer in a single line. See: https://google.github.io/styleguide/go/guide#line-length

Thanks.

You seemed to have moved it to the top of the test. That is not what I requested. I requested that the creation of the creds and the stubserver be moved inside of t.Run. That is the only way to make the subtests hermetic.

authz/audit/audit_logging_test.go

authz/grpc_authz_end2end_test.go

easwars · 2025-01-07T20:01:09Z

internal/stubserver/stubserver.go

+	StreamingInputCallF  func(stream testgrpc.TestService_StreamingInputCallServer) error                                          // Client-Streaming request
+	StreamingOutputCallF func(req *testpb.StreamingOutputCallRequest, stream testgrpc.TestService_StreamingOutputCallServer) error // Server-streaming response


Nit: Please remove the trailing comments for the newly added fields since the other existing fields don't have them and there is a top-level comment saying these are customizable implementations of server handlers, which is explanatory enough I feel. Thanks.

…to stubserver-streamingcall

janardhanvissa · 2025-01-08T17:31:34Z

All the above mentioned comments are addressed.

easwars

LGTM, modulo minor nits raised in the most recent round of review.

easwars · 2025-01-09T22:27:04Z

authz/audit/audit_logging_test.go

-				t.Fatalf("Error listening: %v", err)
-			}
-			go s.Serve(lis)
+			ss.S = s


You seemed to have moved it to the top of the test. That is not what I requested. I requested that the creation of the creds and the stubserver be moved inside of t.Run. That is the only way to make the subtests hermetic.

easwars · 2025-01-09T22:29:09Z

authz/grpc_authz_end2end_test.go

+			return &testpb.SimpleResponse{}, nil
+		},
+		// Start a gRPC server with gRPC authz unary server interceptor.
+		S: grpc.NewServer(grpc.ChainUnaryInterceptor(i.UnaryInterceptor))}


Nit: Please terminate that line with a , and move the } to the next line. That way the opening and closing curly braces will match.

easwars · 2025-01-09T22:29:25Z

authz/grpc_authz_end2end_test.go

+		UnaryCallF: func(ctx context.Context, req *testpb.SimpleRequest) (*testpb.SimpleResponse, error) {
+			return &testpb.SimpleResponse{}, nil
+		},
+		S: grpc.NewServer(grpc.ChainUnaryInterceptor(i.UnaryInterceptor))}


Nit: Please terminate that line with a , and move the } to the next line. That way the opening and closing curly braces will match.

…ementations (grpc#7888)

Initializing StreamingInputCall and SteamingOutputCall in the stubserver

cb0a479

arjan-bal requested a review from purnesh42H December 3, 2024 05:45

arjan-bal assigned purnesh42H Dec 3, 2024

arjan-bal added this to the 1.69 Release milestone Dec 3, 2024

arjan-bal added the Type: Testing label Dec 3, 2024

janardhankrishna-sai added 2 commits December 3, 2024 07:00

switching to stubserver in tests instead of testserviceimpl

d364fdc

switching to stubserver instead of testservice

309abd1

purnesh42H modified the milestones: 1.69 Release, 1.70 Release Dec 5, 2024

purnesh42H reviewed Dec 5, 2024

View reviewed changes

authz/audit/audit_logging_test.go Show resolved Hide resolved

purnesh42H assigned janardhanvissa and unassigned purnesh42H Dec 5, 2024

removing redundant server code

fa74a99

purnesh42H reviewed Dec 6, 2024

View reviewed changes

purnesh42H changed the title ~~stubserver: Initializing StreamingInputCall and StreamingOutputCall in the stubserver~~ authz: Modify the tests to use stubserver instead of testservice implementations Dec 6, 2024

purnesh42H changed the title ~~authz: Modify the tests to use stubserver instead of testservice implementations~~ authz: modify the tests to use stubserver instead of testservice implementations Dec 6, 2024

janardhankrishna-sai added 2 commits December 18, 2024 09:49

renaming streamingcalls

1e7a099

moving initialization of NewServer inside stub

924a9e7

purnesh42H reviewed Dec 20, 2024

View reviewed changes

internal/stubserver/stubserver.go Show resolved Hide resolved

internal/stubserver/stubserver.go Show resolved Hide resolved

purnesh42H reviewed Dec 20, 2024

View reviewed changes

renaming client and server handlers

ecdab23

janardhanvissa force-pushed the stubserver-streamingcall branch from cfba48c to ecdab23 Compare December 20, 2024 09:56

janardhankrishna-sai added 2 commits December 20, 2024 10:10

updating grpc_authz_end2end_test.go

7287983

Merge branch 'master' of https://github.com/janardhanvissa/grpc-go in…

5b41dbe

…to stubserver-streamingcall

purnesh42H reviewed Dec 21, 2024

View reviewed changes

purnesh42H approved these changes Dec 21, 2024

View reviewed changes

janardhankrishna-sai added 2 commits December 22, 2024 06:23

updating client and server streaming comments

4588fc1

removing empty lines for re-trigger

4298e86

purnesh42H requested a review from easwars January 7, 2025 13:20

purnesh42H assigned easwars and unassigned janardhanvissa Jan 7, 2025

easwars reviewed Jan 7, 2025

View reviewed changes

easwars assigned janardhanvissa and unassigned easwars Jan 7, 2025

janardhankrishna-sai added 2 commits January 8, 2025 07:57

Merge branch 'master' of https://github.com/janardhanvissa/grpc-go in…

96cd53a

…to stubserver-streamingcall

removing explicit listener creation code

136cc82

easwars self-assigned this Jan 8, 2025

easwars approved these changes Jan 9, 2025

View reviewed changes

easwars removed their assignment Jan 9, 2025

moving creds and the stubserver code inside of t.Run

754386e

purnesh42H merged commit f35fb34 into grpc:master Jan 13, 2025
12 of 13 checks passed

janardhanvissa added a commit to janardhanvissa/grpc-go that referenced this pull request Jan 26, 2025

authz: modify the tests to use stubserver instead of testservice impl…

5b6d298

…ementations (grpc#7888)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

authz: modify the tests to use stubserver instead of testservice implementations #7888

authz: modify the tests to use stubserver instead of testservice implementations #7888

janardhanvissa commented Dec 2, 2024

codecov bot commented Dec 2, 2024 •

edited

Loading

purnesh42H left a comment

purnesh42H Dec 6, 2024

purnesh42H Dec 18, 2024

purnesh42H Dec 20, 2024

purnesh42H Dec 21, 2024 •

edited

Loading

purnesh42H left a comment

purnesh42H Dec 21, 2024

purnesh42H commented Jan 7, 2025

easwars Jan 7, 2025

easwars Jan 9, 2025

easwars Jan 7, 2025

janardhanvissa commented Jan 8, 2025

easwars left a comment

easwars Jan 9, 2025

easwars Jan 9, 2025

easwars Jan 9, 2025

		StreamingInputCallF func(stream testgrpc.TestService_StreamingInputCallServer) error // Client-Streaming request
		StreamingOutputCallF func(req *testpb.StreamingOutputCallRequest, stream testgrpc.TestService_StreamingOutputCallServer) error // Server-streaming response

authz: modify the tests to use stubserver instead of testservice implementations #7888

authz: modify the tests to use stubserver instead of testservice implementations #7888

Conversation

janardhanvissa commented Dec 2, 2024

codecov bot commented Dec 2, 2024 • edited Loading

Codecov Report

purnesh42H left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

purnesh42H Dec 21, 2024 • edited Loading

Choose a reason for hiding this comment

purnesh42H left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

purnesh42H commented Jan 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janardhanvissa commented Jan 8, 2025

easwars left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 2, 2024 •

edited

Loading

purnesh42H Dec 21, 2024 •

edited

Loading