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

Look for test classes in the model loaded from the Gradle tooling #29808

Merged
merged 1 commit into from
Dec 14, 2022

Conversation

aloubyansky
Copy link
Member

With this change, when Quarkus tests are launched from an IDE for Gradle projects as "JUnit tests", test classes will be looked up in directories provided by the application model loaded with the Gradle tooling API.

This kind of class look up should be done for all cases, including Maven. I guess this could be the first step towards that refactoring.

@quarkus-bot quarkus-bot bot added area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/gradle Gradle area/testing labels Dec 12, 2022
@quarkus-bot

This comment has been minimized.

@gsmet gsmet requested a review from glefloch December 13, 2022 09:52
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

I haven't checked the code as it's probably something for @glefloch and @gastaldi but I spotted something that looks weird.

@@ -46,6 +46,7 @@ public QuarkusPluginExtension(Project project) {
}

public void beforeTest(Test task) {
project.getLogger().info("QuarkusPluginExtension.beforeTask " + task.getName());
Copy link
Member

Choose a reason for hiding this comment

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

Hmmm, do we want to keep that info log? It looks like a debug message.

Copy link
Member Author

Choose a reason for hiding this comment

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

The issue is that loading an app model this way is known to be slow. It could take 5 sec, for example, or even longer. I added it just for that reason and because it's only for launching from an IDE use-case.

Copy link
Member

Choose a reason for hiding this comment

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

Well, in that case, it should be a message that an end user can understand such as Loading the application model... or something similar?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, sorry! I thought of a different info message I added in another place. Yes, this is a leftover, thanks for spotting it!

Copy link
Contributor

@gastaldi gastaldi left a comment

Choose a reason for hiding this comment

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

LGTM, but I agree with @gsmet that the INFO message should be clearer

Copy link
Member

@glefloch glefloch left a comment

Choose a reason for hiding this comment

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

LGTM

@glefloch glefloch added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Dec 13, 2022
@quarkus-bot
Copy link

quarkus-bot bot commented Dec 13, 2022

Failing Jobs - Building 87eb67f

Status Name Step Failures Logs Raw logs
✔️ JVM Tests - JDK 11
✔️ JVM Tests - JDK 17
JVM Tests - JDK 17 MacOS M1 Set up runner ⚠️ Check → Logs Raw logs
✔️ JVM Tests - JDK 18

@aloubyansky aloubyansky merged commit 057e847 into quarkusio:main Dec 14, 2022
@quarkus-bot quarkus-bot bot added this to the 2.16 - main milestone Dec 14, 2022
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Dec 14, 2022
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/gradle Gradle area/testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants