Make sure TCKs are CDI-version agnostic #743
Conversation
|
I should add that I was frankly surprised to see this few tests are affected, but the rest was working just fine for me with CDI 4/Weld 5 (372 tests executed in total). |
|
Can one of the admins verify this patch? |
| @@ -98,8 +99,7 @@ public static WebArchive deploy() { | |||
| } | |||
|
|
|||
| @Inject | |||
| @ConfigProperty(name = "tck.config.test.javaconfig.converter.duckname") | |||
| private Duck namedDuck; | |||
| InjectingBean bean; | |||
There was a problem hiding this comment.
I think a simplier update is to make this Test class CDI bean as you see line 75 also uses Inject.
There was a problem hiding this comment.
@Emily-Jiang I see what you mean and noticed it myself but I disagree.
Currently, the test class is treated as a bean "by accident" as there is discovery mode all and the class will be processed as if it were a bean - all events fired from ProcessAnnotatedType to ProcessInjectionTarget.
However, Arquillian itself will never even use this bean and will instead create an instance of test class manually and then proceeds to detect injection points and use CDI to set the fields (this is why I say it works "by accident").
Marking the test as bean just re-introduces this side effect that has no real use. Or, to put it differently, marking the test as a bean doesn't resemble a real world scenario.
Looking at MP Config impl, I saw that a CDI extension is used to detect PAT and PIT and other events to see what beans are needed and then registering them synthetically based on discovered injection points - this is perfectly fine but it requires actual meaningful CDI bean which is what I added in this PR.
As for like 75 - this is fine because the Config bean is always present (whenever there is MP config impl) by definition and it won't interfere with anything. You could inject such bean anywhere or even resolve it directly from CDI.current() inside a non-contextual object which is what makes it different from the injection points I moved into a separate bean.
There was a problem hiding this comment.
Does my explanation make sense @Emily-Jiang?
There was a problem hiding this comment.
I am not sure whether you can inject the Config anywhere though. For supporting injection, it would have to be CDI beans or Java/Jakarta EE component classes. How would CDI container discover this test class and perform the injection? I am just worried about not being able to pass the tests from a Jakarta EE context. I completely understand you can get hold of the bean from CDI.current() but unsure about the support of the injection.
There was a problem hiding this comment.
@Emily-Jiang the injection into Arq. test instances is governed by Arq. adapter itself, see https://github.com/arquillian/arquillian-core/tree/master/testenrichers/cdi-jakarta
If memory serves, this will actually use something like CDI.current() to perform the injection into test instance.
The thing about Config is that the bean for it is registered without detecting an injection point requiring it. That's not the case for the rest of beans I moved elsewhere.
There was a problem hiding this comment.
ah, okay. Thanks for the explanation! I was worried about that being injected by the CDI container.
Fixes #742
This doesn't change the TCK behavior in any way. It simply makes it CDI-version agnostic so that the TCK works with CDI 3 and CDI 4 implementations. It will ease transition for any early adopters and will eliminate future fraction when actual migration to EE 10 happens. See linked issue for more information.
This change was tested with SR impl customized to run the TCKs on Weld 5 container and the tests passed.