-
Notifications
You must be signed in to change notification settings - Fork 25.2k
Convert most OSS plugins from integTest to [yaml | java]RestTest or internalClusterTest #59444
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
Changes from 27 commits
376db42
1e56bb3
424fd5e
5fd84d2
aa96ed8
dfebc85
2be2c71
c603b80
bbc08f9
a9a34d9
5c75412
2fab21b
cc09550
cd76cf4
4a2d639
8682017
4a1d8c8
9414e3d
f2cf390
21cc84a
ebb6703
6147473
05e9421
885c84a
4ea875a
ea705a5
c89b5de
849d19d
ef34668
7b07432
8058400
1832e79
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,9 +16,8 @@ | |
* specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
apply plugin: 'elasticsearch.testclusters' | ||
apply plugin: 'elasticsearch.esplugin' | ||
apply plugin: 'elasticsearch.rest-resources' | ||
apply plugin: 'elasticsearch.yaml-rest-test' | ||
|
||
esplugin { | ||
name 'custom-settings' | ||
|
@@ -28,7 +27,8 @@ esplugin { | |
noticeFile rootProject.file('NOTICE.txt') | ||
} | ||
|
||
testClusters.integTest { | ||
testClusters.all { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason this needs to be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I figured the best example would be to use |
||
// Adds a setting in the Elasticsearch keystore before running the integration tests | ||
keystore 'custom.secured', 'password' | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,9 +18,9 @@ import org.elasticsearch.gradle.info.BuildParams | |
* specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
apply plugin: 'elasticsearch.testclusters' | ||
apply plugin: 'elasticsearch.esplugin' | ||
apply plugin: 'elasticsearch.rest-resources' | ||
apply plugin: 'elasticsearch.yaml-rest-test' | ||
apply plugin: 'elasticsearch.java-rest-test' | ||
|
||
esplugin { | ||
name 'rest-handler' | ||
|
@@ -34,21 +34,16 @@ esplugin { | |
test.enabled = false | ||
|
||
tasks.register("exampleFixture", org.elasticsearch.gradle.test.AntFixture) { | ||
dependsOn testClasses | ||
env 'CLASSPATH', "${-> project.sourceSets.test.runtimeClasspath.asPath}" | ||
dependsOn javaRestTestClasses | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done 849d19d |
||
env 'CLASSPATH', "${-> project.sourceSets.javaRestTest.runtimeClasspath.asPath}" | ||
executable = "${BuildParams.runtimeJavaHome}/bin/java" | ||
args 'org.elasticsearch.example.resthandler.ExampleFixture', baseDir, 'TEST' | ||
} | ||
|
||
integTest { | ||
javaRestTest { | ||
dependsOn exampleFixture | ||
runner { | ||
nonInputProperties.systemProperty 'external.address', "${-> exampleFixture.addressAndPort}" | ||
} | ||
} | ||
|
||
testingConventions.naming { | ||
IT { | ||
baseClass 'org.elasticsearch.test.ESTestCase' | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
apply plugin: 'elasticsearch.testclusters' | ||
apply plugin: 'elasticsearch.esplugin' | ||
apply plugin: 'elasticsearch.rest-resources' | ||
apply plugin: 'elasticsearch.java-rest-test' | ||
|
||
import org.elasticsearch.gradle.util.GradleUtils | ||
|
||
esplugin { | ||
name 'security-authorization-engine' | ||
|
@@ -11,19 +12,24 @@ esplugin { | |
noticeFile rootProject.file('NOTICE.txt') | ||
} | ||
|
||
// let the javaRestTest see the classpath of main | ||
GradleUtils.extendSourceSet(project, "main", "javaRestTest") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this necessary here but not done by default in the java rest test plugin? Since this is an example project we should use idiomatic patterns as much as possible. If this is required because of some kind of nasty test implementation we should refactor this. @rjernst thoughts? As I understand it a Java REST test should be no different than a YAML one in that the only interactions should be via the REST API, therefore there should be no need for plugin code itself to be on the test classpath. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The examples are not exactly idomatic to start with, they could use some updating. In this case the test really does need to see the main classes, which should probably be converted to a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I don't like the idea of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For this case I am able to simply add the dependency to the main runtimeClasspath. ef34668
|
||
|
||
dependencies { | ||
compileOnly "org.elasticsearch.plugin:x-pack-core:${versions.elasticsearch}" | ||
testImplementation "org.elasticsearch.client:elasticsearch-rest-high-level-client:${versions.elasticsearch}" | ||
javaRestTestImplementation "org.elasticsearch.plugin:x-pack-core:${versions.elasticsearch}" | ||
javaRestTestImplementation "org.elasticsearch.client:elasticsearch-rest-high-level-client:${versions.elasticsearch}" | ||
} | ||
|
||
integTest { | ||
//no unit tests | ||
test.enabled = false | ||
javaRestTest { | ||
dependsOn buildZip | ||
runner { | ||
systemProperty 'tests.security.manager', 'false' | ||
} | ||
} | ||
|
||
testClusters.integTest { | ||
testClusters.javaRestTest { | ||
setting 'xpack.security.enabled', 'true' | ||
setting 'xpack.ml.enabled', 'false' | ||
setting 'xpack.license.self_generated.type', 'trial' | ||
|
@@ -36,4 +42,3 @@ testClusters.integTest { | |
user role: 'custom_superuser' | ||
} | ||
|
||
check.dependsOn integTest |
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.
What's the purpose of this of this intermediary variable?
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.
it made sense when i was configuring multiple forbidden[name] ... but now not so much...will fix.
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 849d19d