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

Use versioned sourceset for REST compat testing #78418

Merged
merged 19 commits into from
Oct 12, 2021
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,8 @@ class YamlRestCompatTestPluginFuncTest extends AbstractRestResourcesFuncTest {
String wrongTest = "wrong_version.yml"
String additionalTest = "additional_test.yml"
setupRestResources([wrongApi], [wrongTest]) //setups up resources for current version, which should not be used for this test
addRestTestsToProject([additionalTest], "yamlRestCompatTest")
String sourceSetName = "yamlRestTestV" + compatibleVersion + "Compat"
addRestTestsToProject([additionalTest], sourceSetName)
//intentionally adding to yamlRestTest source set since the .classes are copied from there
file("src/yamlRestTest/java/MockIT.java") << "import org.junit.Test;class MockIT { @Test public void doNothing() { }}"

Expand All @@ -107,17 +108,17 @@ class YamlRestCompatTestPluginFuncTest extends AbstractRestResourcesFuncTest {
file("/build/${testIntermediateDir}/original/rest-api-spec/test/" + test).exists()
file("/build/${testIntermediateDir}/transformed/rest-api-spec/test/" + test).exists()
file("/build/${testIntermediateDir}/transformed/rest-api-spec/test/" + test).text.contains("headers") //transformation adds this
file("/build/resources/yamlRestCompatTest/rest-api-spec/test/" + additionalTest).exists()
file("/build/resources/${sourceSetName}/rest-api-spec/test/" + additionalTest).exists()

//additionalTest is not copied from the prior version, and thus not in the intermediate directory, nor transformed
file("/build/resources/yamlRestCompatTest/" + testIntermediateDir + "/rest-api-spec/test/" + additionalTest).exists() == false
file("/build/resources/yamlRestCompatTest/rest-api-spec/test/" + additionalTest).text.contains("headers") == false
file("/build/resources/${sourceSetName}/" + testIntermediateDir + "/rest-api-spec/test/" + additionalTest).exists() == false
file("/build/resources/${sourceSetName}/rest-api-spec/test/" + additionalTest).text.contains("headers") == false

file("/build/classes/java/yamlRestTest/MockIT.class").exists() //The "standard" runner is used to execute the compat test

file("/build/resources/yamlRestCompatTest/rest-api-spec/api/" + wrongApi).exists() == false
file("/build/resources/yamlRestCompatTest/" + testIntermediateDir + "/rest-api-spec/test/" + wrongTest).exists() == false
file("/build/resources/yamlRestCompatTest/rest-api-spec/test/" + wrongTest).exists() == false
file("/build/resources/${sourceSetName}/rest-api-spec/api/" + wrongApi).exists() == false
file("/build/resources/${sourceSetName}/" + testIntermediateDir + "/rest-api-spec/test/" + wrongTest).exists() == false
file("/build/resources/${sourceSetName}/rest-api-spec/test/" + wrongTest).exists() == false

result.task(':copyRestApiSpecsTask').outcome == TaskOutcome.NO_SOURCE
result.task(':copyYamlTestsTask').outcome == TaskOutcome.NO_SOURCE
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,14 @@
import org.elasticsearch.gradle.internal.test.rest.InternalYamlRestTestPlugin;
import org.elasticsearch.gradle.testclusters.TestClustersPlugin;
import org.elasticsearch.gradle.util.GradleUtils;
import org.gradle.api.Action;
import org.gradle.api.Plugin;
import org.gradle.api.Project;
import org.gradle.api.Task;
import org.gradle.api.artifacts.Configuration;
import org.gradle.api.artifacts.Dependency;
import org.gradle.api.file.Directory;
import org.gradle.api.file.FileTree;
import org.gradle.api.plugins.JavaBasePlugin;
import org.gradle.api.provider.Provider;
import org.gradle.api.tasks.SourceSet;
Expand All @@ -35,7 +37,11 @@

import java.io.File;
import java.nio.file.Path;
import java.util.HashSet;
import java.util.Map;
import java.util.Set;
import java.util.stream.Collectors;
import java.util.stream.Stream;

import static org.elasticsearch.gradle.internal.test.rest.RestTestUtil.setupTestDependenciesDefaults;

Expand All @@ -44,7 +50,6 @@
*/
public class YamlRestCompatTestPlugin implements Plugin<Project> {
private static final String REST_COMPAT_CHECK_TASK_NAME = "checkRestCompat";
private 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");
private static final Path RELATIVE_REST_API_RESOURCES = Path.of("rest-api-spec/src/main/resources");
Expand All @@ -55,6 +60,7 @@ public class YamlRestCompatTestPlugin implements Plugin<Project> {
@Override
public void apply(Project project) {
final int compatibleVersion = Version.fromString(VersionProperties.getVersions().get("elasticsearch")).getMajor() - 1;
final String sourceSetName = "yamlRestTestV" + compatibleVersion + "Compat";
jakelandis marked this conversation as resolved.
Show resolved Hide resolved
final Path compatRestResourcesDir = Path.of("restResources").resolve("v" + compatibleVersion);
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are having version-specific source sets, do we still need to capture the version here in the file path as well. As it is now we have files like src/yamlRestTestV7Compat/resources/rest-api-spec/test/v7compat/foo/bar.yml. We probably don't need to track the version twice now. Also begs the question whether we need this change in the first place if we are already managing per-version test resources. How will this affect backporting, etc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

src/yamlRestTestV7Compat/resources/rest-api-spec/test/v7compat/foo/bar.yml uses a 4 part path to keept it from colliding with the actual v7 test/foo/bar.yml if it exists. (Both the real v7 tests and the custom test in this source set are on the same classpath, and there quite a bit of re-use of file names). v7compat in the path is just a convention and included the v7 to help demark these are intended for compatibilty with v7. The fact this is just a convention and without any teeth (i.e. these would execute when master is v9) is a motivating factor for this PR.

I can remove the v7 from the v7compat convention in this PR since I agree it doesn't make sense now. I think I could also remove this extra directory entirely and ensure uniqueness of the files between the two sources of tests (with validation in Gradle). Thoughts ?

Since our REST compatilbity only supports 1 major version, these should never be backported and anything in this sourceset should be removed in the next major version.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can remove the v7 from the v7compat convention in this PR since I agree it doesn't make sense now. I think I could also remove this extra directory entirely and ensure uniqueness of the files between the two sources of tests (with validation in Gradle). Thoughts ?

Yeah, I think that makes sense. It's just less stuff to try and keep in sync.

final Path compatSpecsDir = compatRestResourcesDir.resolve("yamlSpecs");
final Path compatTestsDir = compatRestResourcesDir.resolve("yamlTests");
Expand All @@ -69,9 +75,9 @@ public void apply(Project project) {

// create source set
SourceSetContainer sourceSets = project.getExtensions().getByType(SourceSetContainer.class);
SourceSet yamlCompatTestSourceSet = sourceSets.create(SOURCE_SET_NAME);
SourceSet yamlCompatTestSourceSet = sourceSets.create(sourceSetName);
SourceSet yamlTestSourceSet = sourceSets.getByName(InternalYamlRestTestPlugin.SOURCE_SET_NAME);
GradleUtils.extendSourceSet(project, InternalYamlRestTestPlugin.SOURCE_SET_NAME, SOURCE_SET_NAME);
GradleUtils.extendSourceSet(project, InternalYamlRestTestPlugin.SOURCE_SET_NAME, sourceSetName);

// copy compatible rest specs
Configuration bwcMinorConfig = project.getConfigurations().create(BWC_MINOR_CONFIG_NAME);
Expand Down Expand Up @@ -138,7 +144,7 @@ public void apply(Project project) {
task.onlyIf(t -> isEnabled(project));
});


// transform the copied tests task
TaskProvider<RestCompatTestTransformTask> transformCompatTestTask = project.getTasks()
.register("yamlRestTestV"+ compatibleVersion + "CompatTransform", RestCompatTestTransformTask.class, task -> {
Expand All @@ -163,7 +169,7 @@ public void apply(Project project) {
.flatMap(CopyRestTestsTask::getOutputResourceDir);

String testTaskName = "yamlRestTestV"+ compatibleVersion + "CompatTest";

// setup the test task
Provider<RestIntegTestTask> yamlRestCompatTestTask = RestTestUtil.registerTestTask(project, yamlCompatTestSourceSet, testTaskName);
project.getTasks().withType(RestIntegTestTask.class).named(testTaskName).configure(testTask -> {
Expand All @@ -178,11 +184,33 @@ public void apply(Project project) {
.minus(project.files(originalYamlSpecsDir))
.minus(project.files(originalYamlTestsDir))
);

// Ensure no duplicates YAML tests on classpath since tests are sourced from the prior major and the local source set
testTask.doFirst(new Action<>() {
@Override
public void execute(Task task) {
FileTree yamlTests = project.getTasks().withType(RestIntegTestTask.class)
.getByName(testTaskName).getClasspath().getAsFileTree().matching(f -> f.include("**/*.yml"));
Set<Path> found = new HashSet<>();
Set<Path> duplicates = yamlTests.getFiles().stream().map(file -> {
Path p = file.toPath();
int partCount = p.getNameCount();
return p.subpath(partCount - 2, partCount);
}).filter(p -> found.add(p) == false).collect(Collectors.toSet());
if (duplicates.isEmpty() == false) {
throw new IllegalStateException("Found duplicated test(s) ["
+ duplicates.stream().map(Path::toString).collect(Collectors.joining(", "))
+ "] please ensure there not any duplicate YAML test files between the [" + compatibleVersion
+ ".x] branch and the [" + sourceSetName + "] tests. ");
}
}
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

example of failure:

* What went wrong:
Execution failed for task ':rest-api-spec:yamlRestTestV7CompatTest'.
> Found duplicated test(s) [search/30_limits.yml] please ensure there not any duplicate YAML test files between the [7.x] branch and the [yamlRestTestV7Compat] tests. 

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if it might be better (and simpler) to implement this in the yaml testing framework code instead. It could be as simple as checking the result of the call to add() here and throwing an exception if it returns false.

// run compatibility tests after "normal" tests
testTask.mustRunAfter(project.getTasks().named(InternalYamlRestTestPlugin.SOURCE_SET_NAME));
testTask.onlyIf(t -> isEnabled(project));
});


setupTestDependenciesDefaults(project, yamlCompatTestSourceSet);

// setup IDE
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import org.gradle.api.DefaultTask;
import org.gradle.api.file.ArchiveOperations;
import org.gradle.api.file.DirectoryProperty;
import org.gradle.api.file.DuplicatesStrategy;
import org.gradle.api.file.FileCollection;
import org.gradle.api.file.FileSystemOperations;
import org.gradle.api.file.FileTree;
Expand Down Expand Up @@ -126,13 +127,15 @@ void copy() {

getLogger().debug("Rest specs for project [{}] will be copied to the test resources.", projectPath);
fileSystemOperations.copy(c -> {
c.setDuplicatesStrategy(DuplicatesStrategy.FAIL);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These duplicate fails are not actually needed for this PR, but I realized while adding the custom duplicate check to the compat testing task, it would be good to add a duplicate check here too.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is necessary since FAIL is now the default behavior in Gradle 7.0 and later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah.. it is. gradle/gradle#15759 I will remove this.

c.from(configToFileTree.apply(config));
c.into(restSpecOutputDir);
c.include(patternSet.getIncludes());
});
// copy any additional config
if (additionalConfig != null) {
fileSystemOperations.copy(c -> {
c.setDuplicatesStrategy(DuplicatesStrategy.FAIL);
c.from(additionalConfigToFileTree.apply(additionalConfig));
c.into(restSpecOutputDir);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import org.apache.tools.ant.filters.ReplaceTokens;
import org.gradle.api.DefaultTask;
import org.gradle.api.file.DirectoryProperty;
import org.gradle.api.file.DuplicatesStrategy;
import org.gradle.api.file.FileCollection;
import org.gradle.api.file.FileSystemOperations;
import org.gradle.api.file.FileTree;
Expand Down Expand Up @@ -136,6 +137,7 @@ void copy() {
if (includeCore.get().isEmpty() == false) {
getLogger().debug("Rest tests for project [{}] will be copied to the test resources.", projectPath);
fileSystemOperations.copy(c -> {
c.setDuplicatesStrategy(DuplicatesStrategy.FAIL);
c.from(coreConfigToFileTree.apply(coreConfig));
c.into(restTestOutputDir);
c.include(corePatternSet.getIncludes());
Expand All @@ -148,6 +150,7 @@ void copy() {
if (includeXpack.get().isEmpty() == false) {
getLogger().debug("X-pack rest tests for project [{}] will be copied to the test resources.", projectPath);
fileSystemOperations.copy(c -> {
c.setDuplicatesStrategy(DuplicatesStrategy.FAIL);
c.from(xpackConfigToFileTree.apply(xpackConfig));
c.into(restTestOutputDir);
c.include(xpackPatternSet.getIncludes());
Expand All @@ -159,6 +162,7 @@ void copy() {
// copy any additional config
if (additionalConfig != null) {
fileSystemOperations.copy(c -> {
c.setDuplicatesStrategy(DuplicatesStrategy.FAIL);
c.from(additionalConfigToFileTree.apply(additionalConfig));
c.into(restTestOutputDir);
if (substitutions != null) {
Expand Down
4 changes: 3 additions & 1 deletion x-pack/plugin/build.gradle
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import org.elasticsearch.gradle.internal.info.BuildParams
import org.elasticsearch.gradle.util.GradleUtils
import org.elasticsearch.gradle.internal.test.RestIntegTestTask
import org.elasticsearch.gradle.VersionProperties

apply plugin: 'elasticsearch.internal-yaml-rest-test'
apply plugin: 'elasticsearch.yaml-rest-compat-test'
Expand All @@ -18,7 +19,8 @@ dependencies {

// let the yamlRestTests see the classpath of test
GradleUtils.extendSourceSet(project, "test", "yamlRestTest", tasks.named("yamlRestTest"))
GradleUtils.extendSourceSet(project, "test", "yamlRestCompatTest")
int compatVersion = VersionProperties.getElasticsearchVersion().getMajor() - 1;
GradleUtils.extendSourceSet(project, "test", "yamlRestTestV${compatVersion}Compat")

restResources {
restApi {
Expand Down