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

Add a scenario with an extension registering many classes #324

Merged
merged 1 commit into from
Feb 2, 2023
Merged

Add a scenario with an extension registering many classes #324

merged 1 commit into from
Feb 2, 2023

Conversation

essobedo
Copy link
Contributor

@essobedo essobedo commented Dec 6, 2022

A scenario to illustrate quarkusio/quarkus#29693

@rsvoboda
Copy link
Member

rsvoboda commented Dec 7, 2022

Thank you for the contribution. I will keep the PR as is till Quarkus issue gets resolved.

This test suite is kept in current form without new additions. For Qiuarkus 2 series we moved to https://github.com/quarkus-qe/quarkus-test-suite to unify for both bare metal and OpenShift scenarios.
This test suite is primarily used for daily runs against Quarkus main to detect significant changes done to the codebase.

@essobedo
Copy link
Contributor Author

essobedo commented Dec 7, 2022

@rsvoboda Thx for the info, do you confirm that it is the right place to define a use case to be tested?

@rsvoboda
Copy link
Member

rsvoboda commented Dec 7, 2022

Your PR can land here, no issue with that. Tests from this repo are executed against Quarkus main.

If you plan to evolve the test coverage in the future, https://github.com/quarkus-qe/quarkus-test-suite is better place as more people are looking into that repo.

@rsvoboda
Copy link
Member

rsvoboda commented Feb 2, 2023

@essobedo Quarkus PR is merged, so I tried to run the module against Quarkus main quarkusio/quarkus@9666d20

Unfortunately I receive Could not find artifact io.quarkus.qe:024-quarkus-extension-with-many-classes-to-register-deployment:jar:1.0.0-SNAPSHOT when running mvn -e clean verify -f 024-quarkus-extension-with-many-classes-to-register

Is there a special way how to run the module?

@rsvoboda
Copy link
Member

rsvoboda commented Feb 2, 2023

I was able to get around the above issue by disabling generate-code step (defined in root pom.xml) on deployment extension .

--- a/024-quarkus-extension-with-many-classes-to-register/deployment/pom.xml
+++ b/024-quarkus-extension-with-many-classes-to-register/deployment/pom.xml
@@ -8,6 +8,9 @@
         <relativePath>../pom.xml</relativePath>
     </parent>
     <artifactId>024-quarkus-extension-with-many-classes-to-register-deployment</artifactId>
+    <properties>
+        <quarkus.generate-code.skip>true</quarkus.generate-code.skip>
+    </properties>
     <dependencies>
         <dependency>
             <groupId>io.quarkus</groupId>

The build passes with Quarkus main.

But it also passes with 2.16.0.Final mvn clean verify -f 024-quarkus-extension-with-many-classes-to-register -Dquarkus.platform.version=2.16.0.Final -Dquarkus-plugin.version=2.16.0.Final
So the added scenario doesn't seem to be a full reproducer for quarkusio/quarkus#29693 / quarkusio/quarkus#29886

@essobedo
Copy link
Contributor Author

essobedo commented Feb 2, 2023

Weird, let me ckeck

@essobedo
Copy link
Contributor Author

essobedo commented Feb 2, 2023

@rsvoboda Your change is correct, I included it in my PR and added you as co-author.

Regarding your test, it is normal that it works because the bug only happen in native mode not in JVM mode, add -Pnative to your command and you should get a failure

@essobedo
Copy link
Contributor Author

essobedo commented Feb 2, 2023

Hum I don't have the right email to add you as co-author

@rsvoboda
Copy link
Member

rsvoboda commented Feb 2, 2023

[email protected] is correct one. Tried native mode and it compiles with main, fails with 2.16

With main I just get many warnings like Warning: Could not resolve io.quarkus.qe.SomeClass28655 for reflection configuration. Reason: java.lang.ClassNotFoundException: io.quarkus.qe.SomeClass28655.. That's imho expected.

@essobedo
Copy link
Contributor Author

essobedo commented Feb 2, 2023

@rsvoboda yes indeed, it’s the expected behavior.

@rsvoboda
Copy link
Member

rsvoboda commented Feb 2, 2023

Will merge once CI finishes.

@rsvoboda rsvoboda merged commit 3dacb68 into quarkus-qe:main Feb 2, 2023
@essobedo essobedo deleted the 29693/fix-MethodTooLargeException-when-too-many-classes-to-register branch February 2, 2023 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants