Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding UTs for negative cases in MRD #3010

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 56 additions & 0 deletions internal/gcsx/multi_range_downloader_wrapper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,11 @@ func (t *mrdWrapperTest) Test_Read() {
start: 10,
end: 10 + int(t.object.Size)/2,
},
{
name: "ReadEmpty",
start: 10,
end: 10,
},
}

for _, tc := range testCases {
Expand All @@ -160,6 +165,57 @@ func (t *mrdWrapperTest) Test_Read() {
}
}

func (t *mrdWrapperTest) Test_Read_ErrorHandling() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other cases are when callback returns EOF and non EOF errors.

testCases := []struct {
name string
start int
end int
expectedErrKeyword string
}{
{
name: "ErrorInCreatingMRD",
start: 0,
end: 100,
expectedErrKeyword: "MultiRangeDownloader",
},
{
name: "TimeoutError",
start: 0,
end: 100,
expectedErrKeyword: "Timeout",
},
{
name: "ContextCancelled",
start: 0,
end: 100,
expectedErrKeyword: "Context Cancelled",
},
}

for _, tc := range testCases {
t.Run(tc.name, func() {
ctx := context.Background()
buf := make([]byte, tc.end-tc.start)
t.mrdWrapper.Wrapped = nil
if tc.name == "ErrorInCreatingMRD" {
t.mockBucket.On("NewMultiRangeDownloader", mock.Anything, mock.Anything).Return(nil, fmt.Errorf("Error in creating MRD")).Once()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please extract these also into seperate test methods. Its okay to duplicate code in test methods for readability.

} else if tc.name == "TimeoutError" {
t.mockBucket.On("NewMultiRangeDownloader", mock.Anything, mock.Anything).Return(fake.NewFakeMultiRangeDownloaderWithSleep(t.object, t.objectData, t.mrdTimeout+time.Millisecond), nil).Once()
} else if tc.name == "ContextCancelled" {
var cancel context.CancelFunc
ctx, cancel = context.WithCancel(context.Background())
cancel()
t.mockBucket.On("NewMultiRangeDownloader", mock.Anything, mock.Anything).Return(fake.NewFakeMultiRangeDownloaderWithSleep(t.object, t.objectData, time.Microsecond), nil).Once()
}

bytesRead, err := t.mrdWrapper.Read(ctx, buf, int64(tc.start), int64(tc.end), t.mrdTimeout, common.NewNoopMetrics())

assert.ErrorContains(t.T(), err, tc.expectedErrKeyword)
assert.Equal(t.T(), 0, bytesRead)
})
}
}

func (t *mrdWrapperTest) Test_NewMultiRangeDownloaderWrapper() {
testCases := []struct {
name string
Expand Down
50 changes: 31 additions & 19 deletions internal/gcsx/random_reader_stretchr_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -805,47 +805,59 @@ func (t *RandomReaderStretchrTest) Test_ReadFromMultiRangeReader_ReadChunk() {
}
}

func (t *RandomReaderStretchrTest) Test_ReadFromMultiRangeReader_ValidateTimeout() {
func (t *RandomReaderStretchrTest) Test_ReadFromMultiRangeReader_ErrorHandling() {
testCases := []struct {
name string
dataSize int
sleepTime time.Duration
name string
dataSize int
sleepTime time.Duration
expectedErrKeyword string
}{
{
name: "TimeoutPlusOneMilliSecond",
dataSize: 100,
sleepTime: TestTimeoutForMultiRangeRead + time.Millisecond,
name: "NilMRDWrapper",
dataSize: 100,
sleepTime: 0,
expectedErrKeyword: "MultiRangeDownloaderWrapper",
},
// Ensure that this is always the last test
{
name: "TimeoutValue",
dataSize: 100,
sleepTime: TestTimeoutForMultiRangeRead,
name: "TimeoutPlusOneMilliSecond",
dataSize: 100,
sleepTime: TestTimeoutForMultiRangeRead + time.Millisecond,
expectedErrKeyword: "Timeout",
},
{
name: "TimeoutValue",
dataSize: 100,
sleepTime: TestTimeoutForMultiRangeRead,
expectedErrKeyword: "Timeout",
},
}

for i, tc := range testCases {
for _, tc := range testCases {
t.Run(tc.name, func() {
t.rr.wrapped.reader = nil
t.rr.wrapped.isMRDInUse = false
t.object.Size = uint64(tc.dataSize)
testContent := testutil.GenerateRandomBytes(int(t.object.Size))
fakeMRDWrapper, err := NewMultiRangeDownloaderWrapperWithClock(t.mockBucket, t.object, &clock.FakeClock{})
assert.Nil(t.T(), err, "Error in creating MRDWrapper")
t.rr.wrapped.mrdWrapper = &fakeMRDWrapper
t.mockBucket.On("NewMultiRangeDownloader", mock.Anything, mock.Anything).Return(fake.NewFakeMultiRangeDownloaderWithSleep(t.object, testContent, tc.sleepTime)).Times(1)
t.mockBucket.On("BucketType", mock.Anything).Return(gcs.BucketType{Zonal: true}).Times(1)
if tc.name == "NilMRDWrapper" {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its preferred to extract testcases in seperate methods if you need if-else. It becomes difficult to maintain and extend in future.
For eg, NilMRDWrapper doesn't require lot of this code. can you move it as seperate test method..

t.rr.wrapped.mrdWrapper = nil
} else {
fakeMRDWrapper, err := NewMultiRangeDownloaderWrapperWithClock(t.mockBucket, t.object, &clock.FakeClock{})
assert.Nil(t.T(), err, "Error in creating MRDWrapper")
t.rr.wrapped.mrdWrapper = &fakeMRDWrapper
t.mockBucket.On("NewMultiRangeDownloader", mock.Anything, mock.Anything).Return(fake.NewFakeMultiRangeDownloaderWithSleep(t.object, testContent, tc.sleepTime)).Once()
}
t.mockBucket.On("BucketType", mock.Anything).Return(gcs.BucketType{Zonal: true}).Once()
buf := make([]byte, tc.dataSize)

bytesRead, err := t.rr.wrapped.readFromMultiRangeReader(t.rr.ctx, buf, 0, int64(t.object.Size), TestTimeoutForMultiRangeRead)

if i == len(testCases)-1 && bytesRead != 0 {
if tc.name == "TimeoutValue" && bytesRead != 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only 1 test case matches this name. why do you need 2nd condition?

assert.NoError(t.T(), err)
assert.Equal(t.T(), tc.dataSize, bytesRead)
assert.Equal(t.T(), testContent[:tc.dataSize], buf[:bytesRead])
return
}
assert.ErrorContains(t.T(), err, "Timeout")
assert.ErrorContains(t.T(), err, tc.expectedErrKeyword)
})
}
}
Loading