Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Convert discovery-* from integTest to [yaml | java]RestTest or internalClusterTest #60084

Merged
merged 6 commits into from
Jul 28, 2020
Merged

Convert discovery-* from integTest to [yaml | java]RestTest or internalClusterTest #60084

merged 6 commits into from
Jul 28, 2020

Conversation

jakelandis
Copy link
Contributor

@jakelandis jakelandis commented Jul 22, 2020 •
edited
Loading

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: #56841
related: #59444

@jakelandis jakelandis marked this pull request as ready for review July 22, 2020 21:08
@jakelandis jakelandis added :Delivery/Build Build or test infrastructure v7.9.1 v8.0.0 labels Jul 22, 2020
@elasticmachine
Copy link
Collaborator

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

@elasticmachine elasticmachine added the Team:Core/Infra Meta label for core/infra team label Jul 22, 2020
mark-vieira
mark-vieira approved these changes Jul 22, 2020
View reviewed changes
Copy link
Contributor

@mark-vieira mark-vieira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple minor comments, otherwise LGTM.

This also is another reminder that we need to get rid of these gross ant fixtures.

plugins/discovery-azure-classic/build.gradle
@@ -88,9 +90,10 @@ task createKey(type: LoggedExec) {
'-keypass', 'keypass',
'-storepass', 'keypass'
}

//no unit tests
test.enabled = false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like one step forward and one step back. I assume this is also just to appease our testing conventions stuff? If so I feel like this should not be a failure.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup. However, it is a bit more rare to have no unit tests (at least until i start converting the qa projects). For this one we will probably want to toggle the enable/disable the test task if the directory exists or not. I can look into that for a follow up PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we are going to have this all over the place for the qa projects that contain only rest tests.

plugins/discovery-ec2/qa/amazon-ec2/build.gradle Outdated
@@ -66,19 +65,26 @@ integTest.enabled = false
*/
['KeyStore', 'EnvVariables', 'SystemProperties', 'ContainerCredentials', 'InstanceProfile'].forEach { action ->
AntFixture fixture = tasks.create(name: "ec2Fixture${action}", type: AntFixture) {
dependsOn compileTestJava
env 'CLASSPATH', "${-> project.sourceSets.test.runtimeClasspath.asPath}"
dependsOn compileYamlRestTestJava
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should really be dependsOn project.sourceSets.yamlRestTest.runtimeClasspath. Just compileYamlRestTestJava doesn't include a) processing resources or b) upstream runtime dependencies so we risk a race condition here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done 01114f9

plugins/discovery-gce/qa/gce/build.gradle Outdated
@@ -42,26 +40,26 @@ restResources {

/** A task to start the GCEFixture which emulates a GCE service **/
task gceFixture(type: AntFixture) {
dependsOn compileTestJava
env 'CLASSPATH', "${-> project.sourceSets.test.runtimeClasspath.asPath}"
dependsOn compileYamlRestTestJava
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, we should depending on the source set runtime classpath here, not just the compile task.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done 01114f9

@jakelandis
Copy link
Contributor Author

@elasticmachine update branch

@jakelandis jakelandis added v7.10.0 and removed v7.9.1 labels Jul 27, 2020
@jakelandis
Copy link
Contributor Author

@elasticmachine update branch

@jakelandis jakelandis merged commit 46342ac into elastic:master Jul 28, 2020
@jakelandis jakelandis deleted the yamlRestTest_plugins_discovery branch July 28, 2020 21:49
jakelandis added a commit to jakelandis/elasticsearch that referenced this pull request Jul 28, 2020
@jakelandis
Convert discovery-* from integTest to [yaml | java]RestTest or intern…
72b44a8 
…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
@jakelandis jakelandis mentioned this pull request Jul 28, 2020
Merged
jakelandis added a commit that referenced this pull request Jul 29, 2020
@jakelandis
[7.x] Convert discovery-* from integTest to [yaml | java]RestTest or …
f6abd67 
…internalClusterTest (#60084) (#60344)

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: #56841
related: #59444
@mark-vieira mark-vieira added Team:Delivery Meta label for Delivery team and removed Team:Core/Infra Meta label for core/infra team labels Nov 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mark-vieira mark-vieira mark-vieira approved these changes

@rjernst rjernst Awaiting requested review from rjernst

Assignees
No one assigned
Labels
:Delivery/Build Build or test infrastructure Team:Delivery Meta label for Delivery team v7.10.0 v8.0.0-alpha1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jakelandis @elasticmachine @mark-vieira