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
Fix #3728 AWS S3 integration test should remove all objects in finall… #3729
Conversation
.body(is(blobContent)); | ||
} finally { | ||
// Delete | ||
deleteObject(oid); |
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.
Should be done in an AfterEach method instead of a finally block
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.
The oid is a random generated id and there are some other tests such as copyObjectDeleteBucket
which has a different test logic. There's also a @AfterEach
method to check the objects leaking.
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.
@essobedo thanks for the review!
We were hit by high AWS bills in the past when tests were either missing cleanup code or when something failed midair and the cleanup code was not invoked. Using try-catch helps to address both: missing cleanup code is easier to spot and existing cleanup code is harder to skip. Thus +1 for this change from my side.
…y block