From 9bea6d63b965bd3eeda19e27898c160eca5618b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C4=90urica=20Yuri=20Nikoli=C4=87?= Date: Wed, 25 Oct 2023 18:56:48 +0200 Subject: [PATCH] Improve distributor_test (#6479) * Fix distributor_test error bug Signed-off-by: Yuri Nikolic * Fixing review findings Co-authored-by: Nick Pillitteri <56quarters@users.noreply.github.com> --------- Signed-off-by: Yuri Nikolic Co-authored-by: Nick Pillitteri <56quarters@users.noreply.github.com> --- pkg/distributor/distributor_test.go | 36 +++++------------------------ pkg/distributor/push_test.go | 5 ++++ 2 files changed, 11 insertions(+), 30 deletions(-) diff --git a/pkg/distributor/distributor_test.go b/pkg/distributor/distributor_test.go index 415f5bfdaee..406a0327e59 100644 --- a/pkg/distributor/distributor_test.go +++ b/pkg/distributor/distributor_test.go @@ -4785,8 +4785,6 @@ func TestSeriesAreShardedToCorrectIngesters(t *testing.T) { func TestHandleIngesterPushError(t *testing.T) { testErrorMsg := "this is a test error message" outputErrorMsgPrefix := "failed pushing to ingester" - userID := "test" - errWithUserID := fmt.Errorf("user=%s: %s", userID, testErrorMsg) // Ensure that no error gets translated into no error. t.Run("no error gives no error", func(t *testing.T) { @@ -4826,7 +4824,7 @@ func TestHandleIngesterPushError(t *testing.T) { }) } - // Ensure that the errors created by gogo/status package get translated + // Ensure that the errors created by gogo/status package get translated // into ingesterPushError messages. statusTests := map[string]struct { ingesterPushError error @@ -4836,6 +4834,11 @@ func TestHandleIngesterPushError(t *testing.T) { ingesterPushError: createStatusWithDetails(t, codes.Unavailable, testErrorMsg, mimirpb.INVALID).Err(), expectedIngesterPushError: newIngesterPushError(createStatusWithDetails(t, codes.Unavailable, testErrorMsg, mimirpb.INVALID)), }, + "a DeadlineExceeded gRPC ingester error an ingesterPushError with INVALID cause": { + // This is how context.DeadlineExceeded error is translated into a gRPC error. + ingesterPushError: status.Error(codes.DeadlineExceeded, context.DeadlineExceeded.Error()), + expectedIngesterPushError: newIngesterPushError(createStatusWithDetails(t, codes.Unavailable, context.DeadlineExceeded.Error(), mimirpb.INVALID)), + }, "an Unavailable gRPC error without details gives an ingesterPushError with INVALID cause": { ingesterPushError: status.Error(codes.Unavailable, testErrorMsg), expectedIngesterPushError: newIngesterPushError(createStatusWithDetails(t, codes.Unavailable, testErrorMsg, mimirpb.INVALID)), @@ -4859,33 +4862,6 @@ func TestHandleIngesterPushError(t *testing.T) { require.Equal(t, testData.expectedIngesterPushError.errorCause(), ingesterPushErr.errorCause()) }) } - - // Ensure that the errors with no status (e.g., context.Canceled, - // context.DeadLine, etc) are correctly wrapped. - otherTests := map[string]struct { - ingesterPushError error - expectedError error - }{ - "a random ingester error without status gives the same wrapped error": { - ingesterPushError: errWithUserID, - expectedError: errors.Wrap(errWithUserID, outputErrorMsgPrefix), - }, - "a context canceled error gives the same wrapped error": { - ingesterPushError: context.Canceled, - expectedError: errors.Wrap(errWithUserID, outputErrorMsgPrefix), - }, - "a context deadline exceeded error gives the same wrapped error": { - ingesterPushError: context.DeadlineExceeded, - expectedError: errors.Wrap(errWithUserID, outputErrorMsgPrefix), - }, - } - for testName, testData := range otherTests { - t.Run(testName, func(t *testing.T) { - err := handleIngesterPushError(testData.ingesterPushError) - require.Errorf(t, err, testData.expectedError.Error()) - require.ErrorIs(t, err, testData.ingesterPushError) - }) - } } func TestHandlePushError(t *testing.T) { diff --git a/pkg/distributor/push_test.go b/pkg/distributor/push_test.go index fbc9a3d9335..92a8f235d8f 100644 --- a/pkg/distributor/push_test.go +++ b/pkg/distributor/push_test.go @@ -944,6 +944,11 @@ func TestHandler_ToHTTPStatus(t *testing.T) { expectedHTTPStatus: http.StatusInternalServerError, expectedErrorMsg: fmt.Sprintf("%s: %s", failedPushingToIngesterMessage, originalMsg), }, + "an ingesterPushError obtained from a DeadlineExceeded coming from the ingester gets translated into an HTTP 500": { + err: newIngesterPushError(createStatusWithDetails(t, codes.Internal, context.DeadlineExceeded.Error(), mimirpb.INVALID)), + expectedHTTPStatus: http.StatusInternalServerError, + expectedErrorMsg: fmt.Sprintf("%s: %s", failedPushingToIngesterMessage, context.DeadlineExceeded), + }, } for name, tc := range testCases { t.Run(name, func(t *testing.T) {