-
Notifications
You must be signed in to change notification settings - Fork 175
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
Github-issue#1048 : s3-sink integration test implementation. #2624
Github-issue#1048 : s3-sink integration test implementation. #2624
Conversation
Signed-off-by: Deepak Sahu <deepak.sahu562@gmail.com>
Codecov Report
@@ Coverage Diff @@
## main #2624 +/- ##
============================================
+ Coverage 93.56% 93.61% +0.04%
- Complexity 2253 2255 +2
============================================
Files 262 262
Lines 6308 6308
Branches 521 521
============================================
+ Hits 5902 5905 +3
+ Misses 268 266 -2
+ Partials 138 137 -1 |
S3SinkService s3SinkService = createObjectUnderTest(); | ||
s3SinkService.output(setEventQueue()); | ||
int s3ObjectCountAfterIngest = gets3ObjectCount(); | ||
assertThat(s3ObjectCountAfterIngest, greaterThan(s3ObjectCountBeforeIngest)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks unnecessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved.
Signed-off-by: Deepak Sahu <deepak.sahu562@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
s3ObjectCount = objects.size(); | ||
} catch (S3Exception e) { | ||
LOG.error(e.awsErrorDetails().errorMessage()); | ||
System.exit(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to fail the whole test suite and will prevent accurate reporting of failures.
Just let the exception be thrown here. Remove the try-catch block entirely. JUnit will handle it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the try-catch block.
s3SinkService.output(setEventQueue()); | ||
int s3ObjectCountAfterIngest = gets3ObjectCount(); | ||
assertThat(s3ObjectCountAfterIngest, equalTo(s3ObjectCountBeforeIngest + 1)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test should actually load the S3 object and verify the data is correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved.
Added two test-cases.
- verify_flushed_object_count_into_s3_bucket
- verify_flushed_records_into_s3_bucket
Signed-off-by: Deepak Sahu <deepak.sahu562@gmail.com>
24dc6c9
@ashoktelukuntla \ @dlvenable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
Description
Issues Resolved
GitHub-issue #1048
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.