Read the full REST spec from a common directory #51794
Conversation
jakelandis
added
:Delivery/Build
|
Pinging @elastic/es-core-infra (:Core/Infra/Build) |
| } | ||
|
|
||
| dependencies { | ||
| restSpec project(':rest-api-spec') |
There was a problem hiding this comment.
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 :rest-api-spec only exists in the Elasticsearch build.
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.
Understood, I think the refactoring you suggested will remove the need for this.
It's not even clear why this is necessary. Folks building plugins should not need to run our rest api tests.
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 :)
| @@ -69,9 +69,21 @@ class RestIntegTestTask extends DefaultTask { | |||
| runner.systemProperty('test.clustername', System.getProperty("tests.clustername")) | |||
| } | |||
|
|
|||
| // copy the rest spec/tests onto the test classpath | |||
| Copy copyRestSpec = createCopyRestSpecTask() | |||
| // copy the full rest spec (including modules, plugins, and x-pack) to a common location | |||
There was a problem hiding this comment.
We probably don't want this logic to live in this task, for several reasons:
- We shouldn't have any logic in task constructors to begin with. We actually have an existing issue to refactor this task. This pattern is a big Gradle no-no.
- We create lots of these tasks, across many projects, so we'll end up creating a set of copy tasks for each project which defines a
RestIntegTestTask. As you can see in this scan, we end up copying this stuff 64 separate times. Presumably, all to the same location. - All these copy tasks share the same copy destination which is going to cause all sorts of issues in parallel builds with concurrently executing copy tasks clobbering eachothers output.
- Gradle will actually issue warnings because of issue (3).
- We use
RestIntegTestTaskwhenever 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.
- Create a new plugin that we apply to the root project only to set all this stuff up. This ensure we only ever do this once. We could create this as a binary plugin but given the way we set this stuff up I'm inclined to start as a script plugin. That is. create a
rest-test-spec.gradlefile in thegradlefolder that we then appy to the root build script viaappy from: 'gradle/rest-test-spect.gradle. - Define the output of all these copy/unpack tasks as a build artifact. This will allow projects that need it to depend on it, which is the correct way of modeling this, and also ensures stuff gets built only if necessary, and in the correct order. We can actually declare a directory as an artifact (this is how compiled classes are depended on in fact), and you can see an example of that here.
- We then in projects that require this stuff, declare a dependency on this stuff. Since we just need this stuff on the test runtime classpath, we'd so something like
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.
Thanks @mark-vieira, I exactly wasn't sure how to approach this (and still learning Gradle) so this helps alot.
| * 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.
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 /rest-api-spec). We are now just having to set an additional system property for all our tests.
There was a problem hiding this comment.
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.
I'm a bit confused, are we adding rootBuildDir to the classpath? As far as I can tell we are still loading these things from the classpath, not from the filesystem, or did I miss something?
There was a problem hiding this comment.
to close the loop here...
no, the rootBuildDir was not added to the classpath, the api specs (not the tests) in this pr are read from the file system which is why the security policy change was required. The refactoring you suggested removes the need for this. Update coming soon.
| @@ -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.
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.
it is see comment above.
|
Closing this review in favor of #51890 (it was cleaner to just get a clean start) |
This commit changes where the REST API specs are read. The REST API
spec is required to run the REST YAML tests and are currently copied
to every project that has REST YAML tests.
The REST API specs are now copied to the build directory of the root
gradle project under a "rest-spec-api-current" directory. Since
each module or plugin (including all x-pack plugins) can contribute
to the REST API spec, they too are copied to the common directory.
The result is a single directory created during the build which
includes the full REST API spec. A system property has been
introduced to allow the testing framework to know where this common
directory lives on disk.
Additionally, the BWC based tests continue to copy the API
and tests locally since they are special cases of the REST
YAML testing.
Note - this change related (but not dependent) to how the REST testing strategy #51816 will work. For example, that testing plans to use a "rest-spec-api-prior" directory for it's REST specs.