add test case and improved coverage for datasource/file/s3 #2441
Conversation
…r into EN/test-datasource-file-s3
| ) | ||
|
|
||
| var ( | ||
| ErrGetObject = errors.New("failed to get object from S3") |
There was a problem hiding this comment.
Why are these errors exported? If not used outside of the package can we please unexport them?
|
|
||
| err := f.Close() | ||
|
|
||
| if tc.expected == nil { |
There was a problem hiding this comment.
We should not have if-else conditions inside test methods, if needed you can break the method into two separate methods (ex: one for success cases and other for error).
| } | ||
| } | ||
|
|
||
| var errS3Test = errors.New("s3 error") |
There was a problem hiding this comment.
It should also be declared along with other error vars at the top.
| require.NoError(t, err, "ReadAll() unexpected error") | ||
| require.NotNil(t, reader, "ReadAll() returned nil reader on success") | ||
|
|
||
| switch expectedType { |
There was a problem hiding this comment.
Switch cases in test methods are not a very good practice. Please break the tests if needed and remove the switch case.
| tt.setupMocks() | ||
| _, err := fs.Create(tt.createPath) | ||
|
|
||
| if tt.expectError { |
There was a problem hiding this comment.
Please remove this if-else block from this test method too.
| f := S3File{ | ||
| logger: mockLogger, | ||
| metrics: mockMetrics, | ||
| conn: mockS3, | ||
| } | ||
|
|
||
| fs := &FileSystem{ | ||
| s3File: f, | ||
| conn: mockS3, | ||
| logger: mockLogger, | ||
| config: config, | ||
| metrics: mockMetrics, | ||
| } |
There was a problem hiding this comment.
This code is being repeated 11 times inside the file can we please refactor and reuse it to avoid code duplication.
| // Execute the test | ||
| _, err := fs.Open(tt.fileName) | ||
|
|
||
| if tt.expectError { |
There was a problem hiding this comment.
No if else inside test methods please
| // Create a mock S3 client | ||
| mockS3 := NewMocks3Client(ctrl) | ||
| mockLogger := NewMockLogger(ctrl) | ||
| mockMetrics := NewMockMetrics(ctrl) |
There was a problem hiding this comment.
I saw these three initializations 7 times in the file, let's try and reuse it from a common function.
2 participants
Pull Request Template
Description:
Additional Information:
Tests use mock objects for S3 client
Testify assertions used for better test readability
Helper functions created for consistent test setup
Checklist:
Thank you for your contribution!