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

Smarter copying of the rest specs and tests #52114

Merged
merged 27 commits into from
Feb 26, 2020
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
c7d36e0
Smarter copying of the rest specs and tests
jakelandis Feb 5, 2020
1e02fbe
fix autoscaling project (merge clash)
jakelandis Feb 9, 2020
02f95b8
fix graph tests
jakelandis Feb 9, 2020
7b81e65
fix xpack docs build
jakelandis Feb 10, 2020
f334d40
fix ml tests
jakelandis Feb 10, 2020
c1ee4ff
Merge branch 'master' into rest_spec_from_common_dir3
elasticmachine Feb 10, 2020
f43429c
Use lazy config, task, and review comments
jakelandis Feb 10, 2020
818e761
Merge branch 'master' into rest_spec_from_common_dir3
jakelandis Feb 12, 2020
bddf05b
change name of plugin
jakelandis Feb 12, 2020
0c1d164
fix hlrc test
jakelandis Feb 12, 2020
bbcde3a
fix some missed includes
jakelandis Feb 12, 2020
2dc1a3a
move to 1 extension, minor clean up
jakelandis Feb 12, 2020
8942ac2
split to two tasks
jakelandis Feb 13, 2020
2f5739c
fix smoke-test-security-with-mustache test
jakelandis Feb 13, 2020
907c0dd
fix some of the x-pack:qa tests
jakelandis Feb 13, 2020
173b494
fix another x-pack qa test
jakelandis Feb 13, 2020
6d0aec5
fix spotless
jakelandis Feb 13, 2020
6a74f0b
another qa xpack test fix
jakelandis Feb 13, 2020
a391dd7
fix another x-pack test (the prior iteration auto added all of x-pac…
jakelandis Feb 14, 2020
9583d15
clean up
jakelandis Feb 24, 2020
1617f6f
Merge remote-tracking branch 'upstream/master' into rest_spec_from_co…
jakelandis Feb 24, 2020
d86fc58
move logger to debug and remove incidental change
jakelandis Feb 24, 2020
852b30e
fix 3rd party config and remove uneeded copy
jakelandis Feb 25, 2020
10700bc
address review comments
jakelandis Feb 25, 2020
bb84161
fix checkStyle
jakelandis Feb 25, 2020
2e4d1d6
Merge remote-tracking branch 'upstream/master' into rest_spec_from_co…
jakelandis Feb 25, 2020
9e6e2aa
fix plugin name
jakelandis Feb 25, 2020
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 @@ -24,6 +24,7 @@ import org.elasticsearch.gradle.NoticeTask
import org.elasticsearch.gradle.Version
import org.elasticsearch.gradle.VersionProperties
import org.elasticsearch.gradle.info.BuildParams
import org.elasticsearch.gradle.test.RestApiSpecForTestingPlugin
import org.elasticsearch.gradle.test.RestIntegTestTask
import org.elasticsearch.gradle.testclusters.RunTask
import org.elasticsearch.gradle.testclusters.TestClustersPlugin
Expand Down Expand Up @@ -51,6 +52,7 @@ class PluginBuildPlugin implements Plugin<Project> {
void apply(Project project) {
project.pluginManager.apply(BuildPlugin)
project.pluginManager.apply(TestClustersPlugin)
project.pluginManager.apply(RestApiSpecForTestingPlugin)

PluginPropertiesExtension extension = project.extensions.create(PLUGIN_EXTENSION_NAME, PluginPropertiesExtension, project)
configureDependencies(project)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,29 +18,19 @@
*/
package org.elasticsearch.gradle.test

import org.elasticsearch.gradle.VersionProperties
import org.elasticsearch.gradle.info.BuildParams
import org.elasticsearch.gradle.testclusters.ElasticsearchCluster
import org.elasticsearch.gradle.testclusters.RestTestRunnerTask
import org.elasticsearch.gradle.tool.Boilerplate
import org.gradle.api.DefaultTask
import org.gradle.api.Task
import org.gradle.api.file.FileCopyDetails
import org.gradle.api.tasks.Copy
import org.gradle.api.tasks.Input
import org.gradle.api.tasks.testing.Test
import org.gradle.plugins.ide.idea.IdeaPlugin

/**
* A wrapper task around setting up a cluster and running rest tests.
*/
class RestIntegTestTask extends DefaultTask {

protected Test runner

/** Flag indicating whether the rest tests in the rest spec should be run. */
@Input
Boolean includePackaged = false

RestIntegTestTask() {
runner = project.tasks.create("${name}Runner", RestTestRunnerTask.class)
super.dependsOn(runner)
Expand Down Expand Up @@ -69,10 +59,6 @@ class RestIntegTestTask extends DefaultTask {
runner.systemProperty('test.clustername', System.getProperty("tests.clustername"))
}

// copy the rest spec/tests onto the test classpath
Copy copyRestSpec = createCopyRestSpecTask()
project.sourceSets.test.output.builtBy(copyRestSpec)

// this must run after all projects have been configured, so we know any project
// references can be accessed as a fully configured
project.gradle.projectsEvaluated {
Expand All @@ -83,12 +69,6 @@ class RestIntegTestTask extends DefaultTask {
}
}

/** Sets the includePackaged property */
public void includePackaged(boolean include) {
includePackaged = include
}


@Override
public Task dependsOn(Object... dependencies) {
runner.dependsOn(dependencies)
Expand All @@ -114,37 +94,4 @@ class RestIntegTestTask extends DefaultTask {
project.tasks.getByName("${name}Runner").configure(configure)
}

Copy createCopyRestSpecTask() {
Boilerplate.maybeCreate(project.configurations, 'restSpec') {
project.dependencies.add(
'restSpec',
BuildParams.internal ? project.project(':rest-api-spec') :
"org.elasticsearch:rest-api-spec:${VersionProperties.elasticsearch}"
)
}

return Boilerplate.maybeCreate(project.tasks, 'copyRestSpec', Copy) { Copy copy ->
copy.dependsOn project.configurations.restSpec
copy.into(project.sourceSets.test.output.resourcesDir)
copy.from({ project.zipTree(project.configurations.restSpec.singleFile) }) {
includeEmptyDirs = false
include 'rest-api-spec/**'
filesMatching('rest-api-spec/test/**') { FileCopyDetails details ->
if (includePackaged == false) {
details.exclude()
}
}
}

if (project.plugins.hasPlugin(IdeaPlugin)) {
project.idea {
module {
if (scopes.TEST != null) {
scopes.TEST.plus.add(project.configurations.restSpec)
}
}
}
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@ class StandaloneRestTestPlugin implements Plugin<Project> {
// only setup tests to build
SourceSetContainer sourceSets = project.extensions.getByType(SourceSetContainer)
SourceSet testSourceSet = sourceSets.create('test')
// need to apply plugin after test source sets are created
project.pluginManager.apply(RestApiSpecForTestingPlugin)

project.tasks.withType(Test) { Test test ->
test.testClassesDirs = testSourceSet.output.classesDirs
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ public void doCheck() throws IOException {
Files.write(getSuccessMarker().toPath(), new byte[] {}, StandardOpenOption.CREATE);
} else {
getLogger().error(problems);
throw new IllegalStateException("Testing conventions are not honored");
throw new IllegalStateException(String.format("Testing conventions [%s] are not honored", problems));
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
package org.elasticsearch.gradle.test;

import java.util.ArrayList;
import java.util.List;

public class RestApiSpecExtension {
private List<String> includesCoreSpec = new ArrayList<>();
private List<String> includesCoreTests = new ArrayList<>();
private List<String> includesXpackSpec = new ArrayList<>();
private List<String> includesXpackTests = new ArrayList<>();
private boolean alwaysCopySpec = false;
private boolean copyTests = false;
private boolean copyXpackTests = false;

public void includeCoreSpec(String include) {
jakelandis marked this conversation as resolved.
Show resolved Hide resolved
includesCoreSpec.add(include);
}

public List<String> getIncludesCoreSpec() {
return includesCoreSpec;
}

public void includeCoreTests(String include) {
includesCoreTests.add(include);
}

public List<String> getIncludesCoreTests() {
return includesCoreTests;
}

public void includeXpackSpec(String include) {
includesXpackSpec.add(include);
}

public List<String> getIncludesXpackSpec() {
return includesXpackSpec;
}

public void copyCoreTests(boolean copyTests) {
jakelandis marked this conversation as resolved.
Show resolved Hide resolved
this.copyTests = copyTests;
}

public boolean shouldCopyCoreTests() {
return copyTests;
}

public void includeXpackTests(String include) {
includesXpackTests.add(include);
}

public List<String> getIncludesXpackTests() {
return includesXpackTests;
}

public void copyXpackTests(boolean copyTests) {
this.copyXpackTests = copyTests;
}

public boolean shouldCopyXpackTests() {
return copyXpackTests;
}

public boolean shouldAlwaysCopySpec() {
return alwaysCopySpec;
}

public void alwaysCopySpec(boolean alwaysCopySpec) {
this.alwaysCopySpec = alwaysCopySpec;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,141 @@
package org.elasticsearch.gradle.test;

import org.elasticsearch.gradle.VersionProperties;
import org.elasticsearch.gradle.info.BuildParams;
import org.elasticsearch.gradle.tool.Boilerplate;
import org.gradle.api.Plugin;
import org.gradle.api.Project;
import org.gradle.api.artifacts.Configuration;
import org.gradle.api.artifacts.Dependency;
import org.gradle.api.logging.Logger;
import org.gradle.api.logging.Logging;
import org.gradle.api.tasks.Copy;
import org.gradle.api.tasks.SourceSet;

import java.io.File;
import java.io.IOException;
import java.nio.file.Files;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.stream.Collectors;

//TODO: change logger.info to logger.debug
public class RestApiSpecForTestingPlugin implements Plugin<Project> {

private static final Logger logger = Logging.getLogger(RestApiSpecForTestingPlugin.class);
private static final String EXTENSION_NAME = "restApiSpec";
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 everywhere we see REST we should actually use YAML. If you are not using are YAML testing framework, is anything of this stuff useful? That is, for the Java-based REST tests, do we need any of this stuff? If not, let's be more specific since we already overload the phrase "rest tests" to an enormous degree, let's be as specific as possible with naming conventions here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ended up with two different extensions, copyRestApiSpecs and copyYamlTests. I am abit adverse to referring to the RestApiSpec with YAML, since they are actually defined in JSON, and have purposes outside of the YAML RestTests. But I agree rest tests are overloaded and am flexible on the naming.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am abit adverse to referring to the RestApiSpec with YAML, since they are actually defined in JSON, and have purposes outside of the YAML RestTests.

What else to we expect build consumers to need with these spec outside the scope of YAML testing? Are there instances of projects consuming rest specs but not doing YAML testing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of the client's (except HLRC) use the spec to drive code generation or language specific testing, however not sure if they integrate with Gradle. The HLRC tests does some validation outside of the RestTests framework, and docs also use them for testing (but in a slightly different way). There could also be additional future usages of the spec outside the testing framework.

private static final String apiDir = "rest-api-spec/api";
private static final String testDir = "rest-api-spec/test";

@Override
public void apply(Project project) {

RestApiSpecExtension extension = project.getExtensions().create(EXTENSION_NAME, RestApiSpecExtension.class);
// need to defer to after evaluation to allow the custom extension to be populated
project.afterEvaluate(p -> {
try {
// copy tests
boolean hasCopiedTests = false;
if (extension.shouldCopyCoreTests()) {
jakelandis marked this conversation as resolved.
Show resolved Hide resolved
Configuration coreTestConfig = project.getConfigurations().create("copyRestSpecCoreTests");
Dependency coreTestDep = project.getDependencies()
.project(Map.of("path", ":rest-api-spec", "configuration", "restSpecCoreTests"));
logger.info("Rest specs tests for project [{}] will be copied to the test resources.", project.getPath());
createCopyTask(project, coreTestConfig, coreTestDep, extension.getIncludesCoreTests(), testDir, false);
hasCopiedTests = true;
}
if (extension.shouldCopyXpackTests()) {
Configuration xpackSpecTestsConfig = project.getConfigurations().create("copyRestSpecXpackTests");
Dependency xpackTestsDep = project.getDependencies()
.project(Map.of("path", ":x-pack:plugin", "configuration", "restSpecXpackTests"));
logger.info("Rest specs x-pack tests for project [{}] will be copied to the test resources.", project.getPath());
createCopyTask(project, xpackSpecTestsConfig, xpackTestsDep, extension.getIncludesXpackTests(), testDir, false);
hasCopiedTests = true;
}
// copy specs
if (projectHasRestTests(project) || hasCopiedTests || extension.shouldAlwaysCopySpec()) {
Configuration coreSpecConfig = project.getConfigurations().create("restSpec"); // name chosen for passivity
Dependency coreSpecDep = project.getDependencies()
.project(Map.of("path", ":rest-api-spec", "configuration", "restSpecCore"));

if (BuildParams.isInternal()) {
// source the specs from this project - this is the path for Elasticsearch builds
if (project.getPath().startsWith(":x-pack")) {
Configuration xpackSpecConfig = project.getConfigurations().create("copyRestSpecXpack");
Dependency xpackSpecDep = project.getDependencies()
.project(Map.of("path", ":x-pack:plugin", "configuration", "restSpecXpack"));
logger.info("X-pack rest specs for project [{}] will be copied to the test resources.", project.getPath());
createCopyTask(project, xpackSpecConfig, xpackSpecDep, extension.getIncludesXpackSpec(), apiDir, false);
}

logger.info("Rest specs for project [{}] will be copied to the test resources.", project.getPath());
createCopyTask(project, coreSpecConfig, coreSpecDep, extension.getIncludesCoreSpec(), apiDir, false);

} else {
// source the specs from the published jar - this is the path plugin developers
logger.info(
"Rest specs for project [{}] will be copied to the test resources from the published jar (version: [{}]).",
project.getPath(),
VersionProperties.getElasticsearch()
);
Dependency coreSpecFromJarDep = project.getDependencies()
.create("org.elasticsearch:rest-api-spec:" + VersionProperties.getElasticsearch());

createCopyTask(project, coreSpecConfig, coreSpecFromJarDep, extension.getIncludesCoreSpec(), "", true);
}
} else {
logger.info("Rest specs will be ignored for project [{}] since there are no REST tests", project.getPath());
return;
}
} catch (Exception e) {
throw new IllegalStateException("Error configuring the rest-api-spec-for-testing plugin. This is likely a bug.", e);
}
});
}

private void createCopyTask(Project project, Configuration config, Dependency dep, List<String> includes, String into, boolean isJar) {
project.getDependencies().add(config.getName(), dep);
Copy copyTask = project.getTasks().create(config.getName() + "Task", Copy.class, copy -> {
jakelandis marked this conversation as resolved.
Show resolved Hide resolved
copy.from(isJar ? project.zipTree(config.getSingleFile()) : config.getSingleFile());
jakelandis marked this conversation as resolved.
Show resolved Hide resolved
copy.into(new File(getTestSourceSet(project).getOutput().getResourcesDir(), into));
if (includes.isEmpty()) {
copy.include(isJar ? apiDir + "/**" : "*/**");
} else {
includes.forEach(s -> copy.include(isJar ? apiDir + "/" + s + "*/**" : s + "*/**"));
}
copy.setIncludeEmptyDirs(false);
jakelandis marked this conversation as resolved.
Show resolved Hide resolved
});
copyTask.dependsOn(config);
project.getTasks().getByName("processTestResources").dependsOn(copyTask);
}

private boolean projectHasRestTests(Project project) throws IOException {
File testResourceDir = getTestResourceDir(project);
if (testResourceDir == null || new File(testResourceDir, "rest-api-spec/test").exists() == false) {
return false;
}
return Files.walk(testResourceDir.toPath().resolve("rest-api-spec/test")).anyMatch(p -> p.getFileName().toString().endsWith("yml"));
}

private File getTestResourceDir(Project project) {
SourceSet testSources = getTestSourceSet(project);
if (testSources == null) {
return null;
}
Set<File> resourceDir = testSources.getResources()
.getSrcDirs()
.stream()
.filter(f -> f.isDirectory() && f.getParentFile().getName().equals("test") && f.getName().equals("resources"))
.collect(Collectors.toSet());
assert resourceDir.size() <= 1;
if (resourceDir.size() == 0) {
return null;
}
return resourceDir.iterator().next();
}

private SourceSet getTestSourceSet(Project project) {
return Boilerplate.getJavaSourceSets(project).findByName("test");
}
}
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.RestApiSpecForTestingPlugin
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ private static String propertyName(String methodName) {
}

public static class MutableBuildParams {
private static MutableBuildParams INSTANCE = new MutableBuildParams();
public static MutableBuildParams INSTANCE = new MutableBuildParams();

private MutableBuildParams() {}

Expand Down
Loading