-
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
[test] packaging: add java test project #29297
[test] packaging: add java test project #29297
Conversation
Adds behavior to the vagrant test plugin that runs jars from the packagingTest configuration inside test VMs. Makes :qa:vagrant a project which builds such a packaging test uberjar. This test project doesn't run anything yet. For elastic#26741
Jenkins test this please |
Pinging @elastic/es-core-infra |
description 'Clean the project build directory' | ||
group 'Build' | ||
delete project.buildDir | ||
if (project.tasks.findByName('clean') == null) { |
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.
++. Because if it has the java plugin this isn't needed.
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.
Yeah I figured it would be good to not couple it to the expectation that the project also has the java plugin applied
qa/vagrant/build.gradle
Outdated
packagingTest project(path: project.path, configuration: 'shadow') | ||
} | ||
|
||
shadowJar { |
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 wonder if it'd be simpler not to make an uber jar at all and instead execute a main class with classpath. I see that you iterate so you can run multiple jar files so you'd lose that. And you'd have to communicate the main class name to the plugin. I don't know if that is worth it just not to have to build the uber jar, but I think it is worth thinking about.
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.
Good point. The iterating over multiple jars thing isn't really super important, I just figured "all the jars in this directory get executed" is a simple way to reason about it. It also gives a flexible way (the packagingTest
configuration) to choose which test projects get included
Similarly, using an uberjar isn't necessarily the goal but I did want however it works to be easy to run by hand inside the test VM like we can with bats (since iterating through gradle on the host is time consuming)
I'll think about this more (also would like to hear thoughts from @rjernst on this)
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 would stay away from shading if we can. What about having the executable jar contain a classpath entry, and having a simple "test" script be generated which invokes the jar?
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'll give that a try. After thinking about it a little, dumping the test jars and all their dependencies into a directory and doing java -cp /project/packaging/tests MainClass
is still really simple
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.
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.
LGTM
jenkins please test this |
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 left a couple questions.
qa/vagrant/build.gradle
Outdated
testMainClass 'org.elasticsearch.packaging.PackagingMain' | ||
} | ||
|
||
tasks.test.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 does this use elasticsearch.build if all these tasks are just disabled? Should this just be a plain groovy project, or is there something else that is needed? If something else, please leave a comment in this file explaining the reasoning.
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 discussed this - I was able to get most of the precommit tasks passing, now only test and the license checks are disabled
qa/vagrant/build.gradle
Outdated
@@ -39,3 +54,18 @@ setupPackagingTest { | |||
expectedPlugins.setText(plugins.join('\n'), 'UTF-8') | |||
} | |||
} | |||
|
|||
esvagrant { | |||
testMainClass 'org.elasticsearch.packaging.PackagingMain' |
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.
Could it be simpler to use a junit runner instead of having to define our own main 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.
We discussed this and I changed it to use junit as an entrypoint, as we likely would have ended up reimplementing a bunch of junit's runners' features
Closing in favor of #30161 |
Adds behavior to the vagrant test plugin that runs jars from the
packagingTest configuration inside test VMs. Makes :qa:vagrant a project
which builds such a packaging test uberjar. This test project doesn't
run anything yet.
For #26741