Add test sourceset support to MRJAR plugin #106571
Conversation
Mrjar plugin supports main sourcesets automatically configured with newer java versions. This commit adds support for test sourcesets as well. The key difference between main and test is that test sourcesets also extend their main counterparts.
|
Pinging @elastic/es-delivery (Team:Delivery) |
There was a problem hiding this comment.
I feel like I'm missing the bit where we configure the test task. How are these source sets making it on to the testing classpath? Also, should we have individual test tasks for each source set? How do we plan on conditionally handling tests that are applicable only to a particular runtime version?
| SourceSet sourceSet = javaExtension.getSourceSets().maybeCreate(sourcesetName); | ||
| for (String parentSourceSetName : parentSourceSets) { | ||
| GradleUtils.extendSourceSet(project, parentSourceSetName, sourcesetName); | ||
| List<Integer> testVersions = findSourceVersions(project, MRJAR_TEST_SOURCESET_PATTERN); |
There was a problem hiding this comment.
I don't think we need to do this twice. We can assume for every main source set, there is a corresponding test source set, and if it's empty that's fine the tasks will just get skipped with NO_SOURCE.
There was a problem hiding this comment.
Ah ok, didn't think about NO_SOURCE.
| String sourcesetName = SourceSet.TEST_SOURCE_SET_NAME + javaVersion; | ||
| if (mainVersions.contains(javaVersion)) { | ||
| // tests extend both their parent test source sets, and all of main sourcesets up to that version | ||
| testSourceSets.add(SourceSet.TEST_SOURCE_SET_NAME + javaVersion); |
There was a problem hiding this comment.
Should this be:
| testSourceSets.add(SourceSet.TEST_SOURCE_SET_NAME + javaVersion); | |
| testSourceSets.add(SourceSet.MAIN_SOURCE_SET_NAME + javaVersion); |
| var jarTask = project.getTasks().withType(Jar.class).named(JavaPlugin.JAR_TASK_NAME); | ||
| jarTask.configure(task -> { | ||
| task.into("META-INF/versions/" + javaVersion, copySpec -> copySpec.from(sourceSet.getOutput())); | ||
| for (var version : versions) { | ||
| SourceSet sourceSet = javaExtension.getSourceSets().maybeCreate(baseSourcesetName + version); |
There was a problem hiding this comment.
Why are we attempting to create a source set here? Shouldn't this already exist? We also should definitely not be registering source sets in a task configuration action.
There was a problem hiding this comment.
Oops, copy/paste issue, I'll fix
In my mind I was thinking this was automatic with sourceset creation. Makes sense though that they are independent. I'll add that. |
|
@mark-vieira I believe I addressed all your comments in 1b2d055 |
| testTask.dependsOn(jarTask); | ||
|
|
||
| SourceSetContainer sourceSets = GradleUtils.getJavaSourceSets(project); | ||
| FileCollection mainRuntime = sourceSets.getByName(SourceSet.MAIN_SOURCE_SET_NAME + javaVersion).getOutput(); |
There was a problem hiding this comment.
Don't we need to remove all the parent main source sets here. So if this was test23 we'd need to remove main21, main22 and main23 or else we'll hit jar hell.
There was a problem hiding this comment.
Of course, you are right. I've fixed this to loop over the main sourcesets up to that point.
|
|
||
| private void createTestTask(Project project, SourceSet sourceSet, int javaVersion) { | ||
| var jarTask = project.getTasks().withType(Jar.class).named(JavaPlugin.JAR_TASK_NAME); | ||
| project.getTasks().register(JavaPlugin.TEST_TASK_NAME + javaVersion, Test.class).configure(testTask -> { |
There was a problem hiding this comment.
We'll want to have the check task depend on this so that they actually get run.
| testTask.setClasspath(testRuntime.minus(mainRuntime).plus(project.files(jarTask))); | ||
|
|
||
| testTask.getJavaLauncher().set( | ||
| javaToolchains.launcherFor(spec -> { spec.getLanguageVersion().set(JavaLanguageVersion.of(javaVersion)); }) |
There was a problem hiding this comment.
nit: we can omit the curly braces in this lambda.
| var jarTask = project.getTasks().withType(Jar.class).named(JavaPlugin.JAR_TASK_NAME); | ||
| jarTask.configure(task -> { | ||
| task.into("META-INF/versions/" + javaVersion, copySpec -> copySpec.from(sourceSet.getOutput())); | ||
| for (var version : versions) { |
There was a problem hiding this comment.
Looking at this more, this feels a bit awkward. I feel like we should be doing this in addSourceSet instead.
Mrjar plugin supports main sourcesets automatically configured with newer java versions. This commit adds support for test sourcesets as well. The key difference between main and test is that test sourcesets also extend their main counterparts.