Use fixture to test repository-s3 plugin #29296

tlrx · 2018-03-29T15:54:08Z

This commit adds a new fixture that emulates a S3 service in order to improve the existing integration tests. This is very similar to what has been made for Google Cloud Storage in #28788, and such tests would have helped a lot to catch bugs like #22534.

The AmazonS3Fixture is a bit brittle and only implements the very necessary stuff for the S3 repository to work, but at least it works and can be adapted for specific tests needs.

This commit adds a new fixture that emulates a S3 service in order to improve the existing integration tests. This is very similar to what has been made for Google Cloud Storage in elastic#28788, and such tests would have helped a lot to catch bugs like elastic#22534. The AmazonS3Fixture is brittle and only implements the very necessary stuff for the S3 repository to work, but at least it works and can be adapted for specific tests needs.

nik9000 · 2018-03-29T19:03:45Z

plugins/repository-s3/src/test/java/org/elasticsearch/repositories/s3/AmazonS3Fixture.java

+ */
+public class AmazonS3Fixture {
+
+    @SuppressForbidden(reason = "PathUtils#get is fine - we don't have environment here")


Can you wrap that method call in a smaller method and just @SuppressForbidden on that? That way we still get forbidden checking on everything else. Just in case.

Actually, the reason is not exact. It suppresses warnings for both Paths.get() and the usages of com.sun.net.httpserver.HttpServer which is used multiple times in this method.

nik9000 · 2018-03-29T19:04:55Z

plugins/repository-s3/src/test/java/org/elasticsearch/repositories/s3/AmazonS3Fixture.java

+        }
+    }
+
+    @SuppressForbidden(reason = "Use a http server")


Are you suppressing the non-portable warnings here? I think you can change out the forbidden definitions so you don't have to do that.

Yes, it suppresses the warnings. I removed the @SuppressForbidden and I updated the build.gradle file to change the forbiddenApisTest's bundledSignatures, as we do for some other projects. Let me know what you think.

tlrx · 2018-03-30T08:02:08Z

@nik9000 Thanks for your review! I applied some changes.

nik9000

I haven't looked at the actual fixture implementation but the theory is sound and the build changes look fine to me. As does the new test to run.

nik9000 · 2018-03-30T20:10:34Z

plugins/repository-gcs/build.gradle

+forbiddenApisTest {
+  // we are using jdk-internal instead of jdk-non-portable to allow for com.sun.net.httpserver.* usage
+  bundledSignatures -= 'jdk-non-portable'
+  bundledSignatures += 'jdk-internal'


nik9000 · 2018-03-30T20:15:21Z

plugins/repository-s3/src/test/resources/rest-api-spec/test/repository_s3/10_basic.yml

+"Snapshot/Restore with repository-s3":
+  - skip:
+      version: " - 6.3.99"
+      reason: repository-s3 was not testable through YAML tests until 6.4.0


Is this because fixture won't be running? In that case you shouldn't need this because the fixture will always be running if you have this change.

Good to know, thanks!

tlrx · 2018-04-03T09:31:16Z

Thanks @nik9000 !

* master: Reindex: Fix error in delete-by-query rest spec (elastic#29318) Improve similarity integration. (elastic#29187) Fix some query extraction bugs. (elastic#29283) [Docs] Correct experimental note formatting Move Nullable into core (elastic#29341) [Docs] Update getting-started.asciidoc (elastic#29294) Elasticsearch 6.3.0 is now on Lucene 7.3. [DOCS] Refer back to index API for full-document updates in _update API section (elastic#28677) Fix missing comma in ingest-node.asciidoc (elastic#29343) Improve exception handling on TransportMasterNodeAction (elastic#29314) Don't break allocation if resize source index is missing (elastic#29311) Use fixture to test repository-s3 plugin (elastic#29296) Fix NDCG for empty search results (elastic#29267) Pass through script params in scripted metric agg (elastic#29154) Fix Eclipse build. Upgrade to lucene-7.3.0-snapshot-98a6b3d. (elastic#29298) Painless: Remove extraneous INLINE constant. (elastic#29340)

* master: Build: Fix Java9 MR build (elastic#29312) Reindex: Fix error in delete-by-query rest spec (elastic#29318) Improve similarity integration. (elastic#29187) Fix some query extraction bugs. (elastic#29283) [Docs] Correct experimental note formatting Move Nullable into core (elastic#29341) [Docs] Update getting-started.asciidoc (elastic#29294) Elasticsearch 6.3.0 is now on Lucene 7.3. [DOCS] Refer back to index API for full-document updates in _update API section (elastic#28677) Fix missing comma in ingest-node.asciidoc (elastic#29343) Improve exception handling on TransportMasterNodeAction (elastic#29314) Don't break allocation if resize source index is missing (elastic#29311) Use fixture to test repository-s3 plugin (elastic#29296) Fix NDCG for empty search results (elastic#29267) Pass through script params in scripted metric agg (elastic#29154) Fix Eclipse build. Upgrade to lucene-7.3.0-snapshot-98a6b3d. (elastic#29298) Painless: Remove extraneous INLINE constant. (elastic#29340) Remove HTTP max content length leniency (elastic#29337) Begin moving XContent to a separate lib/artifact (elastic#29300)

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

…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 commit adds a new fixture that emulates an Azure Storage service in order to improve the existing integration tests. This is very similar to what has been made for Google Cloud Storage in elastic#28788 and for Amazon S3 in elastic#29296, and it would have helped a lot to catch bugs like elastic#22534.

This commit adds a new fixture that emulates an Azure Storage service in order to improve the existing integration tests. This is very similar to what has been made for Google Cloud Storage in #28788 and for Amazon S3 in #29296, and it would have helped a lot to catch bugs like #22534.

This commit moves the repository-s3 fixture test added in elastic#29296 in a new `qa/third-party/amazon-s3` project. This new project allows the REST integration tests to be executed using the real S3 service when all the required environment variables are provided. When no env var is provided, then the tests are executed using the fixture added in elastic#29296. The REST tests located in the plugin project now only verify that the plugin is correctly loaded. The REST tests have been adapted to allow a bucket name and a base path to be specified as env vars. This way it is possible to run the tests with different base paths (could be anything, like a CI job name or a branch name) without multiplicating buckets. Related to elastic#29349

This commit moves the repository-s3 fixture test added in #29296 in a new `repository-s3/qa/amazon-s3` project. This new project allows the REST integration tests to be executed using the real S3 service when all the required environment variables are provided. When no env var is provided, then the tests are executed using the fixture added in #29296. The REST tests located at the `repository-s3`plugin project now only verify that the plugin is correctly loaded. The REST tests have been adapted to allow a bucket name and a base path to be specified as env vars. This way it is possible to run the tests with different base paths (could be anything, like a CI job name or a branch name) without multiplicating buckets. Related to #29349

tlrx · 2018-06-07T07:20:09Z

This change has been backported to 6.x together with #29372.

…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.

tlrx added review :Distributed/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v7.0.0 labels Mar 29, 2018

nik9000 reviewed Mar 29, 2018

View reviewed changes

Apply feedback

5d54149

tlrx mentioned this pull request Mar 30, 2018

Use ESBlobStoreRepositoryIntegTestCase to test the repository-s3 plugin #29315

Merged

nik9000 approved these changes Mar 30, 2018

View reviewed changes

Remove version skip in YAML test

866e9bf

tlrx mentioned this pull request Apr 3, 2018

Use fixture to test repository-azure plugin #29347

Merged

tlrx merged commit 989e465 into elastic:master Apr 3, 2018

tlrx deleted the add-repository-s3-integration-tests branch April 3, 2018 09:30

tlrx mentioned this pull request Apr 3, 2018

Test cloud based repository plugins using real services backends #29349

Closed

3 tasks

tlrx added the >test Issues or PRs that are addressing/adding tests label Apr 3, 2018

tlrx mentioned this pull request Apr 4, 2018

Move repository-s3 fixture tests to QA third party project #29372

Merged

tlrx added the v6.4.0 label Jun 7, 2018

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use fixture to test repository-s3 plugin #29296

Use fixture to test repository-s3 plugin #29296

tlrx commented Mar 29, 2018

nik9000 Mar 29, 2018

tlrx Mar 30, 2018

nik9000 Mar 29, 2018

tlrx Mar 30, 2018

tlrx commented Mar 30, 2018

nik9000 left a comment

nik9000 Mar 30, 2018

nik9000 Mar 30, 2018

tlrx Apr 3, 2018

tlrx commented Apr 3, 2018

tlrx commented Jun 7, 2018

Use fixture to test repository-s3 plugin #29296

Use fixture to test repository-s3 plugin #29296

Conversation

tlrx commented Mar 29, 2018

nik9000 Mar 29, 2018

Choose a reason for hiding this comment

tlrx Mar 30, 2018

Choose a reason for hiding this comment

nik9000 Mar 29, 2018

Choose a reason for hiding this comment

tlrx Mar 30, 2018

Choose a reason for hiding this comment

tlrx commented Mar 30, 2018

nik9000 left a comment

Choose a reason for hiding this comment

nik9000 Mar 30, 2018

Choose a reason for hiding this comment

nik9000 Mar 30, 2018

Choose a reason for hiding this comment

tlrx Apr 3, 2018

Choose a reason for hiding this comment

tlrx commented Apr 3, 2018

tlrx commented Jun 7, 2018