Read the full REST spec from a common directory #51890
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| package org.elasticsearch.gradle.test | ||
|
|
||
| import org.elasticsearch.gradle.VersionProperties | ||
| import org.elasticsearch.gradle.info.BuildParams | ||
| import org.elasticsearch.gradle.testclusters.RestTestRunnerTask | ||
| import org.gradle.api.Plugin | ||
| import org.gradle.api.Project | ||
| import org.gradle.api.Task | ||
| import org.gradle.api.tasks.Copy | ||
|
|
||
| /** | ||
| * Copies the core REST specs to the local resource directory for the REST YAML tests. | ||
| */ | ||
| class CopyRestApiSpecPlugin implements Plugin<Project> { | ||
|
There was a problem hiding this comment. We still have a few places where we need the specs copied to the local resources. Notably the examples so that they can rely on the release jar. There are few others projects where it is easier to copy them locally then run from the global dir. However, I believe all (but 1) of the modules, plugins, and x-pack plugins REST Yaml tests run from the global dir. There was a problem hiding this comment. It's not clear to me what the difference is between just putting them on the test runtime classpath, and copying them to the resources output directory. Either way, we are just throwing these files on the classpath. There was a problem hiding this comment. I think I may have mis-attributed "easier", I will re-visit the usage of this. However, this is still required for the examples project. |
||
|
|
||
| @Override | ||
| void apply(Project project) { | ||
| project.with { | ||
| configurations.create('restSpec') | ||
| dependencies.add( | ||
| 'restSpec', | ||
| BuildParams.internal ? project.project(':rest-api-spec') : | ||
| "org.elasticsearch:rest-api-spec:${VersionProperties.elasticsearch}" | ||
| ) | ||
|
|
||
| tasks.create("copyCoreRestSpecLocal", Copy) { | ||
| dependsOn project.configurations.restSpec | ||
| into(project.sourceSets.test.output.resourcesDir) | ||
| from({ project.zipTree(project.configurations.restSpec.singleFile) }) { | ||
| includeEmptyDirs = false | ||
| include 'rest-api-spec/api/**' | ||
| } | ||
| } | ||
|
|
||
| tasks.withType(RestTestRunnerTask).each { Task t -> | ||
| t.dependsOn('copyCoreRestSpecLocal') | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| # | ||
| # Licensed to Elasticsearch under one or more contributor | ||
| # license agreements. See the NOTICE file distributed with | ||
| # this work for additional information regarding copyright | ||
| # ownership. Elasticsearch licenses this file to you under | ||
| # the Apache License, Version 2.0 (the "License"); you may | ||
| # not use this file except in compliance with the License. | ||
| # You may obtain a copy of the License at | ||
| # | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # Unless required by applicable law or agreed to in writing, | ||
| # software distributed under the License is distributed on an | ||
| # "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| # KIND, either express or implied. See the License for the | ||
| # specific language governing permissions and limitations | ||
| # under the License. | ||
| # | ||
|
|
||
| implementation-class=org.elasticsearch.gradle.test.CopyRestApiSpecPlugin |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
|
|
||
| /** | ||
| * Copies all of the REST api specs (including core, modules, plugins, and x-pack) to common location for the REST YAML test. | ||
| * This plugin is expected to only be applied to applied to the root project. | ||
| */ | ||
| File rootBuildDir = rootProject.buildDir | ||
|
There was a problem hiding this comment. Ok, so I think we decided against centralizing all specs because I came late to the party with the whole cache hit issue. Sorry about that. Given that then the only common specs are those in |
||
|
|
||
| tasks.create("copyCoreRestSpecGlobal", Copy) { | ||
| from project.findProject(':rest-api-spec').projectDir | ||
| into rootBuildDir | ||
| includeEmptyDirs = false | ||
| include '**/src/**/rest-api-spec/api/**' | ||
| eachFile { | ||
| path = new File("rest-api-spec/api", name) | ||
| } | ||
| } | ||
|
|
||
| tasks.create("copyModulesRestSpecGlobal", Copy) { | ||
|
There was a problem hiding this comment. So essentially this is the stuff you ripped out of There was a problem hiding this comment. As mentioned above, there are many usages of There was a problem hiding this comment. Not all There was a problem hiding this comment. That makes sense... However, I would like to also remove the Thoughts on removing There was a problem hiding this comment.
We'll want to backport this to
I don't think so. There are a number of plugins that don't use YAML tests. I think the problem is when we say "rest tests" we actually mean a few things. This includes both Java-based tests that target an external cluster and YAML tests. We only need the specs for the latter, but we group them together as a single thing. We need to tease these apart. Not all projects that execute tests against an external cluster (i.e. have a |
||
| from project.findProject(':modules').projectDir | ||
| into rootBuildDir | ||
| includeEmptyDirs = false | ||
| include '**/src/**/rest-api-spec/api/**' | ||
| eachFile { | ||
| path = new File("rest-api-spec/api", name) | ||
| } | ||
| } | ||
|
|
||
| tasks.create("copyPluginsRestSpecGlobal", Copy) { | ||
| from project.findProject(':plugins').projectDir | ||
| into rootBuildDir | ||
| includeEmptyDirs = false | ||
| include '**/src/**/rest-api-spec/api/**' | ||
| exclude '**/examples/**' | ||
| eachFile { | ||
| path = new File("rest-api-spec/api", name) | ||
| } | ||
| } | ||
|
|
||
| tasks.create("copyXpackPluginsRestSpecGlobal", Copy) { | ||
| from project.findProject(':x-pack:plugin').projectDir | ||
| into rootBuildDir | ||
| includeEmptyDirs = false | ||
| include '**/src/**/rest-api-spec/api/**' | ||
| eachFile { | ||
| path = new File("rest-api-spec/api", name) | ||
| } | ||
| } | ||
|
|
||
| tasks.create("copyAllRestSpecsGlobal") { | ||
| dependsOn copyCoreRestSpecGlobal | ||
| dependsOn copyModulesRestSpecGlobal | ||
| dependsOn copyPluginsRestSpecGlobal | ||
| dependsOn copyXpackPluginsRestSpecGlobal | ||
| } | ||
|
|
||
| configurations { | ||
| copyRestApiSpecGlobal | ||
| } | ||
| artifacts { | ||
| copyRestApiSpecGlobal(file(rootBuildDir)) { builtBy copyAllRestSpecsGlobal } | ||
| } | ||
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.
I think this logic should move to the existing
RestTestPluginrather than shove it in awithType()block. This has the side effect of creating this dependency multiple times, once for each instance of that task type. This is benign, but also 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.
The
RestTestPlugincreates aintegTesttask of typeRestIntegTestTasktask. But so does thePluginBuildPlugin(esplugin) (not sure why it explicitly creates it instead of applying theRestTestPlugin).Additionally, many of the projects create custom tasks of type
RestIntegTestTask. The requirement for theRestTestRunnerTaskwhich is created by theRestIntegTestTask.Actually, I think this should actually be
withType(RestTestRunnerTask)since a couple project create that directly.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.
We aren't configuring the task, we are adding a
testRuntimedependency to the project. All test tasks inherit that classpath so there is no need to couple this to the existence of aRestIntegTestTask.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.
Based on your comment below
I can move this to the
RestTestPlugin