Create plugin for yamlTest task #56841
Conversation
|
@elasticmachine test this please |
jakelandis
added
:Delivery/Build
|
Pinging @elastic/es-core-infra (:Core/Infra/Build) |
Why? The main motivator here is to make these things very explicit. The directory this stuff goes in dictates the kind of test. Supporting such a "hybrid" approach conflicts with that goal. In what scenario would a developer require these test live in the unit testing source set instead of the YAML test one? |
| @@ -36,6 +36,8 @@ | |||
| private static final String TESTS_CLUSTER = "tests.cluster"; | |||
| private static final String TESTS_CLUSTER_NAME = "tests.clustername"; | |||
|
|
|||
| // TODO: refactor this so that work is not done in constructor and find usages and register them, not create them | |||
| // See: https://docs.gradle.org/current/userguide/task_configuration_avoidance.html | |||
There was a problem hiding this comment.
FWIW, there's an open issue for this. Perhaps we can link it here.
| @@ -109,7 +111,8 @@ public FileTree getInputDir() { | |||
|
|
|||
| @OutputDirectory | |||
| public File getOutputDir() { | |||
| return new File(getTestSourceSet().getOutput().getResourcesDir(), REST_API_PREFIX); | |||
| assert Util.getTestSourceSet(getProject()).isPresent(); | |||
There was a problem hiding this comment.
We should use orElseThrow() here and supply an exception with an appropriate message.
There was a problem hiding this comment.
I added a message to the assert
EDIT: after testing .. the assert did not fire as expected, so removed it in favor of orElseThrow
| * @param project The project to look for test Java resources. | ||
| * @return An Optional that contains the Java test SourceSet if it exists. | ||
| */ | ||
| public static Optional<SourceSet> getTestSourceSet(Project project) { |
There was a problem hiding this comment.
As mentioned in my comment, I don't think we need to support this case. We should require that all YAML tests live in the appropriate source set. This awkward conditional stuff is a great example of why.
Also, what happens if a project has both tests types? Which source set should this method return? As part of splitting these types of tests up there is no longer a canonical "test source set", as any given project might (and likely will) have several.
There was a problem hiding this comment.
Discussed in person. Since this is used primarily for copy around of REST resources, I will remove this method in favor of having the REST resources be explicit about which source set they will use dependent on the application (or not) of the new YAML test task.
Also, this PR updated to use this with test conventions task, but that isn't needed for custom test source sets.
There will still be a todo to circle back around and remove the conditional aspect of which test source set is used for copying around REST resources, but that can only be done after all existing usages of the java test source set have been addressed. This includes ensuring that the clients teams won't break on moving tests around, as well all internal usages where YAML tests can live in the java test source set. While plugin developers may be non-passively impacted by requiring the new YAML plugin, the migration path is pretty simple and would only impact those that leverage YAML testing (suspected to be very few).
There was a problem hiding this comment.
done ... will add summary to main comment.
| @Override | ||
| public void apply(Project project) { | ||
|
|
||
| if (project.getPluginManager().hasPlugin("elasticsearch.build") == false |
There was a problem hiding this comment.
We shouldn't have to do this. Let's just apply the correct plugin here, which is going to be the ElasticsearchJavaPlugin most likely. We don't want to have users have to worry about this.
| yamlTestTask.setDescription("Runs the YAML based REST tests against an external cluster"); | ||
|
|
||
| // setup task dependency | ||
| project.getDependencies().add(SOURCE_SET_NAME + "Compile", project.project(":test:framework")); |
There was a problem hiding this comment.
For now we need to check if BuildParams.isInternal here and use an artifact dependency if not.
| runner.setTestClassesDirs(yamlTestSourceSet.getOutput().getClassesDirs()); | ||
| runner.setClasspath(yamlTestSourceSet.getRuntimeClasspath()); | ||
|
|
||
| // if this a module or plugin, it may have an associated zip file with it's contents, add that to the test cluster |
There was a problem hiding this comment.
Shouldn't the PluginBuildPlugin be doing this?
There was a problem hiding this comment.
I'm not sure I follow ?
This section of code is the magic that adds only the required modules/plugins to the test clusters for the OSS YAML tests. This is mostly a copy for how it currently works.
| GradleUtils.setupIdeForTestSourceSet(project, yamlTestSourceSet); | ||
|
|
||
| // run test tasks first if they exist since they are presumably faster and less resources intensive | ||
| Task testTask = project.getTasks().findByName("test"); |
There was a problem hiding this comment.
Let's remove this. I don't think it should go here, plus using findByName is going to eagerly realize the task.
There was a problem hiding this comment.
updated it to be inline with #57016
| } | ||
|
|
||
| // validation of the rest specification is wired to precommit, so ensure that runs first | ||
| yamlTestTask.mustRunAfter(project.getTasks().getByName("precommit")); |
There was a problem hiding this comment.
What's creating the precommit task in this scenario? Is it some other plugin we're applying?
There was a problem hiding this comment.
Note #56926 will make this easier, you shouldn't have to worry about it. Currently that we already have:
project.getTasks().named(JavaPlugin.TEST_TASK_NAME).configure(t -> t.mustRunAfter(precommit));
But we can change that to all Test tasks.
There was a problem hiding this comment.
I removed this line since #56926 addressed all Test tasks
Two reasons:
For both cases I can follow up with removal for 8.0 only. |
I don't think we need to wait until 8.0. We have been very explicit to plugin developers that there can be breaks in minor versions. |
Some standalone projects such as :distribution:archives:integ-test-zip have tests but are not intended to ever create a jar. Applying the new ElasticsearchJavaPlugin in favor over the elder "Standalone*Plugin" or BuildPlugin can result in these non-jar projects erroring due to the lack of the license or notice file. To avoid creating more permutations of ElasticsearchJavaPlugin (i.e. ElasticsearchJavaPluginWithoutJar), the conuming project should be able disable the jar task (e.g. `jar.enabled = false`) and the license checks should be skipped as well as the javadoc and sources jars. related: elastic#56841
) (#60026) Introduce a javaRestTest source set and task to compliment the yamlRestTest. javaRestTest differs such that the code is sourced from Java and may have different dependencies and setup requirements for the test clusters. This also allows the tests to run in parallel in different cluster instances to prevent any cross test contamination between the two types of tests. Included in this PR is all :modules no longer use the integTest task. The tests are now driven by test, yamlRestTest, javaRestTest, and internalClusterTest. Since only :modules (and :rest-api-spec) have been converted to yamlRestTest we can now disable the integTest task if either yamlRestTest or javaRestTest have been applied. Once all projects are converted, we can delete the integTest task. related: #56841 related: #59444
…nternalClusterTest (#59444) For all OSS plugins (except repository-* and discovery-*) integTest task is now a no-op and all of the tests are now executed via a test, yamlRestTest, javaRestTest, or internalClusterTest. This commit does NOT convert the discovery-* and repository-* since they are bit more complex then the rest of tests and this PR is large enough. Those plugins will be addressed in a future PR(s). This commit also fixes a minor issue that did not copy the rest api for projects that only had YAML TEST tests. related: #56841
…nternalClusterTest (elastic#59444) For all OSS plugins (except repository-* and discovery-*) integTest task is now a no-op and all of the tests are now executed via a test, yamlRestTest, javaRestTest, or internalClusterTest. This commit does NOT convert the discovery-* and repository-* since they are bit more complex then the rest of tests and this PR is large enough. Those plugins will be addressed in a future PR(s). This commit also fixes a minor issue that did not copy the rest api for projects that only had YAML TEST tests. related: elastic#56841 # Conflicts: # plugins/examples/painless-whitelist/build.gradle # plugins/examples/security-authorization-engine/build.gradle
…alClusterTest (elastic#60084) For OSS plugins that begin with discovery-*, the integTest task is now a no-op and all of the tests are now executed via a test, yamlRestTest, javaRestTest, or internalClusterTest. related: elastic#56841 related: elastic#59444
…nalClusterTest (elastic#60085) For OSS plugins that being with repository-*, integTest task is now a no-op and all of the tests are now executed via a test, yamlRestTest, javaRestTest, or internalClusterTest. related: elastic#56841 related: elastic#59444
…t or internalClusterTest (#59444) (#60343) For all OSS plugins (except repository-* and discovery-*) integTest task is now a no-op and all of the tests are now executed via a test, yamlRestTest, javaRestTest, or internalClusterTest. This commit does NOT convert the discovery-* and repository-* since they are bit more complex then the rest of tests and this PR is large enough. Those plugins will be addressed in a future PR(s). This commit also fixes a minor issue that did not copy the rest api for projects that only had YAML TEST tests. related: #56841
…est or internalClusterTest (#60630) For 1/2 the plugins in x-pack, the integTest task is now a no-op and all of the tests are now executed via a test, yamlRestTest, javaRestTest, or internalClusterTest. This includes the following projects: async-search, autoscaling, ccr, enrich, eql, frozen-indicies, data-streams, graph, ilm, mapper-constant-keyword, mapper-flattened, ml A few of the more specialized qa projects within these plugins have not been changed with this PR due to additional complexity which should be addressed separately. A follow up PR will address the remaining x-pack plugins (this PR is big enough as-is). related: #61802 related: #56841 related: #59939 related: #55896
…Test or internalClusterTest (#61802) For 1/2 the plugins in x-pack, the integTest task is now a no-op and all of the tests are now executed via a test, yamlRestTest, javaRestTest, or internalClusterTest. This includes the following projects: security, spatial, stack, transform, vecotrs, voting-only-node, and watcher. A few of the more specialized qa projects within these plugins have not been changed with this PR due to additional complexity which should be addressed separately. related: #60630 related: #56841 related: #59939 related: #55896
…est or internalClusterTest (elastic#60630) For 1/2 the plugins in x-pack, the integTest task is now a no-op and all of the tests are now executed via a test, yamlRestTest, javaRestTest, or internalClusterTest. This includes the following projects: async-search, autoscaling, ccr, enrich, eql, frozen-indicies, data-streams, graph, ilm, mapper-constant-keyword, mapper-flattened, ml A few of the more specialized qa projects within these plugins have not been changed with this PR due to additional complexity which should be addressed separately. A follow up PR will address the remaining x-pack plugins (this PR is big enough as-is). related: elastic#61802 related: elastic#56841 related: elastic#59939 related: elastic#55896 # Conflicts: # x-pack/plugin/identity-provider/src/internalClusterTest/java/org/elasticsearch/xpack/idp/saml/sp/SamlServiceProviderIndexTests.java # x-pack/plugin/ilm/qa/with-security/build.gradle
…Test or internalClusterTest (elastic#61802) For 1/2 the plugins in x-pack, the integTest task is now a no-op and all of the tests are now executed via a test, yamlRestTest, javaRestTest, or internalClusterTest. This includes the following projects: security, spatial, stack, transform, vecotrs, voting-only-node, and watcher. A few of the more specialized qa projects within these plugins have not been changed with this PR due to additional complexity which should be addressed separately. related: elastic#60630 related: elastic#56841 related: elastic#59939 related: elastic#55896 # Conflicts: # x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/integration/IndexPrivilegeIntegTests.java # x-pack/plugin/security/src/test/java/org/elasticsearch/integration/IndexPrivilegeIntegTests.java # x-pack/plugin/security/src/test/java/org/elasticsearch/integration/IndexPrivilegeTests.java # x-pack/plugin/transform/qa/multi-node-tests/build.gradle # x-pack/plugin/transform/qa/single-node-tests/build.gradle # x-pack/plugin/watcher/build.gradle
…]RestTest or internalClusterTest (#60630) (#61855) For 1/2 the plugins in x-pack, the integTest task is now a no-op and all of the tests are now executed via a test, yamlRestTest, javaRestTest, or internalClusterTest. This includes the following projects: async-search, autoscaling, ccr, enrich, eql, frozen-indicies, data-streams, graph, ilm, mapper-constant-keyword, mapper-flattened, ml A few of the more specialized qa projects within these plugins have not been changed with this PR due to additional complexity which should be addressed separately. A follow up PR will address the remaining x-pack plugins (this PR is big enough as-is). related: #61802 related: #56841 related: #59939 related: #55896
…a]RestTest or internalClusterTest (#61802) (#61856) For 1/2 the plugins in x-pack, the integTest task is now a no-op and all of the tests are now executed via a test, yamlRestTest, javaRestTest, or internalClusterTest. This includes the following projects: security, spatial, stack, transform, vecotrs, voting-only-node, and watcher. A few of the more specialized qa projects within these plugins have not been changed with this PR due to additional complexity which should be addressed separately. related: #60630 related: #56841 related: #59939 related: #55896
This commit removes `integTest` task from all es-plugins. Most relevant projects have been converted to use yamlRestTest, javaRestTest, or internalClusterTest in prior PRs. A few projects needed to be adjusted to allow complete removal of this task * x-pack/plugin - converted to use yamlRestTest and javaRestTest * plugins/repository-hdfs - kept the integTest task, but use `rest-test` plugin to define the task * qa/die-with-dignity - convert to javaRestTest * x-pack/qa/security-example-spi-extension - convert to javaRestTest * multiple projects - remove the integTest.enabled = false (yay!) related: #61802 related: #60630 related: #59444 related: #59089 related: #56841 related: #59939 related: #55896
This commit removes `integTest` task from all es-plugins. Most relevant projects have been converted to use yamlRestTest, javaRestTest, or internalClusterTest in prior PRs. A few projects needed to be adjusted to allow complete removal of this task * x-pack/plugin - converted to use yamlRestTest and javaRestTest * plugins/repository-hdfs - kept the integTest task, but use `rest-test` plugin to define the task * qa/die-with-dignity - convert to javaRestTest * x-pack/qa/security-example-spi-extension - convert to javaRestTest * multiple projects - remove the integTest.enabled = false (yay!) related: elastic#61802 related: elastic#60630 related: elastic#59444 related: elastic#59089 related: elastic#56841 related: elastic#59939 related: elastic#55896 # Conflicts: # qa/die-with-dignity/src/javaRestTest/java/org/elasticsearch/qa/die_with_dignity/DieWithDignityIT.java # x-pack/qa/security-example-spi-extension/build.gradle
This commit removes `integTest` task from all es-plugins. Most relevant projects have been converted to use yamlRestTest, javaRestTest, or internalClusterTest in prior PRs. A few projects needed to be adjusted to allow complete removal of this task * x-pack/plugin - converted to use yamlRestTest and javaRestTest * plugins/repository-hdfs - kept the integTest task, but use `rest-test` plugin to define the task * qa/die-with-dignity - convert to javaRestTest * x-pack/qa/security-example-spi-extension - convert to javaRestTest * multiple projects - remove the integTest.enabled = false (yay!) related: #61802 related: #60630 related: #59444 related: #59089 related: #56841 related: #59939 related: #55896
mark-vieira
added
Team:Delivery
This commit creates a new Gradle plugin to provide a separate task name
and source set for running YAML based REST tests. The only project
converted to use the new plugin in this PR is distribution/archives/integ-test-zip.
For which the testing has been moved to :rest-api-spec since it makes the most
sense and it avoids a small but awkward change to the distribution plugin.
The remaining cases in modules, plugins, and x-pack will be handled in followups.
This plugin is distinctly different from the plugin introduced in #55896 since
the YAML REST tests are intended to be black box tests over HTTP. As such they
should not (by default) have access to the classpath for that which they are testing.
The YAML based REST tests will be moved to separate source sets (yamlRestTest).
The which source is the target for the test resources is dependent on if this
new plugin is applied. If it is not applied, it will default to the test source
set.
Further, this introduces a breaking change for plugin developers that
use the YAML testing framework. They will now need to either use the new source set
and matching task, or configure the rest resources to use the old "test" source set that
matches the old integTest task. (The former should be preferred).
As part of this change (which is also breaking for plugin developers) the
rest resources plugin has been removed from the build plugin and now requires
either explicit application or application via the new YAML REST test plugin.
Plugin developers should be able to fix the breaking changes to the YAML tests
by adding
apply plugin: 'elasticsearch.yaml-rest-test'and moving the YAML testsunder a
yamlRestTestfolder (instead oftest)