-
Notifications
You must be signed in to change notification settings - Fork 25k
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
Read the full REST spec from a common directory #51794
Changes from all commits
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 |
---|---|---|
|
@@ -29,6 +29,22 @@ esplugin { | |
noticeFile rootProject.file('NOTICE.txt') | ||
} | ||
|
||
configurations { | ||
restSpec | ||
} | ||
|
||
dependencies { | ||
restSpec project(':rest-api-spec') | ||
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 can't use project dependencies like this in our example projects. These examples are mean to be completely self-contained. That is, you should be able to copy and paste this into your own build and it work. This obviously wouldn't work in that scenario as the project It's not even clear why this is necessary. Folks building plugins should not need to run our rest api tests. 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. Understood, I think the refactoring you suggested will remove the need for this.
There is a project with an example REST test, which needs the needs the REST spec. This was an attempt to avoid making a reference to the rootBuildDir. Moot point though :) |
||
} | ||
|
||
processTestResources { | ||
from({ zipTree(configurations.restSpec.singleFile) }) { | ||
includeEmptyDirs = false | ||
include 'rest-api-spec/api/**' | ||
} | ||
dependsOn configurations.restSpec | ||
} | ||
|
||
// No unit tests in this example | ||
test.enabled = false | ||
|
||
|
@@ -43,6 +59,7 @@ integTest { | |
dependsOn exampleFixture | ||
runner { | ||
nonInputProperties.systemProperty 'external.address', "${-> exampleFixture.addressAndPort}" | ||
nonInputProperties.systemProperty('tests.rest.spec_root', project.sourceSets.test.output.resourcesDir) | ||
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. As mentioned in my other comment, I don't see the need for the addition of this system property everywhere. Do we ever set this to any other location? 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. it is see comment above. |
||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,6 +32,7 @@ | |
import org.elasticsearch.client.RestClientBuilder; | ||
import org.elasticsearch.client.sniff.ElasticsearchNodesSniffer; | ||
import org.elasticsearch.common.Strings; | ||
import org.elasticsearch.common.SuppressForbidden; | ||
import org.elasticsearch.common.collect.Tuple; | ||
import org.elasticsearch.common.io.PathUtils; | ||
import org.elasticsearch.common.xcontent.NamedXContentRegistry; | ||
|
@@ -49,6 +50,7 @@ | |
import java.io.IOException; | ||
import java.nio.file.Files; | ||
import java.nio.file.Path; | ||
import java.nio.file.Paths; | ||
import java.util.ArrayList; | ||
import java.util.Arrays; | ||
import java.util.Comparator; | ||
|
@@ -57,6 +59,7 @@ | |
import java.util.List; | ||
import java.util.Locale; | ||
import java.util.Map; | ||
import java.util.Objects; | ||
import java.util.Set; | ||
|
||
/** | ||
|
@@ -66,8 +69,8 @@ | |
* The suite timeout is extended to account for projects with a large number of tests. | ||
*/ | ||
@TimeoutSuite(millis = 30 * TimeUnits.MINUTE) | ||
@SuppressForbidden(reason = "reads the REST spec from the real filesystem, not the mock one used for testing") | ||
public abstract class ESClientYamlSuiteTestCase extends ESRestTestCase { | ||
|
||
/** | ||
* Property that allows to control which REST tests get run. Supports comma separated list of tests | ||
* or directories that contain tests e.g. -Dtests.rest.suite=index,get,create/10_with_id | ||
|
@@ -88,8 +91,14 @@ public abstract class ESClientYamlSuiteTestCase extends ESRestTestCase { | |
*/ | ||
private static final String REST_TESTS_VALIDATE_SPEC = "tests.rest.validate_spec"; | ||
|
||
private static final String TESTS_PATH = "/rest-api-spec/test"; | ||
private static final String SPEC_PATH = "/rest-api-spec/api"; | ||
/** | ||
* Property that allows to set the root for the rest API spec. This value plus SPEC_PATH is the location of the REST API spec can be | ||
* found. | ||
*/ | ||
public static final String REST_SPEC_ROOT = "tests.rest.spec_root"; | ||
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. It's not clear what this property is doing. We seem to be adding a way to specifify where specifically in the classpath to look for this stuff, but everywhere I can tell we simply set this location to the root of the resources output directory. This seems superfluous. If we are going to continue to load these off the classpath, we should just assume they live in some particular locaiton (which we do, at 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. It flexes between the rootBuildDir and the test.output.resoureDir, depending on the test. I think with the refactoring suggested above, it may be able to eliminate the variance and just use a well known path. 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'm a bit confused, are we adding 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. to close the loop here... |
||
public static final Path SPEC_PATH = Paths.get("rest-api-spec/api"); | ||
|
||
private static final String TESTS_CLASSPATH = "/rest-api-spec/test"; | ||
|
||
/** | ||
* This separator pattern matches ',' except it is preceded by a '\'. | ||
|
@@ -125,7 +134,7 @@ public void initAndResetContext() throws Exception { | |
if (restTestExecutionContext == null) { | ||
assert adminExecutionContext == null; | ||
assert blacklistPathMatchers == null; | ||
final ClientYamlSuiteRestSpec restSpec = ClientYamlSuiteRestSpec.load(SPEC_PATH); | ||
final ClientYamlSuiteRestSpec restSpec = ClientYamlSuiteRestSpec.load(getSpecPath()); | ||
validateSpec(restSpec); | ||
final List<HttpHost> hosts = getClusterHosts(); | ||
Tuple<Version, Version> versionVersionTuple = readVersionsFromCatNodes(adminClient()); | ||
|
@@ -164,6 +173,15 @@ protected ClientYamlTestClient initClientYamlTestClient( | |
return new ClientYamlTestClient(restSpec, restClient, hosts, esVersion, masterVersion, this::getClientBuilderWithSniffedHosts); | ||
} | ||
|
||
private Path getSpecPath() { | ||
Path restSpec = Paths.get( | ||
Objects.requireNonNull( | ||
System.getProperty(REST_SPEC_ROOT), "System property [" + REST_SPEC_ROOT + "] must not be null")) | ||
.resolve(SPEC_PATH); | ||
logger.info("Reading REST spec from [{}]", restSpec); | ||
return restSpec; | ||
} | ||
|
||
@AfterClass | ||
public static void closeClient() throws IOException { | ||
try { | ||
|
@@ -184,7 +202,6 @@ public static void closeClient() throws IOException { | |
public static Iterable<Object[]> createParameters() throws Exception { | ||
return createParameters(ExecutableSection.XCONTENT_REGISTRY); | ||
} | ||
|
||
/** | ||
* Create parameters for this parameterized test. | ||
*/ | ||
|
@@ -233,11 +250,12 @@ public static Iterable<Object[]> createParameters(NamedXContentRegistry executea | |
return tests; | ||
} | ||
|
||
|
||
/** Find all yaml suites that match the given list of paths from the root test path. */ | ||
// pkg private for tests | ||
static Map<String, Set<Path>> loadSuites(String... paths) throws Exception { | ||
Map<String, Set<Path>> files = new HashMap<>(); | ||
Path root = PathUtils.get(ESClientYamlSuiteTestCase.class.getResource(TESTS_PATH).toURI()); | ||
Path root = PathUtils.get(ESClientYamlSuiteTestCase.class.getResource(TESTS_CLASSPATH).toURI()); | ||
for (String strPath : paths) { | ||
Path path = root.resolve(strPath); | ||
if (Files.isDirectory(path)) { | ||
|
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 probably don't want this logic to live in this task, for several reasons:
RestIntegTestTask
. As you can see in this scan, we end up copying this stuff 64 separate times. Presumably, all to the same location.RestIntegTestTask
whenever we want to run integration tests against an external cluster. Firstly, not all these are YAML tests, and second, not all projects that use YAML tests also necessarily need to full rest test suite. So essentially, we would be copying these rest specs all over the place for test suites that don't need them.Here's what I would suggest we do instead.
rest-test-spec.gradle
file in thegradle
folder that we then appy to the root build script viaappy from: 'gradle/rest-test-spect.gradle
.testRuntime project(path: ':', configuration: 'test-rest-spec')
and then all this stuff will magically be copied and added to the target projects test runtime classpath. No more copying to output directories.Please feel free to change any of these names to something that makes sense and reach out for questions.
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.
Thanks @mark-vieira, I exactly wasn't sure how to approach this (and still learning Gradle) so this helps alot.