Skip to content
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

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

Merged
merged 3 commits into from
Apr 27, 2018

Conversation

tlrx
Copy link
Member

@tlrx tlrx commented Apr 4, 2018

This commit moves the repository-s3 fixture test added in #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 enviromnent variable is provided, the tests are executed using the fixture added in #29296.

The REST tests located in the plugin project now only verify that the plugin is correctly loaded.

The REST tests in the QA project 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 tlrx added >test Issues or PRs that are addressing/adding tests review :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs v7.0.0 labels Apr 4, 2018
@tlrx tlrx requested a review from rjernst April 4, 2018 13:53
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

}

/** A task to start the AmazonS3Fixture which emulates a S3 service **/
task s3Fixture(type: AntFixture) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: the task is always created but not used when env vars are provided. I find it easier this way.

@tlrx tlrx force-pushed the add-qa-third-party-amazon-s3 branch 2 times, most recently from c5aca22 to 9979f1e Compare April 17, 2018 07:47
@tlrx
Copy link
Member Author

tlrx commented Apr 17, 2018

@elastic/es-core-infra Can someone from the team can review this pull request please? Or maybe do you prefer to hold on this change until another major change is merged? (I'm fine with this but I'd like to know as I have few others PRs that are blocked by this one)

Note that this change should not impact the CI as it will use the fixture to run tests by default.

@tlrx tlrx force-pushed the add-qa-third-party-amazon-s3 branch 3 times, most recently from f945030 to 914964a Compare April 26, 2018 07:35
@tlrx tlrx requested a review from ywelsch April 26, 2018 07:35
Copy link
Contributor

@ywelsch ywelsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recent discussions (Core/Infra meeting yesterday) have concluded that we should put QA projects that are really only testing a single plugin (and not the integration of many) into a subdirectory of that project and hence as a sub-project in Gradle. One of the biggest advantages is that this allows you to test all aspects related to that plugin with just one simple Gradle call. Examples for this are the CCR plugin (see https://github.com/elastic/elasticsearch/tree/ccr/x-pack/plugin/ccr) or the index lifecycle plugin.

The change looks good otherwise.

s3SecretKey = 's3_integration_test_secret_key'
s3Bucket = 'bucket_test'
s3BasePath = 'integration_test'
useFixture = true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should log something if the fixture is not used, i.e., using external service (bucket + path).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should log the bucket name, instead it will print a simple "Using an external service to test the repository-s3 plugin" message

@tlrx
Copy link
Member Author

tlrx commented Apr 26, 2018

Thanks @ywelsch, I didn't know about this decision. The QA test is now located in the plugins/repository-s3/qa/amazon-S3 project. I hooked up the check task so that executing :plugin:repository-s3:check also executes :plugin:repository-s3:qa:amazon-s3:check.

I had to adapt the project fileq of x-pack smoke tests so that they didn't try to include :plugin:repository-s3:qa:amazon-s3 as if it was a real plugin.

Copy link
Contributor

@ywelsch ywelsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

// need to get a non-decorated project object, so must re-lookup the project by path
integTestCluster.plugin(subproj.path)
pluginsCount += 1
if (subproj.plugins.hasPlugin(PluginBuildPlugin) || subproj.plugins.hasPlugin(MetaPluginBuildPlugin)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this is too much magic. An alternative might be to write project.project(':plugins').getChildProjects.each { n, subproj -> ... }
to just range over direct children of plugins.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. This is indeed a good suggestion. I'll update this.

@rjernst
Copy link
Member

rjernst commented Apr 26, 2018

Recent discussions (Core/Infra meeting yesterday) have concluded that we should put QA projects that are really only testing a single plugin (and not the integration of many) into a subdirectory of that project and hence as a sub-project in Gradle.

While this was discussed, it was not concluded. Please don't do this until there is agreement on how (or if) it should be done.

@jasontedor
Copy link
Member

While this was discussed, it was not concluded. Please don't do this until there is agreement on how (or if) it should be done.

We had consensus on the point of tests for features/modules/plugins being next to the feature/module/plugin as sub-projects (e.g., we mentioned examples of CCR/index lifecycle management/reindex). There are some follow-up discussions needed related to the standing up of clusters with various features enabled/disabled, and related to security, but on the broader point of co-locating qa-style tests with the code, we did have consensus in the interest of optimizing workflow for developers on these.

It is apparent that you feel strongly otherwise so we can re-discuss this. However, I do not want to block @tlrx on this discussion where he was acting on guidance from his reviewer who saw the outcome of our previous discussion similarly. I will approve this PR as-is and if we come to a different decision when we discuss this again we can move the tests back (and we will not burden @tlrx with this work), this is okay.

This will be on the agenda for our next core/infra meeting.

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
@tlrx tlrx force-pushed the add-qa-third-party-amazon-s3 branch from 409a480 to 7eeb112 Compare April 27, 2018 08:59
@tlrx tlrx merged commit 7ae3b3b into elastic:master Apr 27, 2018
@tlrx
Copy link
Member Author

tlrx commented Apr 27, 2018

Thanks a lot @ywelsch @jasontedor @rjernst!

This pull request has been merged with the QA project being a sub-project of the repository-s3 plugin. If this decision is revisited by the core/infra team, please don't hesitate to reach me out - I'd be happy to help to move them again.

@tlrx tlrx deleted the add-qa-third-party-amazon-s3 branch April 27, 2018 14:56
tlrx added a commit that referenced this pull request Apr 27, 2018
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
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Apr 27, 2018
* master: (7173 commits)
  Bump changelog version to 6.4 (elastic#30217)
  [DOCS] Adds native realm security settings (elastic#30186)
  Test: Switch painless test to 1 shard
  CCS: Drop http address from remote cluster info (elastic#29568)
  Reindex: Fold "from old" tests into reindex module (elastic#30142)
  Convert FieldCapabilitiesResponse to a ToXContentObject. (elastic#30182)
  [DOCS] Added 'on a single shard' to description of max_thread_count. Closes 28518 (elastic#29686)
  [TEST] Redirect links to new locations (elastic#30179)
  Move repository-s3 fixture tests to QA test project (elastic#29372)
  Fail snapshot operations early on repository corruption (elastic#30140)
  Docs: Document `failures` on reindex and friends
  Build global ordinals terms bucket from matching ordinals (elastic#30166)
  Watcher: Ensure mail message ids are unique per watch action (elastic#30112)
  REST: Remove GET support for clear cache indices (elastic#29525)
  SQL: Correct error message (elastic#30138)
  Require acknowledgement to start_trial license (elastic#30135)
  Fix a bug in FieldCapabilitiesRequest#equals and hashCode. (elastic#30181)
  SQL: Add BinaryMathProcessor to named writeables list (elastic#30127)
  Tests: Use buildDir as base for generated-resources (elastic#30191)
  Fix SliceBuilderTests#testRandom failures
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs >test Issues or PRs that are addressing/adding tests v6.4.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants