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

Questions about why CDI TCK fails when Jakarta MVC / Eclipse Krazo are bundled in Glassfish 7 #400

Closed
erdlet opened this issue Jul 26, 2022 · 6 comments · Fixed by #401
Closed
Labels
accepted Issue/challenge is accepted challenge TCK test challenge

Comments

@erdlet
Copy link

erdlet commented Jul 26, 2022

Hi all,

as the headline describes, I'd like to ask a few questions about specific CDI TCK tests which are failing when Jakarta MVC and Eclipse
Krazo are bundled in GF 7. I've done some analysis but I'm not sure if my conclusions are correct, so maybe someone here can help.

ProcessBeanAttributesNotFiredForBuiltinBean

This test fails, because Krazo creates a @RequestScoped instance of HttpServletRequest in the JaxRsContextProducer which is then observed by the TCK method ProcessBeanAttributesObserver#observeHttpServletRequestBeanAttributes. In this case I'm not sure if the producer in Krazo is really necessary / compliant with the CDI spec, because the container already provides a @Default scoped HttpServletRequest.

Removing the producer method fixes the test, but I'd like to understand if this was some spec violation or maybe a missing scope / exclusion in the CDI TCK?

AfterTypeDiscoveryTest

Here the methods testFinalAlternatives and testInitialAlternatives fail, because the AfterTypeDiscoveryObserver used as test extension contains a bunch of Krazo beans (screenshot) which are added in our KrazoCdiExtension. Most of them have in common, that the priority is set to a really low value, sometimes even 0. In this case I'm not sure if the TCK needs to exclude classes which are not part of the test setup or if Krazo needs to prioritize its beans later.

Would be great if someone can help me and answer those questions. Even a hint what can be wrong would be helpful. Thanks in advance!

Screenshot from 2022-07-25 21-18-06

@manovotn
Copy link
Contributor

ProcessBeanAttributesNotFiredForBuiltinBean

This one looks like spec violation.
The tests is linked to this spec part (first 4 lines of this section) - https://jakarta.ee/specifications/cdi/4.0/jakarta-cdi-spec-4.0.html#process_bean_attributes
It is nothing new either, I checked version 2.0 which already had this.

AfterTypeDiscoveryTest

In this case, I think the TCK should be altered to only make assertions on the classes in the test. It doesn't count with possibility of having some extra alternatives around (maybe the issue is the same for other tests in the same class?).
That being said, using things like @Priority(0) is bad practice and apps and libraries should instead use constants available in interceptor spec and adding numbers to them if they need ordering, so for example @Priority(Interceptor.Priority.PLATFORM_AFTER + 1)

@manovotn
Copy link
Contributor

I will take a closer look at AfterTypeDiscoveryTest tomorrow (no longer in front of laptop) and put together a PR that will change how the test operates. It seems fairly simple from first glance.

@erdlet
Copy link
Author

erdlet commented Jul 26, 2022

Thanks a lot for the quick response and information !

The possible spec violation will be fixed on Krazo side as soon as my PR is merged (tomorrow).

Regarding the other test, I think it makes sense to exclude non-TCK classes from there too.

But just for my personal understanding: Does the priority of a bean has an impact on those events, so e.g. a lower priority can fix this problem?

@manovotn
Copy link
Contributor

manovotn commented Jul 26, 2022

@erdlet see https://jakarta.ee/specifications/cdi/4.0/jakarta-cdi-spec-4.0.html#after_type_discovery

getAlternatives() returns the ordered list of enabled alternatives for the application, sorted by priority in ascending order. Alternatives enabled for a bean archive are not included in the list.

(and other similar sentences)
So yes, this does matter but whether it would help I am not sure from the top of my head. It depends if the test asserts which items are first, second, third or something like first, last (as those added might still be last in the collection)

@manovotn
Copy link
Contributor

@erdlet 4.0.7 TCK should now be syncing with Central.
I was hoping to get it out sooner today, but it took me a while longer as there were some weird network issues in the Jenkins CI.

@erdlet
Copy link
Author

erdlet commented Jul 28, 2022

@manovotn Thanks a lot!

No problem, we had this Eclipse Jenkins network trouble a few days ago too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Issue/challenge is accepted challenge TCK test challenge
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants