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.

Sign up for GitHub

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

Jump to bottom

Qute: fix template global class generation in the dev mode #45771

Merged
merged 1 commit into from
Jan 23, 2025
Merged

Qute: fix template global class generation in the dev mode #45771

merged 1 commit into from
Jan 23, 2025

Conversation

mkouba
Copy link
Contributor

@mkouba mkouba commented Jan 22, 2025 •
edited
Loading

  • if a non-application template global class is present we have to reflect this fact when assigning the priority for an application template global resolver; otherwise a conflict may occur

This pull request should fix the problem demonstrated by https://github.com/FroMage/roq-qute-repro-globals.

Unfortunately, I don't know how to write an automated test easily. Ideally, we should add a QuarkusDevModeTest in Roq (which provides non-application template globals).

@mkouba mkouba requested review from FroMage and ia3andy January 22, 2025 08:39
@quarkus-bot quarkus-bot bot added the area/qute The template engine label Jan 22, 2025
@ia3andy
Copy link
Contributor

ia3andy commented Jan 22, 2025

Thanks @mkouba :)

@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
@FroMage
Copy link
Member

FroMage commented Jan 22, 2025

QuarkusDevModeTest is the way to go, especially due to the steps to reproduce.

You could either add a test dependency on roq, or perhaps there's a way to declare a non-application class in QuarkusDevModeTest?

@mkouba
Copy link
Contributor Author

mkouba commented Jan 22, 2025

QuarkusDevModeTest is the way to go, especially due to the steps to reproduce.

You could either add a test dependency on roq,

I didn't want to add a test dependency on Roq. It would be used for all tests.

or perhaps there's a way to declare a non-application class in QuarkusDevModeTest?

I didn't find a way...

@mkouba
Copy link
Contributor Author

mkouba commented Jan 22, 2025

Actually, we could try generate a dummy global for a specific QuarkusDevModeTest; it's not nice at all but it should work. I'll try this workaround..

@mkouba
Qute: fix template global class generation in the dev mode
c9f1df1 
- if a non-application template global class is present we have to
reflect this fact when assigning the priority for an application
template global resolver; otherwise a conflict may occur
@mkouba mkouba force-pushed the qute-fix-nonapp-globals branch from b694435 to c9f1df1 Compare January 22, 2025 12:57
@quarkus-bot quarkus-bot bot added the area/arc Issue related to ARC (dependency injection) label Jan 22, 2025
mkouba
mkouba commented Jan 22, 2025
View reviewed changes
...e/deployment/src/test/java/io/quarkus/qute/deployment/devmode/TemplateGlobalDevModeTest.java
* The {@link QuteDummyTemplateGlobalMarker} is used to identify an application archive where a dummy built-in class with
* template globals is added.
*/
public class TemplateGlobalDevModeTest {
Copy link
Contributor Author

@mkouba mkouba Jan 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@FroMage ok, so I've added this test. It's a bit hacky but the build step that generates a dummy global is only executed in the dev mode and if QuteDummyTemplateGlobalMarker is present.

Copy link
Member

Choose a reason for hiding this comment

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

I think you can add custom build steps in your test, no?

Copy link
Member

Choose a reason for hiding this comment

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

Ah no, it's only for ProdMode tests. Well… I suppose we could add that to dev mode tests too…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah no, it's only for ProdMode tests. Well… I suppose we could add that to dev mode tests too…

And in QuarkusUnitTest...

I have no idea how complicated this could be 🤷

Copy link
Member

Choose a reason for hiding this comment

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

Surely some work, up to you, I don't mind your build step :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In any case, it's out of scope of this PR. We could file a new issue and revisit the test later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI #45811

FroMage
FroMage approved these changes Jan 22, 2025
View reviewed changes
...e/deployment/src/test/java/io/quarkus/qute/deployment/devmode/TemplateGlobalDevModeTest.java
* The {@link QuteDummyTemplateGlobalMarker} is used to identify an application archive where a dummy built-in class with
* template globals is added.
*/
public class TemplateGlobalDevModeTest {
Copy link
Member

Choose a reason for hiding this comment

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

I think you can add custom build steps in your test, no?

@quarkus-bot quarkus-bot
Copy link

quarkus-bot bot commented Jan 22, 2025 •
edited by github-actions bot
Loading

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit c9f1df1.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

Flaky tests - Develocity

⚙️ JVM Tests - JDK 21

📦 integration-tests/observability-lgtm

✖ io.quarkus.observability.test.LgtmLifecycleTest.testTracing - History

  • java.lang.RuntimeException: java.lang.reflect.InvocationTargetException - java.lang.RuntimeException
java.lang.RuntimeException: java.lang.RuntimeException: java.lang.reflect.InvocationTargetException
	at io.quarkus.test.junit.QuarkusTestExtension.throwBootFailureException(QuarkusTestExtension.java:611)
	at io.quarkus.test.junit.QuarkusTestExtension.interceptTestClassConstructor(QuarkusTestExtension.java:695)
	at java.base/java.util.Optional.orElseGet(Optional.java:364)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
Caused by: java.lang.RuntimeException: java.lang.reflect.InvocationTargetException
	at io.quarkus.test.junit.TestResourceUtil$TestResourceManagerReflections.startReflectively(TestResourceUtil.java:214)

⚙️ Gradle Tests - JDK 17 Windows

📦 integration-tests/gradle

✖ io.quarkus.gradle.devmode.MavenExclusionInExtensionDependencyDevModeTest.main - History

  • Condition with Lambda expression in io.quarkus.test.devmode.util.DevModeClient was not fulfilled within 1 minutes 30 seconds. - org.awaitility.core.ConditionTimeoutException
org.awaitility.core.ConditionTimeoutException: Condition with Lambda expression in io.quarkus.test.devmode.util.DevModeClient was not fulfilled within 1 minutes  30 seconds.
	at app//org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at app//org.awaitility.core.CallableCondition.await(CallableCondition.java:78)
	at app//org.awaitility.core.CallableCondition.await(CallableCondition.java:26)
	at app//org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1006)
	at app//org.awaitility.core.ConditionFactory.until(ConditionFactory.java:975)
	at app//io.quarkus.test.devmode.util.DevModeClient.getHttpResponse(DevModeClient.java:164)
	at app//io.quarkus.gradle.devmode.QuarkusDevGradleTestBase.getHttpResponse(QuarkusDevGradleTestBase.java:164)

@mkouba mkouba mentioned this pull request Jan 23, 2025
Open
@mkouba mkouba merged commit 1c5b939 into quarkusio:main Jan 23, 2025
52 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.19 - main milestone Jan 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FroMage FroMage FroMage approved these changes

@ia3andy ia3andy Awaiting requested review from ia3andy

Assignees
No one assigned
Labels
area/arc Issue related to ARC (dependency injection) area/qute The template engine triage/backport triage/backport-3.15 triage/flaky-test
Projects
None yet
Milestone
3.19 - main
Development

Successfully merging this pull request may close these issues.
3 participants
@mkouba @ia3andy @FroMage