Support for additional test-to-main directory mappings #11142

roguexz · 2020-08-01T05:31:10Z

It is common practice for Gradle projects to have more than one source set as part of the project setup. For example,

src/
  main/java
  test/java
  funcTest/java
  integTest/java

Currently Quarkus's PathTestHelper does not allow developers to include Quarkus tests in to any of these other locations. This commit proposes a change which will allow supplying the additional source sets to the Quarkus test framework.

Once approved, the subsequent change would be to the Gradle plugin where we can determine the various source sets and supply them as environment variables.

E.g.,
Sample build.gradle snippet.

sourceSets {
    // Functional Tests
    functionalTest {
        java.srcDir file('src/funcTest/java')
        resources.srcDir file('src/funcTest/resources')

        compileClasspath += sourceSets.main.output + configurations.testRuntimeClasspath + configurations.compileOnly
        runtimeClasspath += output + compileClasspath + configurations.jacocoAnt
    }
}

/*
 * Functional tests for this module.
 */
task functionalTest(type: Test, description: 'Functional tests', dependsOn: [processResources, classes]) {
    description = 'Run the functional tests.'
    group = 'verification'
    testClassesDirs = sourceSets.functionalTest.output.classesDirs
    classpath = sourceSets.functionalTest.runtimeClasspath
}

test {
    SourceSet mainSS = project.sourceSets.main
    String mainRelativePath = project.buildDir.relativePath(mainSS.output.classesDirs.singleFile)
    String additionalMappings = project.sourceSets.findAll { ss -> ss != mainSS }
            .collect { SourceSet ss ->
                project.buildDir.relativePath(ss.output.classesDirs.singleFile) + ':' + mainRelativePath
            }.join(',')
    environment 'ADDITIONAL_TEST_TO_MAIN_MAPPINGS', additionalMappings
}

roguexz · 2020-08-01T06:51:10Z

@aloubyansky

Here is a reproducer: https://github.com/roguexz/quarkus-sample

Clone the latest commit
Execute ./gradlew clean functionalTest

geoand · 2020-08-01T13:57:05Z

cc @glefloch

roguexz · 2020-08-09T20:53:42Z

Gentlemen .. any suggestions regarding this proposal?

aloubyansky · 2020-08-10T15:21:04Z

One reason I hesitated to comment last week was that I wanted to check how far we can go with the workspace discovery in Gradle. I suspect we can discover the sources w/o the externally configured environment variable which if we add now we'll have to support later (at least for some time).
I'm still busy looking into other critical issues, so couldn't investigate this further. But if you would like to dig into this @roguexz that could be very useful. Basically, this interface needs to be extended with test sources (and output) dirs https://github.com/quarkusio/quarkus/blob/master/independent-projects/bootstrap/gradle-resolver/src/main/java/io/quarkus/bootstrap/resolver/model/WorkspaceModule.java

glefloch · 2020-08-10T15:32:42Z

Yes I agree with @aloubyansky, I think we can discover using the Gradle tooling API.

roguexz · 2020-08-11T19:45:50Z

Let me try this out. I think I had reviewed this code earlier, but let me see what I can do with it. There is always the catch of identifying a sourceSet that should be included as part of the test sources.

gsmet · 2020-09-18T22:02:43Z

@glefloch what's the status of this?

glefloch · 2020-09-19T05:27:35Z

As suggested by @aloubyansky we should by able to do that using the quarkus model instead of having to set an environment variable.
@roguexz were you able to give it a try?

roguexz · 2020-09-21T00:53:41Z

I was tied up with other activities. Just rebased my code. Let me see if I can send a different test sources directory depending on the task being executed.

roguexz · 2020-09-21T02:52:14Z

test-framework/common/src/main/java/io/quarkus/test/common/PathTestHelper.java

@@ -99,6 +100,20 @@
                File.separator + "test-classes",
                File.separator + "classes");
        //endregion
+
+        String mappings = System.getenv("ADDITIONAL_TEST_TO_MAIN_MAPPINGS");


@glefloch Based on my analysis, if we want to keep the code impact to a minimum, then I will need to do the following,

Identify the test sources and serialize it as part of the plugin (via the tooling API)

Read that information in this class and associate the test-to-main mapping so that isInTestDir returns true

If this approach sounds good, then I can serialize it as part of the QuarkusPluginExtension.beforeTest() method.

Apart from the Gradle extension, is there any additional place that needs to be modified?

Yes, actually, we are already scanning workspace here:

quarkus/devtools/gradle/src/main/java/io/quarkus/gradle/builder/QuarkusModelBuilder.java

Line 126 in 77b772c

private WorkspaceModule getWorkspaceModule(Project project, LaunchMode mode) {

If you can add those source set here, you will be able to get them through the model used :

quarkus/test-framework/junit5/src/main/java/io/quarkus/test/junit/QuarkusTestExtension.java

Line 199 in 77b772c

BuildToolHelper.enableGradleAppModel(projectRoot, "TEST", QuarkusModelHelper.TEST_REQUIRED_TASKS);

roguexz · 2020-09-21T09:23:53Z

@glefloch - looks like your comment and my code push happened at the same time.

I am looking at the codepaths that you mentioned and am a little lost. PathTestHelper is explicitly mapping test sources to the main sources. I am trying to include an additional test source set.

According to what I understand from the code in QuarkusModelBuilder, we can include additional files (like the test fixtures) to the model (derived from src/main/java). I am not seeing how we can specify test sources through this model, unless we make a change to WorkspaceModule to include something like a testSourceSet and make PathTestHelper to make use of it.

Additionally, with this approach we will end up adding all test source locations upfront, similar to what is being done right now in PathTestHelper. My latest change ensures that we are associating the most likely sourceSet that is appropriate for the current Test task that is being executed.

So, I have the logic .. I need some guidance to identify the correct approach for pushing this mapping to the test helper.

roguexz · 2020-09-22T16:21:45Z

@glefloch Should we take this conversation to Zulip?

roguexz · 2020-09-24T21:11:29Z

@aloubyansky

@glefloch & I discussed this issue. Guillaume said that he would look at why PathTestHelper does what it does, but the PR itself is ready for you also to review.

aloubyansky

I think it's ok until we figure out how to properly integrate the workspace model into the junit5 extension. Do you think you could add a test for this issue? Thanks @roguexz

devtools/gradle/src/main/java/io/quarkus/gradle/QuarkusPluginExtension.java

test-framework/common/src/main/java/io/quarkus/test/common/PathTestHelper.java

roguexz · 2020-09-26T01:25:46Z

Incorporating the feedback and updating the PR (with functional-tests)

roguexz · 2020-09-26T03:01:20Z

devtools/gradle/build.gradle

@@ -27,6 +27,17 @@ repositories {
    mavenCentral()
 }

+sourceSets {
+    // Functional Tests
+    funcTest {


I am verifying the capability as a functional test.

roguexz · 2020-09-26T03:01:55Z

devtools/gradle/build.gradle

+    systemProperty 'org.gradle.testkit.dir', file("${buildDir}/tmp/test-kit")
+}
+
+check.dependsOn functionalTest


The build task depends on the check task. So, mvn clean install will in-turn trigger the functional tests

roguexz · 2020-09-26T03:06:17Z

devtools/gradle/src/funcTest/resources/additional-source-sets/build.gradle

+/*
+ * Functional tests for this module.
+ */
+task functionalTest(type: Test, description: 'Functional tests', dependsOn: [processResources, classes]) {


Apologies for the name confusion. Here is what's happening.

mvn clean install --executes--> gradle functionalTest --runs-> AdditionalSourceSetsTest --executes--> a Gradle build using this build file --executes--> this task (which happens to be named as "functionalTest")

roguexz · 2020-09-26T03:09:16Z

@aloubyansky @glefloch Do let me know if you have a different approach for the tests.

glefloch · 2020-09-26T05:37:00Z

@roguexz for this kind of feature, we usually create an integration test in integration-test/gradle module.
In test resources, you can add a sample Gradle project with different test directory and the test will basically run Gradle command on this sample project and assert command result.
Don't hesitate to ping me if you have any problem :)

glefloch

I commented the PR before starting a review. Your looks good. We used to have issues with test requiring the gradle plugin to be installed.
Can you move it to integration-test/gradle module?

devtools/gradle/src/funcTest/resources/additional-source-sets/build.gradle

roguexz · 2020-09-26T09:28:15Z

@glefloch
Relocated the tests to the right module. I have aligned the content to mimic what already exists. Since this is the first time I am contributing to this folder, can you confirm if everything looks fine?

Here are the tests that I ran,

Ensure that the happy path works

./gradlew --no-daemon clean test --tests=AdditionalSourceSetsTest

Update PathTestHelper to ignore the mapping and verify that the test fails with the familiar message test is not part of ...

I am assuming that the CI process should take care of verifying the overall sanity of this PR.

devtools/gradle/src/main/java/io/quarkus/gradle/QuarkusPluginExtension.java

aloubyansky · 2020-09-28T14:48:15Z

@roguexz Looks good, thanks! Could you please squash the commits and perhaps review the suggestion from @ebramirez?

roguexz · 2020-09-28T15:42:50Z

@roguexz Looks good, thanks! Could you please squash the commits and perhaps review the suggestion from @ebramirez?

Sure thing. Will do that shortly

roguexz · 2020-09-28T17:06:48Z

Done.

Incorporated the changes
Squashed all commits
Did a sanity check on my end.

* Determine test output directory based on the task * Raise an exception if the mapping data is invalid. * Include a functional-test for verifying the plugin behavior

boring-cyborg bot added the area/testing label Aug 1, 2020

jaikiran requested a review from aloubyansky August 1, 2020 06:14

roguexz force-pushed the master branch 2 times, most recently from 87e2d73 to b49c99f Compare September 21, 2020 02:43

roguexz commented Sep 21, 2020

View reviewed changes

aloubyansky reviewed Sep 25, 2020

View reviewed changes

devtools/gradle/src/main/java/io/quarkus/gradle/QuarkusPluginExtension.java Outdated Show resolved Hide resolved

test-framework/common/src/main/java/io/quarkus/test/common/PathTestHelper.java Outdated Show resolved Hide resolved

roguexz force-pushed the master branch from a86ec6f to f4f780f Compare September 26, 2020 02:32

roguexz commented Sep 26, 2020

View reviewed changes

glefloch reviewed Sep 26, 2020

View reviewed changes

devtools/gradle/src/funcTest/resources/additional-source-sets/build.gradle Outdated Show resolved Hide resolved

roguexz force-pushed the master branch from a23a867 to c9fe140 Compare September 26, 2020 18:53

ebramirez reviewed Sep 28, 2020

View reviewed changes

devtools/gradle/src/main/java/io/quarkus/gradle/QuarkusPluginExtension.java Outdated Show resolved Hide resolved

roguexz force-pushed the master branch from c9fe140 to baf6ebb Compare September 28, 2020 17:05

Gradle: Support for additional test-to-main directory mappings

48fa25b

* Determine test output directory based on the task * Raise an exception if the mapping data is invalid. * Include a functional-test for verifying the plugin behavior

roguexz force-pushed the master branch from baf6ebb to 48fa25b Compare September 28, 2020 17:52

aloubyansky approved these changes Sep 28, 2020

View reviewed changes

glefloch approved these changes Sep 28, 2020

View reviewed changes

aloubyansky merged commit 3676442 into quarkusio:master Sep 29, 2020

gsmet added this to the 1.9.0 - master milestone Oct 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for additional test-to-main directory mappings #11142

Support for additional test-to-main directory mappings #11142

roguexz commented Aug 1, 2020

roguexz commented Aug 1, 2020

geoand commented Aug 1, 2020

roguexz commented Aug 9, 2020

aloubyansky commented Aug 10, 2020

glefloch commented Aug 10, 2020

roguexz commented Aug 11, 2020

gsmet commented Sep 18, 2020

glefloch commented Sep 19, 2020

roguexz commented Sep 21, 2020

roguexz Sep 21, 2020

glefloch Sep 21, 2020

roguexz commented Sep 21, 2020

roguexz commented Sep 22, 2020

roguexz commented Sep 24, 2020

aloubyansky left a comment

roguexz commented Sep 26, 2020

roguexz Sep 26, 2020

roguexz Sep 26, 2020

roguexz Sep 26, 2020

roguexz commented Sep 26, 2020

glefloch commented Sep 26, 2020

glefloch left a comment

roguexz commented Sep 26, 2020

aloubyansky commented Sep 28, 2020

roguexz commented Sep 28, 2020

roguexz commented Sep 28, 2020

Support for additional test-to-main directory mappings #11142

Support for additional test-to-main directory mappings #11142

Conversation

roguexz commented Aug 1, 2020

roguexz commented Aug 1, 2020

geoand commented Aug 1, 2020

roguexz commented Aug 9, 2020

aloubyansky commented Aug 10, 2020

glefloch commented Aug 10, 2020

roguexz commented Aug 11, 2020

gsmet commented Sep 18, 2020

glefloch commented Sep 19, 2020

roguexz commented Sep 21, 2020

roguexz Sep 21, 2020

Choose a reason for hiding this comment

glefloch Sep 21, 2020

Choose a reason for hiding this comment

roguexz commented Sep 21, 2020

roguexz commented Sep 22, 2020

roguexz commented Sep 24, 2020

aloubyansky left a comment

Choose a reason for hiding this comment

roguexz commented Sep 26, 2020

roguexz Sep 26, 2020

Choose a reason for hiding this comment

roguexz Sep 26, 2020

Choose a reason for hiding this comment

roguexz Sep 26, 2020

Choose a reason for hiding this comment

roguexz commented Sep 26, 2020

glefloch commented Sep 26, 2020

glefloch left a comment

Choose a reason for hiding this comment

roguexz commented Sep 26, 2020

aloubyansky commented Sep 28, 2020

roguexz commented Sep 28, 2020

roguexz commented Sep 28, 2020