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

Allow beans to be injected in tests even if they aren't used in the application #1804

Merged
merged 2 commits into from
Apr 2, 2019

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Apr 1, 2019

Fixes: #1749

@geoand geoand requested a review from mkouba April 1, 2019 15:36
@geoand geoand changed the title Allow beans to be injected in tests even if they aren't used in the app Allow beans to be injected in tests even if they aren't used in the application Apr 1, 2019
Copy link
Contributor

@mkouba mkouba left a comment

Choose a reason for hiding this comment

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

Looks good. In theory, if we want a more correct solution we could create a TestAnnotationBuildItem in the core-deployment, and add another build step here and then the TestsAsBeanDefiningAnnotationsProcessor would consume this build item.

@geoand
Copy link
Contributor Author

geoand commented Apr 2, 2019

@mkouba do you think we should make that change now or in the future if we have more changes to do around testing and Arc?

@mkouba
Copy link
Contributor

mkouba commented Apr 2, 2019

@geoand It's up to you ;-). The change should be pretty straightforward. If we decide to go with hard-coded values we should at least add some comment about the possible improvement.

@geoand
Copy link
Contributor Author

geoand commented Apr 2, 2019

@mkouba Let me think about for a few minutes :P. The change is indeed super easy to make and I the reason I think it's good to do is to keep things consistent with what we have done so far

@geoand
Copy link
Contributor Author

geoand commented Apr 2, 2019

@mkouba PR updated 😉 . I decided to change it because the solution you proposed is more in line with how similar cases have been handled so far.

@mkouba
Copy link
Contributor

mkouba commented Apr 2, 2019

Thanks, looks good! Let's wait for CI.

@geoand geoand dismissed stuartwdouglas’s stale review April 2, 2019 09:37

@mkoube has reviewed the latest version

@geoand geoand merged commit be21697 into quarkusio:master Apr 2, 2019
@geoand geoand deleted the #1749 branch April 2, 2019 09:37
@geoand geoand added this to the 0.13.0 milestone Apr 2, 2019
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.

3 participants