test/gracefulstop: use stubserver instead of testservice implementation #7907
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #7907 +/- ##
==========================================
+ Coverage 82.42% 82.47% +0.04%
==========================================
Files 387 387
Lines 39042 39065 +23
==========================================
+ Hits 32179 32217 +38
+ Misses 5546 5530 -16
- Partials 1317 1318 +1 |
purnesh42H
changed the title
test/gracefulstop_test.go
Outdated
| dialed bool | ||
| closed bool |
There was a problem hiding this comment.
@pvsravani where and how are we using closed. Why is it needed when switching to stub server?
There was a problem hiding this comment.
The closed field ensures the close() method is idempotent. It prevents multiple calls to close() from closing the listener again, which could cause errors.
|
@easwars for second review |
test/gracefulstop_test.go
Outdated
| @@ -113,31 +113,27 @@ func (s) TestGracefulStop(t *testing.T) { | |||
| d := func(ctx context.Context, _ string) (net.Conn, error) { return dlis.Dial(ctx) } | |||
There was a problem hiding this comment.
While you are here, could you please rename this d as dialer and move it a line right before it is used. Currently it is only used on line 153, when it is passed to grpc.WithContextDialer in grpc.DialContext.
|
@easwars gentle ping on this |
|
|
||
| // 1. Start Server |
There was a problem hiding this comment.
don't revert the step number
| wg.Done() | ||
| }() | ||
|
|
||
| // 2. GracefulStop() Server after listener's Accept is called, but don't |
There was a problem hiding this comment.
don't revert the step number
test/gracefulstop_test.go
Outdated
| }() | ||
|
|
||
| // 2. GracefulStop() Server after listener's Accept is called, but don't | ||
| // GracefulStop() Server after listener's Accept is called, but don't |
There was a problem hiding this comment.
don't revert the step number
| }() | ||
|
|
||
| // 3. Create a new connection to the server after listener.Close() is called. |
There was a problem hiding this comment.
don't revert the step number
test/gracefulstop_test.go
Outdated
| if err != nil { | ||
| t.Fatalf("grpc.DialContext(_, %q, _) = %v", lis.Addr().String(), err) | ||
| } | ||
| client := testgrpc.NewTestServiceClient(cc) | ||
| defer cc.Close() | ||
|
|
||
| // 4. Send an RPC on the new connection. | ||
| // Send an RPC on the new connection. |
There was a problem hiding this comment.
don't revert the step number
test/gracefulstop_test.go
Outdated
|
|
||
| // 2. GracefulStop() Server after listener's Accept is called, but don't | ||
| // allow Accept() to exit when Close() is called on it. | ||
| //2. Call GracefulStop from a goroutine. It will trigger Close on the listener |
There was a problem hiding this comment.
Please leave a space after the // and before the first character of the comment.
Also, please ensure that comment lines are wrapped at 80-cols wide. See: https://google.github.io/styleguide/go/decisions#comment-line-length
test/gracefulstop_test.go
Outdated
|
|
||
| <-dlis.closeCalled // Block until GracefulStop calls dlis.Close() | ||
|
|
||
| // Now dial. The listener's Accept method will return a valid connection, | ||
| // even though GracefulStop has closed the listener. | ||
| //Dial the server. This will cause a connection to be accepted. This will also unblock the Close method. |
There was a problem hiding this comment.
Same here. Ensure space after the // and ensure wrapping at 80-cols.
|
@purnesh42H : Please feel free to merge once my last two comments have been addressed. |
test/gracefulstop_test.go
Outdated
| } | ||
| s := grpc.NewServer() | ||
| testgrpc.RegisterTestServiceServer(s, ss) | ||
| // 1.Start Server and start serving by calling Serve(). |
There was a problem hiding this comment.
@pvsravani. You missed space here. Please recheck all comments
Partially Addresses: #7291
RELEASE NOTES: None