Use ESBlobStoreRepositoryIntegTestCase to test the repository-s3 plugin #29315
Conversation
tlrx
added
>test
|
Pinging @elastic/es-distributed |
There was a problem hiding this comment.
I think that it's simpler and nicer in MockAmazonS3 to just override the methods that we want to implement (just as we used to before this PR) instead of writing all these when(xyz).then(abc) conditions. I also would prefer not to have this static map within MockAmazonS3, but leave it up to the caller to decide whether to share instances or not. Maybe the map (= storage) could even be passed in when creating the object. @tlrx WDYT ?
|
Thanks for your review @ywelsch. I understand your concern about the mocked methods (even though I like them:)) and I updated the code. I also fixed some corner cases in the way the methods were implemented. Can you have another look please? |
| throw new AmazonS3Exception("[" + blobName + "] already exists."); | ||
| public PutObjectResult putObject(final PutObjectRequest request) throws AmazonClientException { | ||
| assertThat(request.getBucketName(), equalTo(bucket)); | ||
| assertThat(request.getBucketName(), equalTo(bucket)); |
There was a problem hiding this comment.
don't drink and code 🍻 (same line twice)
There was a problem hiding this comment.
I never drink while coding, otherwise it would be even worse :)
|
|
||
| final List<DeleteObjectsResult.DeletedObject> deletions = new ArrayList<>(); | ||
| for (DeleteObjectsRequest.KeyVersion key : request.getKeys()) { | ||
| if(blobs.remove(key.getKey()) == null){ |
There was a problem hiding this comment.
spaces missing after if and before { 🗡
|
|
||
| public class S3BlobStoreRepositoryTests extends ESBlobStoreRepositoryIntegTestCase { | ||
|
|
||
| private static final ConcurrentMap<String, byte[]> blobs = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
this should be reset after each test (and in particular cleaned up at the end as well). I hate statics but don't see another way to achieve this right now.
There was a problem hiding this comment.
Of course, thanks for spotting it.
The test framework contains a base class for testing blob store repository implementations, but the S3 plugin does not use it. This commit adds the S3BlobStoreRepositoryTests class that extends the base testing class for S3. It also cleans up the S3BlobStoreTests and S3BlobStoreContainerTests so that they are now based on pure mock S3 clients. It also removes some usage of socket servers that emulate socket connections in unit tests. It was added to trigger security exceptions, but this won't be needed anymore once elastic#29296 will be merged. closes elastic#16472
|
Thanks @ywelsch ! |
…in (#29315) This commit adds the S3BlobStoreRepositoryTests class that extends the base testing class for S3. It also removes some usage of socket servers that emulate socket connections in unit tests. It was added to trigger security exceptions, but this won't be needed anymore since #29296 is merged.
|
This has been backported to 6.x in 5c3a40b |
* elastic/6.x: (50 commits) Painless: Restructure/Clean Up of Spec Documentation (#31013) Add support for ignore_unmapped to geo sort (#31153) Enable engine factory to be pluggable (#31183) Remove vestiges of animal sniffer (#31178) Rename elasticsearch-core to core (#31185) Move cli sub-project out of server to libs (#31184) QA: Fixup rolling restart tests QA: Better seed nodes for rolling restart [DOCS] Fixes broken link in release notes [DOCS] Fixes broken link in auditing settings [DOCS] Moves ML content to stack-docs [DOCS] Clarifies recommendation for audit index output type (#31146) QA: Set better node names on rolling restart tests QA: Skip mysterious failing rolling upgrade tests Share common parser in some AcknowledgedResponses (#31169) Fix random failure on SearchQueryIT#testTermExpansionExceptionOnSpanFailure Remove reference to multiple fields with one name (#31127) Remove BlobContainer.move() method (#31100) [Docs] Correct minor typos in templates.asciidoc (#31167) Use ESBlobStoreRepositoryIntegTestCase to test the repository-s3 plugin (#29315) ...
The test framework contains three base classes for testing blob store repository implementations, but the S3 plugin does not use all of them.
This commit adds the
S3BlobStoreRepositoryTestsandS3BlobStoreTestsclasses that extends the base testing classes for S3. It also cleans up theS3BlobStoreTestsandS3BlobStoreContainerTestsso that they are now based on pure mock S3 clients.It also removes some usage of socket servers that emulate socket connections in unit tests. It was added to trigger security exceptions, but this won't be needed anymore once #29296 will be merged.
closes #16472