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.

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

Introduce integration test source set for gradle based project #24064

Merged
merged 1 commit into from
Mar 8, 2022

Conversation

glefloch
Copy link
Member

@glefloch glefloch commented Mar 2, 2022

This introduce a new intTest source set (src/intTest/java) and a new quarkusIntTest task that execcute @QuarkusIntegrationTest.
This tasks depends on quarkusBuild to make sure the final artifact used by integration test is generated.

Next step will be:

  • Update code start to create the intTest sourceSet instead of native-test
  • Update nativeTest task to combine both sourceSet
  • Deprecate the native-test sourceSet

Close #23528
Close #22035

@quarkus-bot quarkus-bot bot added area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/documentation area/gradle Gradle area/testing labels Mar 2, 2022
@glefloch glefloch requested review from aloubyansky and geoand March 2, 2022 20:50
@glefloch
Copy link
Member Author

glefloch commented Mar 2, 2022

cc @edeandrea

@@ -498,7 +498,7 @@ private static Path determineBuildOutputDirectory(final URL url) {
if (url.getPath().endsWith("test-classes/")) {
// we have the maven test classes dir
return toPath(url).getParent();
} else if (url.getPath().endsWith("test/")) {
} else if (url.getPath().endsWith("test/") || url.getPath().endsWith("intTest/")) {
Copy link
Member

Choose a reason for hiding this comment

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

We should check whether what we are doing if url.getPath().endsWith("-tests.jar") will work for all the other cases. That could be a separate PR though.

@glefloch
Copy link
Member Author

glefloch commented Mar 4, 2022

@edeandrea is it ok for you? this introduce a new quarkusIntTest task which will run tests located in src/intTest/java.
The tasks depends on the artifact produced by quarkusBuild.
The quarkusIntTest task is not linked to any other task.

intTestTask.dependsOn(quarkusBuild);
intTestTask.setClasspath(intTestSourceSet.getRuntimeClasspath());
intTestTask.setTestClassesDirs(intTestSourceSet.getOutput().getClassesDirs());
});
Copy link
Contributor

@edeandrea edeandrea Mar 4, 2022

Choose a reason for hiding this comment

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

Do you need intTestTask.setShouldRunAfter(List.of(tasks.findByName(JavaPlugin.TEST_TASK_NAME))); as well?

----

This task depends on `quarkusBuild`. The final artifact will be produced before running tests.

Copy link
Contributor

@edeandrea edeandrea Mar 4, 2022

Choose a reason for hiding this comment

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

Does quarkusBuild also run the check task (or at a minimum the test task)? If so, maybe you should mention that the unit tests will also be executed?

Maybe not necessary, but just a thought.

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently quarkusBuild is not linked to any test task

Copy link
Contributor

Choose a reason for hiding this comment

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

Should it (or the intTest task) be?

Copy link
Member Author

Choose a reason for hiding this comment

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

I will add a dependency between quarkusIntTest and check

import javax.ws.rs.Produces;
import javax.ws.rs.core.MediaType;

import org.eclipse.microprofile.config.inject.ConfigProperty;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this import is necessary?

@@ -0,0 +1,20 @@
package org.acme;

import javax.inject.Inject;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this import is necessary?

.when().get("/hello")
.then()
.statusCode(200)
.body(is("Hello from Test"));
Copy link
Contributor

@edeandrea edeandrea Mar 4, 2022

Choose a reason for hiding this comment

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

I don't understand how this test passes? ExampleResource only has a single endpoint, /hello, which returns "Hello World"? Am I missing something obvious somewhere?

I see in src/test/resources/application.properties there are 2 properties, yet ExampleResource.java never references them.

This leads me to believe that simply running the intTest task does not trigger the check task (or at a minimum the test task). Should it? Should you ever have the case where your integration tests pass yet your unit tests fail?

.when().get("test-only")
.then()
.statusCode(200)
.body(is("Test only"));
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand how this test passes? Where is the test-only endpoint defined? Am I missing something obvious?

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed it.

public void shouldRunIntegrationTestAsPartOfBuild() throws Exception {
File projectDir = getProjectDir("it-test-basic-project");

BuildResult buildResult = runGradleWrapper(projectDir, "clean", "quarkusIntTest");
Copy link
Contributor

@edeandrea edeandrea Mar 4, 2022

Choose a reason for hiding this comment

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

See comment above re: should the intTest task trigger the check (or at a minimum test) task?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, I will add a dependency the check task.

@edeandrea
Copy link
Contributor

@glefloch I added some comments.

Also, should this also close #22035?

@gsmet
Copy link
Member

gsmet commented Mar 5, 2022

Is intTest used by other projects? It sounds very weird to me. Maybe it would be better? True question :).

@edeandrea
Copy link
Contributor

Are you talking about the source set name? Personally I think it is too short. I think the test task name is quarkusIntTest.

@glefloch
Copy link
Member Author

glefloch commented Mar 5, 2022

There isn't convention for this.
In documention they usually declare ˋsrc/integrationTest/java And use ˋintegrationTest task name. Maybe we should go with that to be as close as possible from docs.

@edeandrea
Copy link
Contributor

edeandrea commented Mar 5, 2022

integrationTest is just so long and wordy. My last 2 companies we used intTest and the test task name was also intTest, following the convention that the test task name matches the source set name.

@gsmet
Copy link
Member

gsmet commented Mar 5, 2022

Mkay. Personally, I never saw intTest being used but I'll let you guys do what you prefer given I don't use Gradle :).

@edeandrea
Copy link
Contributor

Actually I was just looking at one of my personal projects. I use intTest as the source set name (https://github.com/edeandrea/xjc-generation-gradle-plugin/blob/main/build.gradle.kts#L27) but integrationTest as the task name (https://github.com/edeandrea/xjc-generation-gradle-plugin/blob/main/build.gradle.kts#L100). I also make the integrationTest task run after the test task (https://github.com/edeandrea/xjc-generation-gradle-plugin/blob/main/build.gradle.kts#L105) & the check task depend on the integrationTest task (https://github.com/edeandrea/xjc-generation-gradle-plugin/blob/main/build.gradle.kts#L114).

@gsmet you should use it - it's a far better tool IMO :)

@glefloch
Copy link
Member Author

glefloch commented Mar 6, 2022

I updated the task name to match the name of the source set.
Looking at gradle documentation, there are both intTest and integrationTest.

integrationTest is longer but may be more relevant that intTest. it works too..

@aloubyansky, @geoand do you have a preference ?

@geoand
Copy link
Contributor

geoand commented Mar 6, 2022

I vote for integrationTest

@aloubyansky
Copy link
Member

Another thing is, so far, every command in the Quarkus plugin had quarkus prefix. Could integrationTest potentially conflict with other plugins? Of course quarkusInterationTest will make it even longer, which isn't great.

@glefloch
Copy link
Member Author

glefloch commented Mar 7, 2022

Yes this is a risk.

We can make the source set name configurable with a default value of ˋintegrationTest and use ˋquarkusIntTest as task name ?

@edeandrea
Copy link
Contributor

Another thing is, so far, every command in the Quarkus plugin had quarkus prefix. Could integrationTest potentially conflict with other plugins? Of course quarkusInterationTest will make it even longer, which isn't great.

I agree - keep it consistent. If every task the plugin introduces has the quarkus prefix then this probably should too. quarkusIntTest works for me.

Personally I like intTest over integrationTest as the source set name, but at the end of the day it doesn't really matter that much to me. If you all like integrationTest then so be it.

@glefloch
Copy link
Member Author

glefloch commented Mar 7, 2022

I adressed all comments, updated the sourceSet to integrationTest and the task name to quarkusIntTest

@edeandrea
Copy link
Contributor

Nice work!

@glefloch glefloch merged commit b718fba into quarkusio:main Mar 8, 2022
@quarkus-bot quarkus-bot bot added this to the 2.8 - main milestone Mar 8, 2022
@glefloch glefloch deleted the fix/23528 branch March 8, 2022 09:42
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Mar 8, 2022
@edeandrea
Copy link
Contributor

Hey @glefloch should the Using @QuarkusIntegrationTest section in the getting started testing guide include something about this new quarkusIntTest task?

Currently it just says

As a test annotated with @QuarkusIntegrationTest tests the result of the build, it should be run as part of the integration test suite - i.e. via the maven-failsafe-plugin if using Maven or an additional task if using Gradle. These tests will not work if run in the same phase as @QuarkusTest as Quarkus has not yet created the final artifact.

Maybe it should say something like

As a test annotated with @QuarkusIntegrationTest tests the result of the build, it should be run as part of the integration test suite - i.e. via the maven-failsafe-plugin if using Maven or the quarkusIntTest task if using Gradle. These tests will not work if run in the same phase as @QuarkusTest as Quarkus has not yet created the final artifact.

?

@glefloch
Copy link
Member Author

Right, I will add a note on this guide.

@rinmalavi
Copy link

Right, I will add a note on this guide.

Not sure if ' or the quarkusIntTest task if using Gradle.' is the note added.
Reading this discussion I would assume quarkusIntTest will run only @QuarkusIntegrationTest in integrationTest source set, but it runs everything.
I am guessing i should add something to gradle.build to make this work, or was this supposed to be out of the box.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/documentation area/gradle Gradle area/testing kind/enhancement New feature or request
Projects
None yet
6 participants