-
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
Convert most OSS plugins from integTest to [yaml | java]RestTest or internalClusterTest #59444
Conversation
Pinging @elastic/es-core-infra (:Core/Infra/Build) |
plugins/analysis-icu/build.gradle
Outdated
"com.ibm.icu.text.Collator#getInstance() @ Don't use default locale, use getInstance(ULocale) instead" | ||
] | ||
tasks.withType(CheckForbiddenApis).configureEach { task -> | ||
if(task.name.contains("YamlRestTest") == false) { |
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 is needed since the yamlRestRest does not have access to that class
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.
Maybe this should be explicit, applying the signature to forbiddanApisMain
and forbiddenApisTest
, rather than all CheckForbiddenApis tasks? We are likely to continue adding more source sets in the future, so having configuration apply to all source sets would be problematic.
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.
👍 done 5c75412
(#59444) for the main source set (probably not the extra config noise for the test and internalTestCluster source sets)
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 just have a few comments
plugins/analysis-icu/build.gradle
Outdated
"com.ibm.icu.text.Collator#getInstance() @ Don't use default locale, use getInstance(ULocale) instead" | ||
] | ||
tasks.withType(CheckForbiddenApis).configureEach { task -> | ||
if(task.name.contains("YamlRestTest") == false) { |
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.
Maybe this should be explicit, applying the signature to forbiddanApisMain
and forbiddenApisTest
, rather than all CheckForbiddenApis tasks? We are likely to continue adding more source sets in the future, so having configuration apply to all source sets would be problematic.
@@ -33,6 +33,8 @@ restResources { | |||
} | |||
} | |||
|
|||
integTest.enabled = false |
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.
Why is this being disabled?
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 need a plan for ripping the creation of the integTest
task out from the PluginBuildPlugin
. Otherwise this is going to be necessary for any plugin project that uses only yaml tests. Or tweak our test conventions logic so it doesn't complain about this scenario.
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 found a need to implement a javaRestTest
as part of converting these plugins, so (without hacks) can't finish my conversion to yamlRestTest
.. Between javaRestTest
, yamlRestTest
, and internalClusterTest
there is no need for integTest
to exist anymore. I plan to rip out integTest
once all of these are implemented. It won't take long (but my attention is split among a few things at the moment).
plugins/analysis-nori/build.gradle
Outdated
@@ -32,6 +32,9 @@ restResources { | |||
includeCore '_common', 'indices', 'index', 'search' | |||
} | |||
} | |||
|
|||
integTest.enabled = false |
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.
Ditto. If we need to disable a test, can we have a comment explaining why it is disabled? Someone coming along in the future will have no reason whether this can/should be re-enabled.
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 isn't really disabling a test ... it is to avoid the testing conventions from complaining. I would prefer to tweak the testing conventions instead of a lot of redunant comments. I can do that in a separate PR (and remove these in this PR)
@rjernst @mark-vieira - #59939 has been submitted that introduces the javaRestTest plugin and converts the modules over. That PR also conditionally disables the |
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
…9939) 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: elastic#56841 related: elastic#59444
hold off on reviewing... now that #59939 has been merged and integrated here, there is still some more work on this PR. |
@elasticmachine update branch |
merge conflict between base and head |
) (#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 (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
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
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
, orinternalClusterTest
.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