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

Provide proper error message when using @ConfigProperty in @QuarkusIntegrationTest #24579

Merged
merged 1 commit into from
Mar 30, 2022

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Mar 28, 2022

These tests do not support injection (as they are not beans), but this fact
is not obvious.
This error message (which is analogous to what we already have with @Inject)
should make this fact salient.

…tegrationTest

These tests do not support injection (as they are not beans), but this fact
is not obvious.
This error message (which is analogous to what we already have with @Inject)
should make this fact salient.
@geoand geoand requested a review from gsmet March 28, 2022 07:06
@geoand
Copy link
Contributor Author

geoand commented Mar 28, 2022

Did this because @alesj ran into this issue

@geoand geoand requested a review from famod March 29, 2022 10:35
@geoand geoand merged commit a5ca6cd into quarkusio:main Mar 30, 2022
@quarkus-bot quarkus-bot bot added this to the 2.9 - main milestone Mar 30, 2022
@geoand geoand deleted the qit-configproperty branch March 30, 2022 04:38
@rsvoboda
Copy link
Member

@geoand this change block one of our use-cases where we are using @ConfigProperty to fetch information about started service, Jaeger in our case. We are not receiving config property of Quarkus application, but property set in our TestResource.

Here we want to access it - https://github.com/quarkus-qe/beefy-scenarios/blob/main/010-quarkus-opentracing-reactive-grpc/src/test/java/io/quarkus/qe/AbstractPingPongResourceTest.java#L27

Property is set here: https://github.com/quarkus-qe/beefy-scenarios/blob/main/010-quarkus-opentracing-reactive-grpc/src/test/java/io/quarkus/qe/containers/JaegerTestResource.java#L48

Reproducer:

@geoand
Copy link
Contributor Author

geoand commented Mar 31, 2022

An integration test should not be using that since the injection does not work

@rsvoboda
Copy link
Member

It works, see case with 2.7.5.Final. We are setting the property when JaegerTestResource.java#L48 starts and getting that in the test.

We are not using any property from the application

@geoand
Copy link
Contributor Author

geoand commented Mar 31, 2022

I am not really keen on reverting this just because of that weird case to be honest, but if other folks think otherwise, I'll do it.

@rsvoboda
Copy link
Member

I'm not asking to revert this PR. Just wanted to point out the case for breaking change.

Do you have any idea how we could propagate the URL of started jaeger service to the test case?

@geoand
Copy link
Contributor Author

geoand commented Mar 31, 2022

I'm not asking to revert this PR. Just wanted to point out the case for breaking change.

Yes, I am aware, hence the label.

Do you have any idea how we could propagate the URL of started jaeger service to the test case?

You are basically already doing this, so you could just any marker annotation to do the same thing

@rsvoboda
Copy link
Member

🤦 thanks for the push with marker annotation

rsvoboda added a commit to rsvoboda/beefy-scenarios that referenced this pull request Mar 31, 2022
quarkusio/quarkus#24579 - @ConfigProperty is not intended for integration tests
@geoand
Copy link
Contributor Author

geoand commented Mar 31, 2022

:)

pjgg pushed a commit to quarkus-qe/beefy-scenarios that referenced this pull request Mar 31, 2022
quarkusio/quarkus#24579 - @ConfigProperty is not intended for integration tests
radtriste added a commit to radtriste/kogito-runtimes that referenced this pull request Apr 29, 2022
Due to quarkusio/quarkus#24579, we cannot use @ConfigProperty in integration tests, so I used a similar workaround quarkus-qe/beefy-scenarios#275
radtriste added a commit to apache/incubator-kie-kogito-runtimes that referenced this pull request May 5, 2022
* [main] Bump Quarkus version to 2.9.0.CR1

* added bytebuddy dependency for mockito

* Added QuarkusIntegrationTestProperty annotation

Due to quarkusio/quarkus#24579, we cannot use @ConfigProperty in integration tests, so I used a similar workaround quarkus-qe/beefy-scenarios#275

* remove ConfigProperty handling from test resource

* change name to QuarkusTestProperty

* updated QuarkusTestProperty to contain defaultValue

* updated QuarkusTestProperty

* updated QuarkusTestProperty set

* update to 2.9.0.Final

Co-authored-by: Jenkins CI <[email protected]>
Co-authored-by: radtriste <[email protected]>
radtriste added a commit to radtriste/kogito-runtimes that referenced this pull request May 5, 2022
* [main] Bump Quarkus version to 2.9.0.CR1

* added bytebuddy dependency for mockito

* Added QuarkusIntegrationTestProperty annotation

Due to quarkusio/quarkus#24579, we cannot use @ConfigProperty in integration tests, so I used a similar workaround quarkus-qe/beefy-scenarios#275

* remove ConfigProperty handling from test resource

* change name to QuarkusTestProperty

* updated QuarkusTestProperty to contain defaultValue

* updated QuarkusTestProperty

* updated QuarkusTestProperty set

* update to 2.9.0.Final

Co-authored-by: Jenkins CI <[email protected]>
Co-authored-by: radtriste <[email protected]>
radtriste added a commit to radtriste/kogito-runtimes that referenced this pull request May 5, 2022
* [main] Bump Quarkus version to 2.9.0.CR1

* added bytebuddy dependency for mockito

* Added QuarkusIntegrationTestProperty annotation

Due to quarkusio/quarkus#24579, we cannot use @ConfigProperty in integration tests, so I used a similar workaround quarkus-qe/beefy-scenarios#275

* remove ConfigProperty handling from test resource

* change name to QuarkusTestProperty

* updated QuarkusTestProperty to contain defaultValue

* updated QuarkusTestProperty

* updated QuarkusTestProperty set

* update to 2.9.0.Final

Co-authored-by: Jenkins CI <[email protected]>
Co-authored-by: radtriste <[email protected]>
radtriste added a commit to apache/incubator-kie-kogito-runtimes that referenced this pull request May 6, 2022
* [main] Bump Quarkus version to 2.9.0.CR1

* added bytebuddy dependency for mockito

* Added QuarkusIntegrationTestProperty annotation

Due to quarkusio/quarkus#24579, we cannot use @ConfigProperty in integration tests, so I used a similar workaround quarkus-qe/beefy-scenarios#275

* remove ConfigProperty handling from test resource

* change name to QuarkusTestProperty

* updated QuarkusTestProperty to contain defaultValue

* updated QuarkusTestProperty

* updated QuarkusTestProperty set

* update to 2.9.0.Final

Co-authored-by: Jenkins CI <[email protected]>
Co-authored-by: radtriste <[email protected]>

Co-authored-by: kie-ci <[email protected]>
Co-authored-by: Jenkins CI <[email protected]>
elguardian pushed a commit to elguardian/kogito-build that referenced this pull request Mar 23, 2023
* [main] Bump Quarkus version to 2.9.0.CR1

* added bytebuddy dependency for mockito

* Added QuarkusIntegrationTestProperty annotation

Due to quarkusio/quarkus#24579, we cannot use @ConfigProperty in integration tests, so I used a similar workaround quarkus-qe/beefy-scenarios#275

* remove ConfigProperty handling from test resource

* change name to QuarkusTestProperty

* updated QuarkusTestProperty to contain defaultValue

* updated QuarkusTestProperty

* updated QuarkusTestProperty set

* update to 2.9.0.Final

Co-authored-by: Jenkins CI <[email protected]>
Co-authored-by: radtriste <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants