-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Smarter copying of the rest specs and tests #52114
Conversation
This PR addresses the uncessary copying of the rest specs and allows for better semantics for which specs and tests are copied. By default the rest specs will get copied if the project applies `elasticsearch.standalone-rest-test` or `esplugin` and the project has rest tests. If the project does not have rest tests, you can configure the project to copy the specs anyway with `restApiSpec { alwaysCopySpec true }`. This PR also removes the need for dozens of places where the x-pack specs were copied by supporting copying of the x-pack rest specs if the project starts with `:x-pack` and the preceeding applies. The plugin introduced here can also copy the rest tests to the local project through similar configuration. The new plugin allows a user to minimize the surface area of which rest specs are copyied. Per project can be configured to include only a subset of the specs (or tests) `restApiSpec { includeXpackSpec "ccr" }` through a custom plugin extention. Only copying the specs when actually needed and only copying the minimal set of specs should help with build cache hit rates since we can define only what is in use. Project level optimizations for build cache hit rates are not included with this PR. Also, with this PR you can no longer use the includePackaged flag on integTest task, instead you will need to configure the plugin extention `restApiSpec { copyCoreTests true }`. Support for copying the rest tests from an external JAR is also dropped. While techincally non-passive, I can not imaging why a plugin developer (or any user of that JAR) would need the rest tests accessible from our `integTest` task. (the tests are still aviable in jar) The following items are included in this PR: `apply plugin: 'elasticsearch.rest-api-spec-for-testing'` (applied by default to `elasticsearch.standalone-rest-test` and `esplugin`) ``` restApiSpec { includeCoreSpec "foo" //will include the core specs that start with foo includeXpackSpec "bar" //will include x-pack specs that start with bar copyCoreTests true //will copy the core rest tests to the local project includeCoreTests "foo" //will include the core tests that start with foo copyXpackTests true //will copy the x-pack rest tests to the local project includeXpackTests "bar" //will include the x-pack tests that start with bar alwaysCopySpec true //will always copy the specs (core and/or x-pack), even if no tests exist to to the local project } ```
@mark-vieira - would you mind taking a look at the direction of this PR. If you agree with the direction, I will add some tests, update some docs, and do some minor clean up. Of note that I am unsure about
However, I think this provides a nice compromise to keeping it to just work (for plugin devs) and separating the concerns. |
Pinging @elastic/es-core-infra (:Core/Infra/Build) |
@elasticmachine update branch |
Hold off on reviewing ... I am re-visiting lazy properties and trying pushing some of the conditional logic out of the plugin and into a task at execution time. The basic idea is the same (configuration via a custom extension applied via a plugin by default and conditionally copying a subset of files), but I think lazy properties is what this PR needs to push some of the work out of the configuration phase and avoid the |
I was just going to say this. For what you want the preferred pattern is to have the extension configure lazy properties that are "wired" up to the appropriate tasks. Essentially, since we don't need to know any of this stuff until those tasks actually execute, we are free to mutate those properties up until the point of execution. |
buildSrc/src/main/java/org/elasticsearch/gradle/test/RestApiSpecExtension.java
Outdated
Show resolved
Hide resolved
buildSrc/src/main/java/org/elasticsearch/gradle/test/RestApiSpecExtension.java
Outdated
Show resolved
Hide resolved
public class RestApiSpecForTestingPlugin implements Plugin<Project> { | ||
|
||
private static final Logger logger = Logging.getLogger(RestApiSpecForTestingPlugin.class); | ||
private static final String EXTENSION_NAME = "restApiSpec"; |
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.
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.
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.
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.
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.
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?
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.
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.
buildSrc/src/main/java/org/elasticsearch/gradle/test/RestApiSpecForTestingPlugin.java
Outdated
Show resolved
Hide resolved
buildSrc/src/main/java/org/elasticsearch/gradle/test/RestApiSpecForTestingPlugin.java
Outdated
Show resolved
Hide resolved
buildSrc/src/main/java/org/elasticsearch/gradle/test/RestApiSpecForTestingPlugin.java
Outdated
Show resolved
Hide resolved
buildSrc/src/main/java/org/elasticsearch/gradle/test/RestApiSpecForTestingPlugin.java
Outdated
Show resolved
Hide resolved
@mark-vieira thanks for you help so far! I have updated the PR to use lazy properties and a proper task that is configured via the plugin. I still have some more testing (manual and writing unit) to do, but I think this iteration is more Gradle correct. |
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.
I've left some bigger feedback items and questions. There are also a number of nit picky polish items that I think would be simpler for me to just do myself, either later, or in another PR.
buildSrc/src/main/java/org/elasticsearch/gradle/test/rest/CopyRestApiPlugin.java
Outdated
Show resolved
Hide resolved
buildSrc/src/main/java/org/elasticsearch/gradle/test/rest/CopyRestApiPlugin.java
Outdated
Show resolved
Hide resolved
/** | ||
* Returns true if any files with a .yml extension exist the test resources rest-api-spec/test directory (from source or output dir) | ||
*/ | ||
private boolean projectHasYamlRestTests() { |
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 should live in the plugin not in the task.
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.
Actually, thinking about this a bit more, I think this needs to stay in the task for a couple reasons:
-
This method checks against the source dir for tests committed to GH, but also checks the output directory for tests copied over by prior gradle tasks. This is why the spec task depends on the test task. Since this evaluation is done in the task at execution time, I believe this is a reliable way to ensure that any source or copied tests exist.
-
We can not actually skip the creation of the task based on the output of this. The spec copy task needs to execute if the project has no rest tests, but also you requested to include some tests, and the IIUC the extension property isn't guaranteed to be resolved at configuration time. e.g. since we can not read the property at configuration time we will always need to create the task in case there are no tests at all, but you need the spec (docs and HLRC are examples of this).
buildSrc/src/main/java/org/elasticsearch/gradle/test/rest/CopyRestApiTask.java
Outdated
Show resolved
Hide resolved
buildSrc/src/main/java/org/elasticsearch/gradle/test/rest/CopyRestApiTask.java
Outdated
Show resolved
Hide resolved
The backport PR seems to have been merged. I'm therefore removing the backport pending label here. Please shout if this is incorrect |
A recent PR #52114 introduced two new tasks to copy the REST api and tests. A couple bugs were found in that initial PR that prevents the incremental build from working as expected. The pattern match of empty string is equivalent to match all and it was coded as match none. Fixed with explicit checks against empty patterns. The fileCollection.plus return value was ignored. Fixed by changing how the input's fileTree is constructed. If a project has an src/test/resources directory, and tests are being copied without a rest-api-spec/test directory could result no-op. Masked by the other bugs and fixed by minor changes to logic to determine if a project has tests.
…#52860) A recent PR elastic#52114 introduced two new tasks to copy the REST api and tests. A couple bugs were found in that initial PR that prevents the incremental build from working as expected. The pattern match of empty string is equivalent to match all and it was coded as match none. Fixed with explicit checks against empty patterns. The fileCollection.plus return value was ignored. Fixed by changing how the input's fileTree is constructed. If a project has an src/test/resources directory, and tests are being copied without a rest-api-spec/test directory could result no-op. Masked by the other bugs and fixed by minor changes to logic to determine if a project has tests.
This PR addresses the unnecessary copying of the rest specs and allows for better semantics for which specs and tests are copied. By default the rest specs will get copied if the project applies `elasticsearch.standalone-rest-test` or `esplugin` and the project has rest tests or you configure the custom extension `restResources`. This PR also removes the need for dozens of places where the x-pack specs were copied by supporting copying of the x-pack rest specs too. The plugin/task introduced here can also copy the rest tests to the local project through a similar configuration. The new plugin/task allows a user to minimize the surface area of which rest specs are copied. Per project can be configured to include only a subset of the specs (or tests). Configuring a project to only copy the specs when actually needed should help with build cache hit rates since we can better define what is actually in use. However, project level optimizations for build cache hit rates are not included with this PR. Also, with this PR you can no longer use the includePackaged flag on integTest task. The following items are included in this PR: * new plugin: `elasticsearch.rest-resources` * new tasks: CopyRestApiTask and CopyRestTestsTask - performs the copy * new extension 'restResources' ``` restResources { restApi { includeCore 'foo' , 'bar' //will include the core specs that start with foo and bar includeXpack 'baz' //will include x-pack specs that start with baz } restTests { includeCore 'foo', 'bar' //will include the core tests that start with foo and bar includeXpack 'baz' //will include the x-pack tests that start with baz } } ```
…#52860) A recent PR elastic#52114 introduced two new tasks to copy the REST api and tests. A couple bugs were found in that initial PR that prevents the incremental build from working as expected. The pattern match of empty string is equivalent to match all and it was coded as match none. Fixed with explicit checks against empty patterns. The fileCollection.plus return value was ignored. Fixed by changing how the input's fileTree is constructed. If a project has an src/test/resources directory, and tests are being copied without a rest-api-spec/test directory could result no-op. Masked by the other bugs and fixed by minor changes to logic to determine if a project has tests.
Update documentation for: * restResources config (related elastic#52114) * call out YAML vs. Java based Rest tests * update example to use newer syntax * update example to target a test that is not skipped * provide example for bwcRest test (related elastic#52383)
Update documentation for: * restResources config (related elastic#52114) * call out YAML vs. Java based Rest tests * update example to use newer syntax * update example to target a test that is not skipped * provide example for bwcRest test (related elastic#52383)
A recent PR #52114 introduced two new tasks to copy the REST api and tests. A couple bugs were found in that initial PR that prevents the incremental build from working as expected. The pattern match of empty string is equivalent to match all and it was coded as match none. Fixed with explicit checks against empty patterns. The fileCollection.plus return value was ignored. Fixed by changing how the input's fileTree is constructed. If a project has an src/test/resources directory, and tests are being copied without a rest-api-spec/test directory could result no-op. Masked by the other bugs and fixed by minor changes to logic to determine if a project has tests.
A recent PR #52114 introduced two new tasks to copy the REST api and tests. A couple bugs were found in that initial PR that prevents the incremental build from working as expected. The pattern match of empty string is equivalent to match all and it was coded as match none. Fixed with explicit checks against empty patterns. The fileCollection.plus return value was ignored. Fixed by changing how the input's fileTree is constructed. If a project has an src/test/resources directory, and tests are being copied without a rest-api-spec/test directory could result no-op. Masked by the other bugs and fixed by minor changes to logic to determine if a project has tests.
This commit fixes ensures that for external builds (e.g. plugin development) that the REST tests that are copied are properly filtered to only include the API by default. The code prior to this change resulted in including both the API and tests since the copy.include resulted as an empty list by default since the stream is empty unless explictly configured. related elastic#52114
This commit fixes ensures that for external builds (e.g. plugin development) that the REST tests that are copied are properly filtered to only include the API by default. The code prior to this change resulted in including both the API and tests since the copy.include resulted as an empty list by default since the stream is empty unless explicitly configured. related #52114 fixes #53183
This commit fixes ensures that for external builds (e.g. plugin development) that the REST tests that are copied are properly filtered to only include the API by default. The code prior to this change resulted in including both the API and tests since the copy.include resulted as an empty list by default since the stream is empty unless explicitly configured. related elastic#52114 fixes elastic#53183
This commit fixes ensures that for external builds (e.g. plugin development) that the REST tests that are copied are properly filtered to only include the API by default. The code prior to this change resulted in including both the API and tests since the copy.include resulted as an empty list by default since the stream is empty unless explicitly configured. related elastic#52114 fixes elastic#53183
This commit fixes ensures that for external builds (e.g. plugin development) that the REST tests that are copied are properly filtered to only include the API by default. The code prior to this change resulted in including both the API and tests since the copy.include resulted as an empty list by default since the stream is empty unless explicitly configured. related #52114 fixes #53183
This commit fixes ensures that for external builds (e.g. plugin development) that the REST tests that are copied are properly filtered to only include the API by default. The code prior to this change resulted in including both the API and tests since the copy.include resulted as an empty list by default since the stream is empty unless explicitly configured. related #52114 fixes #53183
This should help with Gradle's incremental compile such that projects only depend upon the resources they use. related elastic#52114
This should help with Gradle's incremental compile such that projects only depend upon the resources they use. related #52114
) This should help with Gradle's incremental compile such that projects only depend upon the resources they use. related elastic#52114
This should help with Gradle's incremental compile such that projects only depend upon the resources they use. related #52114
This PR addresses the unnecessary copying of the rest specs and allows
for better semantics for which specs and tests are copied. By default
the rest specs will get copied if the project applies
elasticsearch.standalone-rest-test
oresplugin
and the projecthas rest tests or you configure the custom extension
restResources
.This PR also removes the need for dozens of places where the x-pack
specs were copied by supporting copying of the x-pack rest specs too.
The plugin/task introduced here can also copy the rest tests to the
local project through a similar configuration.
The new plugin/task allows a user to minimize the surface area of
which rest specs are copied. Per project can be configured to include
only a subset of the specs (or tests). Configuring a project to only
copy the specs when actually needed should help with build cache hit
rates since we can better define what is actually in use.
However, project level optimizations for build cache hit rates are
not included with this PR.
Also, with this PR you can no longer use the includePackaged flag on
integTest task.
The following items are included in this PR:
elasticsearch.rest-resources