-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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 easy way for QuarkusTestResourceLifecycleManager implementations to inject into test #18714
Conversation
* @param annotations Optional annotations that the field must have. If more than one are specified, the field | ||
* must be annotated with all the provided annotations | ||
*/ | ||
void injectIntoFields(Object fieldValue, Class<?> fieldType, Class<? extends Annotation>... annotations); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you could add a variant with javax.enterprise.util.TypeLiteral<?> fieldType
to handle the parameterized types but the problem is that you would have to specify the rules for matching... or rely on existing rules, e.g. from CDI. But these need to be inherently complex and thus it's probably not worth the effort.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point!
I would prefer we start with this simple approach for now and keep your suggestion in mind for when / if people need something more complex
This workflow status is outdated as a new workflow run has been triggered. 🚫 This workflow run has been cancelled. Failing Jobs - Building eb43068
|
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building ec38e0a
Full information is available in the Build summary check run. Test Failures⚙️ Gradle Tests - JDK 11 #📦 integration-tests/gradle✖ |
if (annotationMatchResult == AnnotationMatchResult.MATCHED) { | ||
// if the annotations match, we throw an exception if the types don't match as it's almost certainly a user error | ||
// if however there were no annotation supplied, then we continue searching for candidate fields | ||
throw new RuntimeException("'" + Arrays.toString(annotations) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC, this prevents some thing like:
@InjectTestObject
Foo foo;
@InjectTestObject
Bar bar;
when injectIntoFields(foo, Foo.class, InjectTestObject.class)
is called?
Isn't that too strict when a manager provides multiple objects that can be injected? I mean we would force users to create multiple annotations for that (or none at all).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is indeed true.
When writing this, I did get the feeling that the type parameter is likely unnecessary.
What do you think about making it an Optional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing is that then the method becomes difficult to use...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'm going to do something different. I'll have two methods: One which only takes an annotation as a param and one which takes a Predicate. That way if more advanced checking is needed, a user can specify it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, even though it's more "hidden"/less expressive than using an annotation, injection simply by type is the easiest option after all because the user doesn't have to implement an annotation for that (KISS).
So I think (ideally) there should be four options:
- inject by type
- inject by annotation
- inject by type and annotation
- inject by whatever via Predicate
The first tree methods could simply call that Predicate method... 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And then you'll end up with a new CDI ;-).
I think that injectIntoFields(Object value, Predicate<Field> predicate)
is flexible enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I thought there is much more to CDI than that. 😉
Anyway, my primary focus was to support lazy devs (aka developer joy) but yeah, you can do everything with the Predicate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the PR. WDYT?
This workflow status is outdated as a new workflow run has been triggered. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
A MatchesType
predicate would make it complete or even a dead simple injectIntoFields(Object fieldValue)
that infers the type from fieldValue
, but both can be added later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building 341d7e1
Full information is available in the Build summary check run. Test Failures⚙️ JVM Tests - JDK 11 #📦 integration-tests/openshift-client✖ ✖ ⚙️ JVM Tests - JDK 11 Windows #📦 integration-tests/openshift-client✖ ✖ ⚙️ JVM Tests - JDK 16 #📦 integration-tests/hibernate-reactive-panache✖ 📦 integration-tests/openshift-client✖ ✖ ⚙️ Native Tests - Misc3 #📦 integration-tests/openshift-client✖ ✖ |
…ect into test Resolves: quarkusio#18698
This workflow status is outdated as a new workflow run has been triggered. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, sorry about the late review :(
Resolves: #18698