grpc_test: add tests for cardinality violation #8120

eshitachandwani · 2025-02-25T09:59:02Z

Fixes: #7570

Added tests for cardinality violations :

Client should ensure a status internal error is returned for client-streaming RPCs if the server doesn't send a message before returning status OK.
Also for client-streaming RPCs, when SendAndClose is called, the server should perform a RST_STREAM() after sending the response message successfully.

Skipping the tests for now till the issues are fixed: #8119
RELEASE NOTES: N/A

test/end2end_test.go

codecov · 2025-02-25T10:05:08Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.34%. Comparing base (ae2a04f) to head (ad7ff22).
Report is 21 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8120      +/-   ##
==========================================
+ Coverage   82.29%   82.34%   +0.05%     
==========================================
  Files         387      389       +2     
  Lines       38967    39103     +136     
==========================================
+ Hits        32066    32201     +135     
- Misses       5586     5588       +2     
+ Partials     1315     1314       -1

see 50 files with indirect coverage changes

dfawley · 2025-02-27T21:27:31Z

test/end2end_test.go

+	earlyNil           bool // whether to return nil without calling SendAndClose().
+	recvAfterClose     bool // whether to call Recv() after calling SendAndClose().


We really really don't want to extend testServer. Could you use the stubserver package instead? testServer has a lot of bad properties (especially: it splits the testing code into two different places: the test and the testServer), and this only complicates it further by adding more ways to conditionalize its behavior.

You can follow this kind of example to use stubserver instead:

grpc-go/test/end2end_test.go

Line 4485 in d48317f

ss := &stubserver.StubServer{

Also please add a comment on testServer that it should not be used for any new tests so that this doesn't happen again.

Sorry!

purnesh42H

Looks like we need one more test case when "Server Doesn't RST_STREAM"

Server-Side:

Create a client-streaming RPC handler.
The handler calls SendAndClose with a valid response message.
The handler then attempts to call Recv.

Client-Side:

Make a client-streaming RPC call.
Continuously send messages.

Expected Outcome:

The server's Recv call should return an error indicating that the stream is closed.
The client's Send calls should eventually return an error indicating that the stream has been reset.

purnesh42H · 2025-03-04T04:33:44Z

test/end2end_test.go

@@ -3585,6 +3586,90 @@ func testClientStreamingError(t *testing.T, e env) {
 	}
 }

+func (s) TestClientStreamingMissingSendAndCloseError(t *testing.T) {


Please add docstring for this explaining what its verifying i.e. what is the expected behavior

purnesh42H · 2025-03-04T04:34:00Z

test/end2end_test.go

+	testClientStreamingRecvAfterCloseError(t)
+}
+
+func testClientStreamingRecvAfterCloseError(t *testing.T) {


Please add docstring for this explaining what its verifying i.e. what is the expected behavior

purnesh42H · 2025-03-04T04:34:30Z

test/end2end_test.go

+	// TODO : https://github.com/grpc/grpc-go/issues/8119 - remove `t.Skip()`
+	// after this is fixed.
+	t.Skip()
+	testClientStreamingMissingSendAndCloseError(t)


why are moving the test logic to helper? Can't we just write within this test?

purnesh42H · 2025-03-04T04:38:15Z

test/end2end_test.go

+		t.Fatalf(".StreamingInputCall(_) = _, %v, want <nil>", err)
+	}
+
+	if _, err := stream.CloseAndRecv(); status.Code(err) != codes.Internal {


Client streaming RPCs must terminate with either no message and a non-OK status, or a message and an OK status.

Based on #7570, since we are not sending message from handler, i think we need to verify if we are getting the error and the status is not-ok?

Also in this case, we should receive Error log messages indicating what happened and that it is illegal behavior.

how do we verify this? I think there are some existing tests which parse the error logs and verify

Also, it might be good to have separate if condition for both error and status violation. Although they are always supposed to happen in conjuction but checking them separately make it easier to understand.

if _, err := stream.CloseAndRecv(); err == nil { t.Fatalf("CloseAndRecv should have returned an error") } if status.Code(err) == status.ok { t.Fatalf("CloseAndRecv should have returned a non-ok") }

purnesh42H · 2025-03-04T05:02:50Z

test/end2end_test.go

@@ -3585,6 +3586,90 @@ func testClientStreamingError(t *testing.T, e env) {
 	}
 }

+func (s) TestClientStreamingMissingSendAndCloseError(t *testing.T) {


suggestion for name to be more explicit TestClientStreamingCardinalityViolation_ServerHandlerMissingSendAndClose

purnesh42H · 2025-03-04T05:04:13Z

test/end2end_test.go

+	}
+}
+
+func (s) TestClientStreamingRecvAfterCloseError(t *testing.T) {


suggestion for name to be more explicit TestClientStreamingCardinalityViolation_IgnoreServerHandlerErrorAfterSendAndClose

purnesh42H · 2025-03-04T05:04:50Z

test/end2end_test.go

+			stream.SendAndClose(&testpb.StreamingInputCallResponse{
+				AggregatedPayloadSize: int32(sum),
+			})
+			_, err := stream.Recv()


nit: do we need to do this? What if just return a static error with status code like status.Error(codes.Internal, "something went wrong")). ? Because calling stream.Recv() adding one more possibility to the test case

purnesh42H · 2025-03-04T05:08:15Z

test/end2end_test.go

+		if err = stream.Send(req); err == nil {
+			continue
+		}
+		if _, err := stream.CloseAndRecv(); status.Code(err) != codes.Internal {


nit: We should probably just break out of loop when server closes the stream and call the CloseAndRecv() outside

purnesh42H · 2025-03-04T05:12:23Z

test/end2end_test.go

+			continue
+		}
+		if _, err := stream.CloseAndRecv(); status.Code(err) != codes.Internal {
+			t.Fatalf("%v.CloseAndRecv() = %v, want error %s", stream, err, codes.Internal)


Similar comment here as above, probably good to write 2 separate ifs but the opposite way in this case. 1) raise t.Fatalf if error is not nil. 2) raise t.Fatalf if status is not ok

eshitachandwani · 2025-03-06T16:19:11Z

Looks like we need one more test case when "Server Doesn't RST_STREAM"

Server-Side:

Create a client-streaming RPC handler.

The handler calls SendAndClose with a valid response message.

The handler then attempts to call Recv.

Client-Side:

Make a client-streaming RPC call.

Continuously send messages.

Expected Outcome:

The server's Recv call should return an error indicating that the stream is closed.

The client's Send calls should eventually return an error indicating that the stream has been reset.

This looks like the test I have written already , the TestClientStreamingRecvAfterCloseError test , can you tell me what is the difference between the two?

purnesh42H · 2025-03-07T06:42:30Z

Looks like we need one more test case when "Server Doesn't RST_STREAM"
Server-Side:

Create a client-streaming RPC handler.

The handler calls SendAndClose with a valid response message.

The handler then attempts to call Recv.

Client-Side:

Make a client-streaming RPC call.

Continuously send messages.

Expected Outcome:

The server's Recv call should return an error indicating that the stream is closed.

The client's Send calls should eventually return an error indicating that the stream has been reset.

This looks like the test I have written already , the TestClientStreamingRecvAfterCloseError test , can you tell me what is the difference between the two?

yeah looks close. I think the for loop is not clear. We should do t.LogF when receiving error from client.Send indicating rst_stream. Is it possible to verify if rst stream was received in trailers. If its too complicated we can skip but let's see if we can. Other thing is the for loop shouldn't be indefinite. We should also do t.Fatalf if context is done. That way its clear that we are expecting an error for this test.

Having said that, then I think then we need a separate test case where client ignores the error after SendAndClose. It will slightly shorter where the handler return a static error and client just ignores it and status is ok.

eshitachandwani added 4 commits February 17, 2025 13:09

rough tests

a38ba9a

add tests

68d25fe

skipped tests

2811ac5

remove

d551123

eshitachandwani added the Type: Testing label Feb 25, 2025

eshitachandwani added this to the 1.72 Release milestone Feb 25, 2025

eshitachandwani requested a review from dfawley February 25, 2025 09:59

eshitachandwani assigned dfawley Feb 25, 2025

arjan-bal reviewed Feb 25, 2025

View reviewed changes

test/end2end_test.go Show resolved Hide resolved

remove

6eaf44d

dfawley reviewed Feb 27, 2025

View reviewed changes

dfawley assigned eshitachandwani and unassigned dfawley Feb 27, 2025

eshitachandwani added 2 commits March 3, 2025 12:10

use stub server

988dad9

remove unused var

40abccd

eshitachandwani requested a review from purnesh42H March 3, 2025 07:42

eshitachandwani assigned purnesh42H Mar 3, 2025

change

ad7ff22

purnesh42H reviewed Mar 4, 2025

View reviewed changes

purnesh42H removed their assignment Mar 4, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

grpc_test: add tests for cardinality violation #8120

grpc_test: add tests for cardinality violation #8120

eshitachandwani commented Feb 25, 2025

codecov bot commented Feb 25, 2025 •

edited

Loading

dfawley Feb 27, 2025

purnesh42H left a comment

purnesh42H Mar 4, 2025

purnesh42H Mar 4, 2025

purnesh42H Mar 4, 2025

purnesh42H Mar 4, 2025

purnesh42H Mar 4, 2025

purnesh42H Mar 4, 2025

purnesh42H Mar 4, 2025

purnesh42H Mar 4, 2025

purnesh42H Mar 4, 2025

purnesh42H Mar 4, 2025

purnesh42H Mar 4, 2025

eshitachandwani commented Mar 6, 2025

purnesh42H commented Mar 7, 2025

		earlyNil bool // whether to return nil without calling SendAndClose().
		recvAfterClose bool // whether to call Recv() after calling SendAndClose().

grpc_test: add tests for cardinality violation #8120

Are you sure you want to change the base?

grpc_test: add tests for cardinality violation #8120

Conversation

eshitachandwani commented Feb 25, 2025

codecov bot commented Feb 25, 2025 • edited Loading

Codecov Report

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eshitachandwani commented Mar 6, 2025

purnesh42H commented Mar 7, 2025

codecov bot commented Feb 25, 2025 •

edited

Loading