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.04% 82.15% +0.10%
==========================================
Files 377 379 +2
Lines 38180 38261 +81
==========================================
+ Hits 31326 31432 +106
+ Misses 5551 5532 -19
+ Partials 1303 1297 -6 |
test/gracefulstop_test.go
Outdated
| @@ -122,7 +122,8 @@ func (s) TestGracefulStop(t *testing.T) { | |||
| }, | |||
| } | |||
| s := grpc.NewServer() | |||
| testgrpc.RegisterTestServiceServer(s, ss) | |||
| ss.S = s | |||
There was a problem hiding this comment.
i think you have to assign the listener as well when creating the stub server?
test/gracefulstop_test.go
Outdated
| @@ -122,7 +122,8 @@ func (s) TestGracefulStop(t *testing.T) { | |||
| }, | |||
| } | |||
| s := grpc.NewServer() | |||
| testgrpc.RegisterTestServiceServer(s, ss) | |||
| ss.S = s | |||
There was a problem hiding this comment.
nit: this can be just ss.S = grpc.NewServer() and all the usages can be updated as ss.S. like ss.S.stop() etc.
purnesh42H
changed the title
test/gracefulstop_test.go
Outdated
| @@ -42,7 +42,9 @@ type delayListener struct { | |||
| closeCalled chan struct{} | |||
| acceptCalled chan struct{} | |||
| allowCloseCh chan struct{} | |||
| mu sync.Mutex | |||
There was a problem hiding this comment.
it looks like we are introducing mutex only to update closed? Then it should be put right above it indicating that anything below is guarded by mutex.
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.
test/gracefulstop_test.go
Outdated
|
|
||
| // 1. Start Server | ||
| wg := sync.WaitGroup{} | ||
| wg.Add(1) | ||
| go func() { | ||
| s.Serve(dlis) | ||
| ss.S.Serve(dlis) |
There was a problem hiding this comment.
we should remove this as stubserver.StartTestService already starts the server
|
@easwars for second review |
| @@ -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.
| } | ||
| s := grpc.NewServer() | ||
| testgrpc.RegisterTestServiceServer(s, ss) | ||
| stubserver.StartTestService(t, ss) |
There was a problem hiding this comment.
I don't think this is doing the correct thing. What we want as part of this test is for GracefulStop to be called when Accept is in the middle of accepting a connection.
If you folks have convinced yourself that it actually does what it is supposed to do, then we don't need a sync.WaitGroup. The same can be accomplished with a done channel.
Partially Addresses: #7291
RELEASE NOTES: None