From a5994d80e204e4a85e78b4cf76e8ea3950ae81c3 Mon Sep 17 00:00:00 2001 From: Maxim Sukharev Date: Fri, 5 Apr 2019 13:13:26 +0200 Subject: [PATCH 1/3] remove TestServerCancel test It doesn't test our code and makes an incorrect assumption about how grpc works. The logic is: - create a response of N items - create DataServerHandler which implements grpc interface for an iterator - create grpc Server (vendored code) - create grpc DataClient client (vendored code) - calls grpc method on DataClient (vendored code) with cancellable context - read [0, N-1] items from the response stream - send cancel - check that client read returns any error containing "context cancel" The actual error it receives is "statusError" coming from grpc vendored code. Context gets propagated to the stream and in the stream constructor it runs goroutine which finishes the stream on context cancel. When the stream is finished it just returns the reason error for all calls. So it doesn't matter what server is used and if it handles cancellation or not. It also makes an incorrect assumption that after `cancel` next call is guaranteed to return an error. As explained above handling of context cancelation happens in goroutine, so scheduler *can* run `Recv` before `stream.finish` which we see as flaky test behavior on CI. I believe the original reason for this test was to test `DataServerHandler` code and one can actually see test coverage decrease after removing this test *sometimes*. It happens because of the side effects: - Server sometimes hits an error while writing into the stream using `Send` method. It can be `context canceled` or `transport is closing` on my machine. - It also hits <-cancel path but not very often Though the code doesn't check any of them. Ref: #519 Signed-off-by: Maxim Sukharev --- data_test.go | 93 ---------------------------------------------------- 1 file changed, 93 deletions(-) diff --git a/data_test.go b/data_test.go index b034c4e2..5a976ef6 100644 --- a/data_test.go +++ b/data_test.go @@ -122,99 +122,6 @@ func TestServerGetFilesOk(t *testing.T) { } } -func TestServerCancel(t *testing.T) { - for i := 0; i <= 10; i++ { - for j := 0; j < i; j++ { - revision := &ReferencePointer{ - InternalRepositoryURL: "repo", - Hash: "5262fd2b59d10e335a5c941140df16950958322d", - } - changesReq := &ChangesRequest{Head: revision} - filesReq := &FilesRequest{Revision: revision} - changes := generateChanges(i) - files := generateFiles(i) - changeTick := make(chan struct{}, 1) - fileTick := make(chan struct{}, 1) - dr := &MockService{ - T: t, - ExpectedCRequest: changesReq, - ExpectedFRequest: filesReq, - ChangeScanner: &SliceChangeScanner{ - Changes: changes, - ChangeTick: changeTick, - }, - FileScanner: &SliceFileScanner{ - Files: files, - FileTick: fileTick, - }, - } - srv, client := setupDataServer(t, dr) - - t.Run(fmt.Sprintf("get-changes-size-%d-cancel-at-%d", i, j), - func(t *testing.T) { - require := require.New(t) - - ctx, cancel := context.WithCancel(context.Background()) - respClient, err := client.GetChanges(ctx, changesReq) - require.NoError(err) - require.NotNil(respClient) - require.NoError(respClient.CloseSend()) - - for idx, change := range changes { - if idx >= j { - break - } - - changeTick <- struct{}{} - actualResp, err := respClient.Recv() - require.NoError(err) - require.Equal(change, actualResp) - } - - cancel() - changeTick <- struct{}{} - actualResp, err := respClient.Recv() - require.Error(err) - require.Contains(err.Error(), "context cancel") - require.Zero(actualResp) - }) - - t.Run(fmt.Sprintf("get-files-size-%d-cancel-at-%d", i, j), - func(t *testing.T) { - require := require.New(t) - - ctx, cancel := context.WithCancel(context.Background()) - respClient, err := client.GetFiles(ctx, filesReq) - require.NoError(err) - require.NotNil(respClient) - require.NoError(respClient.CloseSend()) - - for idx, file := range files { - if idx >= j { - break - } - - fileTick <- struct{}{} - actualResp, err := respClient.Recv() - require.NoError(err) - require.Equal(file, actualResp) - } - - cancel() - fileTick <- struct{}{} - actualResp, err := respClient.Recv() - require.Error(err) - require.Contains(err.Error(), "context cancel") - require.Zero(actualResp) - }) - - close(changeTick) - close(fileTick) - tearDownDataServer(t, srv) - } - } -} - func TestServerGetChangesError(t *testing.T) { req := &ChangesRequest{ Head: &ReferencePointer{ From 1deed8437c8288a652df8527640472c9d43267ee Mon Sep 17 00:00:00 2001 From: Maxim Sukharev Date: Fri, 5 Apr 2019 14:01:45 +0200 Subject: [PATCH 2/3] add TestDataServerHandlerCancel Signed-off-by: Maxim Sukharev --- data_test.go | 84 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 84 insertions(+) diff --git a/data_test.go b/data_test.go index 5a976ef6..56d66e18 100644 --- a/data_test.go +++ b/data_test.go @@ -11,6 +11,7 @@ import ( "github.com/stretchr/testify/require" "google.golang.org/grpc" + "google.golang.org/grpc/metadata" "gopkg.in/src-d/lookout-sdk.v0/pb" ) @@ -122,6 +123,89 @@ func TestServerGetFilesOk(t *testing.T) { } } +func TestDataServerHandlerCancel(t *testing.T) { + require := require.New(t) + + revision := &ReferencePointer{ + InternalRepositoryURL: "repo", + Hash: "5262fd2b59d10e335a5c941140df16950958322d", + } + changesReq := &ChangesRequest{Head: revision} + filesReq := &FilesRequest{Revision: revision} + changes := generateChanges(1) + files := generateFiles(1) + changeTick := make(chan struct{}, 1) + fileTick := make(chan struct{}, 1) + dr := &MockService{ + T: t, + ExpectedCRequest: changesReq, + ExpectedFRequest: filesReq, + ChangeScanner: &SliceChangeScanner{ + Changes: changes, + ChangeTick: changeTick, + }, + FileScanner: &SliceFileScanner{ + Files: files, + FileTick: fileTick, + }, + } + h := &DataServerHandler{ChangeGetter: dr, FileGetter: dr} + + // create cancelled context + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + // MockService doesn't respect context cancellation + // which means only DataServerHandler would handle it + changesSrv := &mockDataGetChangesServer{&mockServerStream{ctx}} + err := h.GetChanges(changesReq, changesSrv) + require.EqualError(err, "request canceled: context canceled") + + filesSrv := &mockDataGetFilesServer{&mockServerStream{ctx}} + err = h.GetFiles(filesReq, filesSrv) + require.EqualError(err, "request canceled: context canceled") +} + +type mockDataGetChangesServer struct { + *mockServerStream +} + +// Send implements pb.Data_GetChangesServer +func (s *mockDataGetChangesServer) Send(*pb.Change) error { + return nil +} + +type mockDataGetFilesServer struct { + *mockServerStream +} + +// Send implements pb.Data_GetFilesServer +func (s *mockDataGetFilesServer) Send(*pb.File) error { + return nil +} + +// Implements only `Context() context.Context` method to be able to test client cancellation +type mockServerStream struct { + ctx context.Context +} + +func (s *mockServerStream) SetHeader(metadata.MD) error { + return nil +} +func (s *mockServerStream) SendHeader(metadata.MD) error { + return nil +} +func (s *mockServerStream) SetTrailer(metadata.MD) {} +func (s *mockServerStream) Context() context.Context { + return s.ctx +} +func (s *mockServerStream) SendMsg(m interface{}) error { + return nil +} +func (s *mockServerStream) RecvMsg(m interface{}) error { + return nil +} + func TestServerGetChangesError(t *testing.T) { req := &ChangesRequest{ Head: &ReferencePointer{ From 23232b478d45cf396693231d4d3fa741c872b161 Mon Sep 17 00:00:00 2001 From: Maxim Sukharev Date: Fri, 5 Apr 2019 15:24:57 +0200 Subject: [PATCH 3/3] add TestDataServerHandlerSendError Signed-off-by: Maxim Sukharev --- data_test.go | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 58 insertions(+), 2 deletions(-) diff --git a/data_test.go b/data_test.go index 56d66e18..c3660d9a 100644 --- a/data_test.go +++ b/data_test.go @@ -157,30 +157,86 @@ func TestDataServerHandlerCancel(t *testing.T) { // MockService doesn't respect context cancellation // which means only DataServerHandler would handle it - changesSrv := &mockDataGetChangesServer{&mockServerStream{ctx}} + changesSrv := &mockDataGetChangesServer{ + mockServerStream: &mockServerStream{ctx}} err := h.GetChanges(changesReq, changesSrv) require.EqualError(err, "request canceled: context canceled") - filesSrv := &mockDataGetFilesServer{&mockServerStream{ctx}} + filesSrv := &mockDataGetFilesServer{ + mockServerStream: &mockServerStream{ctx}} err = h.GetFiles(filesReq, filesSrv) require.EqualError(err, "request canceled: context canceled") } +func TestDataServerHandlerSendError(t *testing.T) { + require := require.New(t) + + revision := &ReferencePointer{ + InternalRepositoryURL: "repo", + Hash: "5262fd2b59d10e335a5c941140df16950958322d", + } + changesReq := &ChangesRequest{Head: revision} + filesReq := &FilesRequest{Revision: revision} + changes := generateChanges(1) + files := generateFiles(1) + changeTick := make(chan struct{}, 1) + fileTick := make(chan struct{}, 1) + dr := &MockService{ + T: t, + ExpectedCRequest: changesReq, + ExpectedFRequest: filesReq, + ChangeScanner: &SliceChangeScanner{ + Changes: changes, + ChangeTick: changeTick, + }, + FileScanner: &SliceFileScanner{ + Files: files, + FileTick: fileTick, + }, + } + h := &DataServerHandler{ChangeGetter: dr, FileGetter: dr} + + changesSrv := &mockDataGetChangesServer{ + sendReturnErr: true, + mockServerStream: &mockServerStream{context.Background()}, + } + changeTick <- struct{}{} + err := h.GetChanges(changesReq, changesSrv) + require.EqualError(err, "send error") + + filesSrv := &mockDataGetFilesServer{ + sendReturnErr: true, + mockServerStream: &mockServerStream{context.Background()}} + fileTick <- struct{}{} + err = h.GetFiles(filesReq, filesSrv) + require.EqualError(err, "send error") +} + type mockDataGetChangesServer struct { + sendReturnErr bool *mockServerStream } // Send implements pb.Data_GetChangesServer func (s *mockDataGetChangesServer) Send(*pb.Change) error { + if s.sendReturnErr { + return fmt.Errorf("send error") + } + return nil } type mockDataGetFilesServer struct { + sendReturnErr bool *mockServerStream } // Send implements pb.Data_GetFilesServer func (s *mockDataGetFilesServer) Send(*pb.File) error { + if s.sendReturnErr { + return fmt.Errorf("send error") + } + return nil }