Read the full REST spec from a common directory #51890

jakelandis · 2020-02-04T21:12:50Z

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. 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. Configuration is added to each project
that has a RestIntegTestTask to enable ensure that the common directory is
created when needed.

The numerous places where this spec was copied around has now been removed
in favor of the new common directory, which is the full spec and is part
of the test classpath.

Additionally, to enable plugin developers, a new plugin copy-rest-api-spec
has been introduced that allows the specs to be copied locally
from the jar. This is plugin is also used for the BWC tests which need
to copy the tests for the spec to the local resource directory.

Note - this review supersedes #51794

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. 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. Configuration is added to each project that has a RestIntegTestTask to enable ensure that the common directory is created when needed. The numerous places where this spec was copied around has now been removed in favor of the new common directory, which is the full spec and is part of the test classpath. Additionally, to enable plugin developers, a new plugin copy-rest-api-spec has been introduced that allows the specs to be copied locally from the jar. This is plugin is also used for the BWC tests which need to copy the tests for the spec to the local resource directory.

elasticmachine · 2020-02-04T21:13:47Z

Pinging @elastic/es-core-infra (:Core/Infra/Build)

jakelandis · 2020-02-04T21:43:46Z

working through the test failures they are mostly just need finish wiring up the spec dependencies

jakelandis · 2020-02-04T22:31:21Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/test/CopyRestApiSpecPlugin.groovy

+/**
+ * Copies the core REST specs to the local resource directory for the REST YAML tests.
+ */
+class CopyRestApiSpecPlugin implements Plugin<Project> {


We still have a few places where we need the specs copied to the local resources. Notably the examples so that they can rely on the release jar. There are few others projects where it is easier to copy them locally then run from the global dir. However, I believe all (but 1) of the modules, plugins, and x-pack plugins REST Yaml tests run from the global dir.

It's not clear to me what the difference is between just putting them on the test runtime classpath, and copying them to the resources output directory. Either way, we are just throwing these files on the classpath.

I think I may have mis-attributed "easier", I will re-visit the usage of this. However, this is still required for the examples project.

jakelandis · 2020-02-05T00:26:08Z

@elasticmachine update branch

mark-vieira · 2020-02-05T15:41:22Z

build.gradle

+  //copy the rest api spec to a global location and add to the test classpath
+  project.tasks.withType(RestIntegTestTask) {
+    configurations {
+      copyRestApiSpecGlobal


The consuming project doesn't need to create this configuration. When we mention this configuration below in the dependency notation, we are referring to the configuration on the root project.

mark-vieira · 2020-02-05T15:43:26Z

build.gradle

@@ -138,6 +141,15 @@ subprojects {
      precommit.dependsOn 'spotlessJavaCheck'
    }
  }
+  //copy the rest api spec to a global location and add to the test classpath
+  project.tasks.withType(RestIntegTestTask) {


I think this logic should move to the existing RestTestPlugin rather than shove it in a withType() block. This has the side effect of creating this dependency multiple times, once for each instance of that task type. This is benign, but also unnecessary.

The RestTestPlugin creates a integTest task of type RestIntegTestTask task. But so does the PluginBuildPlugin (esplugin) (not sure why it explicitly creates it instead of applying the RestTestPlugin).

Additionally, many of the projects create custom tasks of type RestIntegTestTask. The requirement for the RestTestRunnerTask which is created by the RestIntegTestTask.

Actually, I think this should actually be withType(RestTestRunnerTask) since a couple project create that directly.

We aren't configuring the task, we are adding a testRuntime dependency to the project. All test tasks inherit that classpath so there is no need to couple this to the existence of a RestIntegTestTask.

Based on your comment below

We should put the reliance on rest specs in an explicit plugin so that if you are using YAML tests and need those specs, you must apply the plugin.

I can move this to the RestTestPlugin

mark-vieira · 2020-02-05T16:31:37Z

gradle/copy-rest-api-spec-to-global.gradle

+ * Copies all of the REST api specs (including core, modules, plugins, and x-pack) to common location for the REST YAML test.
+ * This plugin is expected to only be applied to applied to the root project.
+ */
+File rootBuildDir = rootProject.buildDir


Ok, so I think we decided against centralizing all specs because I came late to the party with the whole cache hit issue. Sorry about that. Given that then the only common specs are those in :rest-api-spec, let's just add this logic to that project, exposing it's specs as a build artifact. Kinda what we already do ☹️

mark-vieira · 2020-02-05T16:32:41Z

gradle/copy-rest-api-spec-to-global.gradle

+  }
+}
+
+tasks.create("copyModulesRestSpecGlobal", Copy) {


So essentially this is the stuff you ripped out of RestIntegTestTask. I think that's still the right thing to do, but let's instead move it to RestTestPlugin. So the plugin sets up the specs on the classpath instead of the task constructor.

As mentioned above, there are many usages of RestIntegTestTask outside of the RestTestPlugin , so I can't move this to RestTestPlugin. I'll look into moving this back to the RestIntegTestTask , but not sure if that will just put us back to where we started.

Not all RestIntegTestTasks require the rest specs though, that's the difference. We use those task for Java tests as well. We should put the reliance on rest specs in an explicit plugin so that if you are using YAML tests and need those specs, you must apply the plugin. Otherwise we are adding the core rest specs to the classpath unnecessarily for many tasks, again, negatively affecting cacheability.

That makes sense... However, I would like to also remove the integTest from the esplugin (here) to help enforce this. I haven't checked yet, but for any esplugin that has REST tests, but does not have YAML based rest tests would need to include the specs unnecessarily or explicitly create the integTest task. I think this is pretty rare, so either option probably not much of an issue.

Thoughts on removing integTest from the esplugin to enforce the inclusion of RestTestPlugin for projects that have Rest tests ? This is technically a breaking change for plugin devs. (I am only targeting 8.0 anyway)

(I am only targeting 8.0 anyway)

We'll want to backport this to 7.x so I don't think we can safely make this change.

I haven't checked yet, but for any esplugin that has REST tests, but does not have YAML based rest tests would need to include the specs unnecessarily or explicitly create the integTest task. I think this is pretty rare, so either option probably not much of an issue.

I don't think so. There are a number of plugins that don't use YAML tests.

I think the problem is when we say "rest tests" we actually mean a few things. This includes both Java-based tests that target an external cluster and YAML tests. We only need the specs for the latter, but we group them together as a single thing. We need to tease these apart.

Not all projects that execute tests against an external cluster (i.e. have a RestIntegTesttask) utilize YAML tests. Pulling the copy rest spec stuff out of that task constructor is the first start. The question remains, where do we now put that logic, such that it get's applied to the appropriate projects. Moving it to RestTestPlugin actually wouldn't be correct, as this would apply again, to all projects. I suggest we create a new YamlRestTestPlugin that does all the spec copying and wiring and we apply this plugin explicitly to projects that leverage YAML tests.

mark-vieira · 2020-02-05T16:33:40Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/test/CopyRestApiSpecPlugin.groovy

+/**
+ * Copies the core REST specs to the local resource directory for the REST YAML tests.
+ */
+class CopyRestApiSpecPlugin implements Plugin<Project> {


It's not clear to me what the difference is between just putting them on the test runtime classpath, and copying them to the resources output directory. Either way, we are just throwing these files on the classpath.

jakelandis · 2020-02-09T23:10:35Z

Ok, so I think we decided against centralizing all specs because I came late to the party with the whole cache hit issue

I took this refactor a slightly different direction due that the potential to make the cache hits worse. So instead of trying to central all of the tests, I introduced a new plugin that still allows us to remove the logic from the integTest task, centralizes the copying, and provides more control over what is copied. Sorry (again) for the new PR, but given the change of direction it feels cleaner for a fresh start.

replaced by: #52114

jakelandis marked this pull request as ready for review February 4, 2020 21:13

jakelandis requested review from mark-vieira and rjernst February 4, 2020 21:13

jakelandis added :Delivery/Build Build or test infrastructure >refactoring v8.0.0 labels Feb 4, 2020

jakelandis mentioned this pull request Feb 4, 2020

Read the full REST spec from a common directory #51794

Closed

fix HLRC test

4266f67

jakelandis added 3 commits February 4, 2020 15:55

fix docker tests

8500ad7

fix xpack doc tests

df804c1

fix rolling upgrade

ec68975

jakelandis commented Feb 4, 2020

View reviewed changes

fix Xpack Rest Tests

5c3c36a

Merge branch 'master' into rest_spec_from_common_dir2

edf3dff

mark-vieira reviewed Feb 5, 2020

View reviewed changes

jakelandis closed this Feb 9, 2020

mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Read the full REST spec from a common directory #51890

Read the full REST spec from a common directory #51890

jakelandis commented Feb 4, 2020 •

edited

Loading

elasticmachine commented Feb 4, 2020

jakelandis commented Feb 4, 2020

jakelandis Feb 4, 2020 •

edited

Loading

mark-vieira Feb 5, 2020

jakelandis Feb 5, 2020

jakelandis commented Feb 5, 2020

mark-vieira Feb 5, 2020

mark-vieira Feb 5, 2020

jakelandis Feb 5, 2020

mark-vieira Feb 5, 2020

jakelandis Feb 5, 2020

mark-vieira Feb 5, 2020

mark-vieira Feb 5, 2020

jakelandis Feb 5, 2020

mark-vieira Feb 5, 2020

jakelandis Feb 5, 2020

mark-vieira Feb 5, 2020

mark-vieira Feb 5, 2020

jakelandis commented Feb 9, 2020

Read the full REST spec from a common directory #51890

Read the full REST spec from a common directory #51890

Conversation

jakelandis commented Feb 4, 2020 • edited Loading

elasticmachine commented Feb 4, 2020

jakelandis commented Feb 4, 2020

jakelandis Feb 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakelandis commented Feb 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakelandis commented Feb 9, 2020

jakelandis commented Feb 4, 2020 •

edited

Loading

jakelandis Feb 4, 2020 •

edited

Loading