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

QuarkusComponentTest: register default and support custom converters #41747

Merged
merged 2 commits into from
Jul 10, 2024
Merged

QuarkusComponentTest: register default and support custom converters #41747

merged 2 commits into from
Jul 10, 2024

Conversation

mkouba
Copy link
Contributor

@mkouba mkouba commented Jul 8, 2024

@mkouba
Copy link
Contributor Author

mkouba commented Jul 8, 2024

CC @diversit
@mkouba mkouba requested a review from manovotn July 8, 2024 09:30
@quarkus-bot

This comment has been minimized.

Sign in to view
radcortez
radcortez reviewed Jul 8, 2024
View reviewed changes
.../junit5-component/src/main/java/io/quarkus/test/component/QuarkusComponentTestExtension.java
@@ -421,6 +422,7 @@ private void startContainer(ExtensionContext context, Lifecycle testInstanceLife
ClassLoader tccl = Thread.currentThread().getContextClassLoader();
SmallRyeConfigBuilder configBuilder = new SmallRyeConfigBuilder().forClassLoader(tccl)
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't we use https://github.com/quarkusio/quarkus/blob/main/core/runtime/src/main/java/io/quarkus/runtime/configuration/ConfigUtils.java#L76 and let the Converters be discovered?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could but I'm not 100% sure it's the best fit for QuarkusComponentTest.

For example, we create a new Config for each test method. And the discovery itself could be quite expensive. Also the discovered components may require some functionality provided by Quarkus runtime - which might not be available in QuarkusComponentTest.
Copy link
Contributor Author

@mkouba mkouba Jul 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

That said, we could make it configurable...

Actually, it would make more sense to allow the test to modify the SmallRyeConfigBuilder. This would be a more advanced feature compared to the QuarkusComponentTest#configConverters(). And it could be only used from the QuarkusComponentTestExtensionBuilder.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.
Copy link
Member

Choose a reason for hiding this comment

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

SmallRyeConfigBuilder can be customized via https://smallrye.io/smallrye-config/3.8.3/config/customizer/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. But the functionality is no different compared to the plain Consumer<SmallRyeConfigBuilder> in this PR, is it?
Copy link
Member

Choose a reason for hiding this comment

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

No. It was just to let you know there is a way to influence the SmallRyeConfigBuilder via ServiceLoader, so you may not need a specific API to expose it.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I get it, but in the context of QuarkusComponentTest we would like to avoid using ServiceLoaders.
@quarkus-bot

This comment has been minimized.

Sign in to view
manovotn
manovotn approved these changes Jul 10, 2024
View reviewed changes
Copy link
Contributor

@manovotn manovotn left a comment

Choose a reason for hiding this comment

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

Added just a minor note, otherwise LGTM.
...it5-component/src/main/java/io/quarkus/test/component/QuarkusComponentTestConfiguration.java Show resolved Hide resolved
@manovotn manovotn added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jul 10, 2024
@quarkus-bot
Copy link

quarkus-bot bot commented Jul 10, 2024 •
edited by github-actions bot
Loading

Status for workflow Quarkus CI

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

✅ 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.
@manovotn manovotn merged commit 389f694 into quarkusio:main Jul 10, 2024
23 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.13 - main milestone Jul 10, 2024
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/testing kind/bugfix
3 participants
@mkouba @manovotn @radcortez