-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Introduce yamlRestCompatTest task and plugin. #62985
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
Changes from all commits
f100cb0
42372d5
339695e
ef6bc3c
96245d7
24aa853
ba34a16
85a26b3
f3878a1
b6b05ce
428babb
d0ebbcd
f03894e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -58,8 +58,10 @@ public class CopyRestApiTask extends DefaultTask { | |
| final ListProperty<String> includeCore = getProject().getObjects().listProperty(String.class); | ||
| final ListProperty<String> includeXpack = getProject().getObjects().listProperty(String.class); | ||
| String sourceSetName; | ||
| boolean skipHasRestTestCheck; | ||
| Configuration coreConfig; | ||
| Configuration xpackConfig; | ||
| Configuration additionalConfig; | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is needed as a way to copy the 7.x module/plugin tests to the source set. For "normal" YAML tests they are already in the source set (since they are part of the current source code). |
||
|
|
||
| private final PatternFilterable corePatternSet; | ||
| private final PatternFilterable xpackPatternSet; | ||
|
|
@@ -89,6 +91,11 @@ String getSourceSetName() { | |
| return sourceSetName; | ||
| } | ||
|
|
||
| @Input | ||
| public boolean isSkipHasRestTestCheck() { | ||
| return skipHasRestTestCheck; | ||
| } | ||
|
|
||
| @SkipWhenEmpty | ||
| @InputFiles | ||
| public FileTree getInputDir() { | ||
|
|
@@ -98,7 +105,7 @@ public FileTree getInputDir() { | |
| xpackPatternSet.setIncludes(includeXpack.get().stream().map(prefix -> prefix + "*/**").collect(Collectors.toList())); | ||
| xpackFileTree = xpackConfig.getAsFileTree().matching(xpackPatternSet); | ||
| } | ||
| boolean projectHasYamlRestTests = projectHasYamlRestTests(); | ||
| boolean projectHasYamlRestTests = skipHasRestTestCheck || projectHasYamlRestTests(); | ||
| if (includeCore.get().isEmpty() == false || projectHasYamlRestTests) { | ||
| if (BuildParams.isInternal()) { | ||
| corePatternSet.setIncludes(includeCore.get().stream().map(prefix -> prefix + "*/**").collect(Collectors.toList())); | ||
|
|
@@ -107,7 +114,10 @@ public FileTree getInputDir() { | |
| coreFileTree = coreConfig.getAsFileTree(); // jar file | ||
| } | ||
| } | ||
| ConfigurableFileCollection fileCollection = getProject().files(coreFileTree, xpackFileTree); | ||
|
|
||
| ConfigurableFileCollection fileCollection = additionalConfig == null | ||
| ? getProject().files(coreFileTree, xpackFileTree) | ||
| : getProject().files(coreFileTree, xpackFileTree, additionalConfig.getAsFileTree()); | ||
|
|
||
| // if project has rest tests or the includes are explicitly configured execute the task, else NO-SOURCE due to the null input | ||
| return projectHasYamlRestTests || includeCore.get().isEmpty() == false || includeXpack.get().isEmpty() == false | ||
|
|
@@ -132,7 +142,7 @@ void copy() { | |
| if (BuildParams.isInternal()) { | ||
| getLogger().debug("Rest specs for project [{}] will be copied to the test resources.", project.getPath()); | ||
| project.copy(c -> { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We'd like to get away from using the project property within task actions. I'm currently working on #62968 Instead using project.copy{} we should inject a FileSystemOperations service and use that one for these kind of operations. See for example the LoggedExec task how this service is injected: https://github.com/elastic/elasticsearch/pull/62968/files#diff-babd4b74d83a9cea8bf0823cf6b71b66
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added some TODO's for the new project.copy's to ensure it uses the newer style once that PR lands. I didn't make the change now to avoid avoid copy/paste code from your PR (which could merge clash). If this PR beats yours then feel free to add it to your, else I will circle back to this. |
||
| c.from(coreConfig.getSingleFile()); | ||
| c.from(coreConfig.getAsFileTree()); | ||
| c.into(getOutputDir()); | ||
| c.include(corePatternSet.getIncludes()); | ||
| }); | ||
|
|
@@ -164,6 +174,14 @@ void copy() { | |
| c.include(xpackPatternSet.getIncludes()); | ||
| }); | ||
| } | ||
| // TODO: once https://github.com/elastic/elasticsearch/pull/62968 lands ensure that this uses `getFileSystemOperations()` | ||
| // copy any additional config | ||
| if (additionalConfig != null) { | ||
| project.copy(c -> { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same as above |
||
| c.from(additionalConfig.getAsFileTree()); | ||
| c.into(getOutputDir()); | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,189 @@ | ||
| /* | ||
| * 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. | ||
| */ | ||
|
|
||
| package org.elasticsearch.gradle.test.rest; | ||
|
|
||
| import org.elasticsearch.gradle.ElasticsearchJavaPlugin; | ||
| import org.elasticsearch.gradle.VersionProperties; | ||
| import org.elasticsearch.gradle.test.RestIntegTestTask; | ||
| import org.elasticsearch.gradle.test.RestTestBasePlugin; | ||
| import org.elasticsearch.gradle.testclusters.ElasticsearchCluster; | ||
| import org.elasticsearch.gradle.testclusters.TestClustersPlugin; | ||
| import org.elasticsearch.gradle.testclusters.TestDistribution; | ||
| import org.elasticsearch.gradle.util.GradleUtils; | ||
| import org.gradle.api.Plugin; | ||
| import org.gradle.api.Project; | ||
| import org.gradle.api.artifacts.Configuration; | ||
| import org.gradle.api.plugins.JavaBasePlugin; | ||
| import org.gradle.api.provider.Provider; | ||
| import org.gradle.api.tasks.SourceSet; | ||
| import org.gradle.api.tasks.SourceSetContainer; | ||
|
|
||
| import java.io.File; | ||
| import java.nio.file.Path; | ||
|
|
||
| import static org.elasticsearch.gradle.test.rest.RestTestUtil.createTestCluster; | ||
| import static org.elasticsearch.gradle.test.rest.RestTestUtil.setupDependencies; | ||
|
|
||
| /** | ||
| * Apply this plugin to run the YAML based REST tests from a prior major version against this version's cluster. | ||
| */ | ||
| // TODO: support running tests against multiple prior versions in addition to bwc:minor. To achieve this we will need to create published | ||
| // artifacts that include all of the REST tests for the latest 7.x.y releases | ||
| public class YamlRestCompatTestPlugin implements Plugin<Project> { | ||
|
|
||
| public static final String SOURCE_SET_NAME = "yamlRestCompatTest"; | ||
| private static final Path RELATIVE_API_PATH = Path.of("rest-api-spec/api"); | ||
| private static final Path RELATIVE_TEST_PATH = Path.of("rest-api-spec/test"); | ||
|
|
||
| @Override | ||
| public void apply(Project project) { | ||
|
|
||
| project.getPluginManager().apply(ElasticsearchJavaPlugin.class); | ||
| project.getPluginManager().apply(TestClustersPlugin.class); | ||
| project.getPluginManager().apply(RestTestBasePlugin.class); | ||
| project.getPluginManager().apply(RestResourcesPlugin.class); | ||
| project.getPluginManager().apply(YamlRestTestPlugin.class); | ||
|
|
||
| RestResourcesExtension extension = project.getExtensions().getByType(RestResourcesExtension.class); | ||
|
|
||
| // create source set | ||
| SourceSetContainer sourceSets = project.getExtensions().getByType(SourceSetContainer.class); | ||
| SourceSet yamlCompatTestSourceSet = sourceSets.create(SOURCE_SET_NAME); | ||
| SourceSet yamlTestSourceSet = sourceSets.getByName(YamlRestTestPlugin.SOURCE_SET_NAME); | ||
| GradleUtils.extendSourceSet(project, YamlRestTestPlugin.SOURCE_SET_NAME, SOURCE_SET_NAME); | ||
|
|
||
| // create the test cluster container, and always use the default distribution | ||
| ElasticsearchCluster testCluster = createTestCluster(project, yamlCompatTestSourceSet); | ||
| testCluster.setTestDistribution(TestDistribution.DEFAULT); | ||
|
|
||
| // TODO: once https://github.com/elastic/elasticsearch/pull/62473 lands refactor this to reference the checkoutDir as an artifact | ||
| int priorMajorVersion = VersionProperties.getElasticsearchVersion().getMajor() - 1; | ||
| final Path checkoutDir = project.findProject(":distribution:bwc:minor") | ||
| .getBuildDir() | ||
| .toPath() | ||
| .resolve("bwc") | ||
| .resolve("checkout-" + priorMajorVersion + ".x"); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are we peeking at the source? Instead can we produce the files as an artifact? This would allow us to test against already released versions in the same way, just grab the zip of the spec/test files of that released version.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The artifacts we produce only contain the OSS core api and tests. I need the x-pack core api/tests as well as all of the OSS/X-pack module/plugin API/tests. Back filling those I think is a reasonable approach as a long term solution, but I think sourcing like this is decent temporary approach.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tend to agree with @rjernst that peeking into other source folders is kind of an anti pattern. Instead sharing it declaratively as described here: https://docs.gradle.org/current/userguide/cross_project_publications.html is what we should aim for.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ideally I think the bwc project would just share the checkoutDir as an artifact. Can you put a TODO in there to look into this after #62473 I will then pick that up at one. point. This PR does quite some rework on the bwc setup
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, exposing the checkoutDir as an artifact, I think will clean this up. I added a TODO in the code to come back to this. |
||
|
|
||
| // copy compatible rest specs | ||
| Configuration compatSpec = project.getConfigurations().create("compatSpec"); | ||
| Configuration xpackCompatSpec = project.getConfigurations().create("xpackCompatSpec"); | ||
| Configuration additionalCompatSpec = project.getConfigurations().create("additionalCompatSpec"); | ||
| Provider<CopyRestApiTask> copyCompatYamlSpecTask = project.getTasks() | ||
| .register("copyRestApiCompatSpecsTask", CopyRestApiTask.class, task -> { | ||
| task.includeCore.set(extension.restApi.getIncludeCore()); | ||
| task.includeXpack.set(extension.restApi.getIncludeXpack()); | ||
| task.sourceSetName = SOURCE_SET_NAME; | ||
| task.skipHasRestTestCheck = true; | ||
| task.coreConfig = compatSpec; | ||
| project.getDependencies() | ||
| .add( | ||
| task.coreConfig.getName(), | ||
| project.files(checkoutDir.resolve("rest-api-spec/src/main/resources").resolve(RELATIVE_API_PATH)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here. We should not rely on file dependencies and provide these dependencies as artifacts of the bwc project. |
||
| ); | ||
| task.xpackConfig = xpackCompatSpec; | ||
| project.getDependencies() | ||
| .add( | ||
| task.xpackConfig.getName(), | ||
| project.files(checkoutDir.resolve("x-pack/plugin/src/test/resources").resolve(RELATIVE_API_PATH)) | ||
| ); | ||
| task.additionalConfig = additionalCompatSpec; | ||
| // per project can define custom specifications | ||
| project.getDependencies() | ||
| .add( | ||
| task.additionalConfig.getName(), | ||
| project.files( | ||
| getCompatProjectPath(project, checkoutDir).resolve("src/yamlRestTest/resources").resolve(RELATIVE_API_PATH) | ||
| ) | ||
| ); | ||
| task.dependsOn(task.coreConfig); | ||
| task.dependsOn(task.xpackConfig); | ||
| task.dependsOn(task.additionalConfig); | ||
| task.dependsOn(":distribution:bwc:minor:checkoutBwcBranch"); | ||
| }); | ||
|
|
||
| // copy compatible rest tests | ||
| Configuration compatTest = project.getConfigurations().create("compatTest"); | ||
| Configuration xpackCompatTest = project.getConfigurations().create("xpackCompatTest"); | ||
| Configuration additionalCompatTest = project.getConfigurations().create("additionalCompatTest"); | ||
| Provider<CopyRestTestsTask> copyCompatYamlTestTask = project.getTasks() | ||
| .register("copyRestApiCompatTestTask", CopyRestTestsTask.class, task -> { | ||
| task.includeCore.set(extension.restTests.getIncludeCore()); | ||
| task.includeXpack.set(extension.restTests.getIncludeXpack()); | ||
| task.sourceSetName = SOURCE_SET_NAME; | ||
| task.coreConfig = compatTest; | ||
| project.getDependencies() | ||
| .add( | ||
| task.coreConfig.getName(), | ||
| project.files(checkoutDir.resolve("rest-api-spec/src/main/resources").resolve(RELATIVE_TEST_PATH)) | ||
| ); | ||
| task.xpackConfig = xpackCompatTest; | ||
| project.getDependencies() | ||
| .add( | ||
| task.xpackConfig.getName(), | ||
| project.files(checkoutDir.resolve("x-pack/plugin/src/test/resources").resolve(RELATIVE_TEST_PATH)) | ||
| ); | ||
| task.additionalConfig = additionalCompatTest; | ||
| project.getDependencies() | ||
| .add( | ||
| task.additionalConfig.getName(), | ||
| project.files( | ||
| getCompatProjectPath(project, checkoutDir).resolve("src/yamlRestTest/resources").resolve(RELATIVE_TEST_PATH) | ||
| ) | ||
| ); | ||
| task.dependsOn(task.coreConfig); | ||
| task.dependsOn(task.xpackConfig); | ||
| task.dependsOn(task.additionalConfig); | ||
| task.dependsOn(":distribution:bwc:minor:checkoutBwcBranch"); | ||
| task.dependsOn(copyCompatYamlSpecTask); | ||
| }); | ||
|
|
||
| // setup the yamlRestTest task | ||
| Provider<RestIntegTestTask> yamlRestCompatTestTask = RestTestUtil.registerTask(project, yamlCompatTestSourceSet); | ||
| project.getTasks().withType(RestIntegTestTask.class).named(SOURCE_SET_NAME).configure(testTask -> { | ||
| // Use test runner and classpath from "normal" yaml source set | ||
| testTask.setTestClassesDirs(yamlTestSourceSet.getOutput().getClassesDirs()); | ||
| testTask.setClasspath( | ||
| yamlTestSourceSet.getRuntimeClasspath() | ||
| // remove the "normal" api and tests | ||
| .minus(project.files(yamlTestSourceSet.getOutput().getResourcesDir())) | ||
| // add any additional classes/resources from the compatible source set | ||
| // the api and tests are copied to the compatible source set | ||
| .plus(yamlCompatTestSourceSet.getRuntimeClasspath()) | ||
| ); | ||
| // run compatibility tests after "normal" tests | ||
| testTask.mustRunAfter(project.getTasks().named(YamlRestTestPlugin.SOURCE_SET_NAME)); | ||
| testTask.dependsOn(copyCompatYamlTestTask); | ||
| }); | ||
|
|
||
| // setup the dependencies | ||
| setupDependencies(project, yamlCompatTestSourceSet); | ||
|
|
||
| // setup IDE | ||
| GradleUtils.setupIdeForTestSourceSet(project, yamlCompatTestSourceSet); | ||
|
|
||
| // wire this task into check | ||
| project.getTasks().named(JavaBasePlugin.CHECK_TASK_NAME).configure(check -> check.dependsOn(yamlRestCompatTestTask)); | ||
| } | ||
|
|
||
| // TODO: implement custom extension that allows us move around of the projects between major versions and still find them | ||
| private Path getCompatProjectPath(Project project, Path checkoutDir) { | ||
| return checkoutDir.resolve(project.getPath().replaceFirst(":", "").replace(":", File.separator)); | ||
| } | ||
| } | ||
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.
This is needed so that you can run compat tests for a project that may no longer have YAML REST tests in master. Also, there may be cases where a module is completely deleted in master, but we will still want to use this plugin to only test compatibility.
This basically ensures that the execution of "compat" YAML tests is not directly coupled to the existence of "normal" YAML rest tests.