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

[TCK Challenge]: ExtensionTests #709

Closed
1 task done
starksm64 opened this issue Apr 18, 2024 · 3 comments · Fixed by #710
Closed
1 task done

[TCK Challenge]: ExtensionTests #709

starksm64 opened this issue Apr 18, 2024 · 3 comments · Fixed by #710
Labels
challenge TCK Challenge

Comments

@starksm64
Copy link
Contributor

Specification

https://github.com/jakartaee/data/blob/main/tck/src/main/java/ee/jakarta/tck/data/core/cdi/ExtensionTests.java

Assertion

CDI beans, custom entity annotation

TCK Version

SNAPSHOT (Locally built)

Implementation being tested

HIbernate / Weld

Challenge Scenario

Claims that a test is not portable or depends on a particular implementation.

Full Description

We are running into two problems with the ExtensionTests, both ee.jakarta.tck.data.core.cdi and ee.jakarta.tck.data.web.cdi.
The first issue is that the ee.jakarta.tck.data.common.cdi.Directory has a custom provider and so Hibernate does not process it to generate an implementation that is a CDI bean. Instead there is a ee.jakarta.tck.data.common.cdi.DirectoryRepository implementation, but it does not have a bean defining annotation, and so is not picked up for processing by the BuildCompatibleExtensionImpl. When Hibernate generates a repository bean it annotates it with a @RequestScoped annotation. The DirectoryRepository should have a bean defining annotation or a beans.xml descriptor that identifies this as a CDI bean.

The second problem relates to the ee.jakarta.tck.data.common.cdi.AddressBook repository interface. This is using a custom EntityDefining annotation, and Hibernate does not generate a repository bean for this as expected. However, the ee.jakarta.tck.data.common.cdi.AddressRepository is also missing a bean defining annotation.

If I add @RequestScoped annotations to both repository beans, we see all tests passing.

Additional Context

No response

Is there an existing challenge for this?

  • I have searched the existing issues
@gavinking
Copy link
Contributor

gavinking commented Apr 18, 2024

So I spent like 2 hours trying to understand all this last night, and it's all still very murky to me.

Apparently, there are two competing ways that these beans are supposed to get registered, both working via a CDI extension. Either:

  1. by ExtensionImpl registering a DirectoryRepositoryProducer which creates a DirectoryRepository, or
  2. by BuildCompatibleExtensionImpl registering the DirectoryRepository bean directly.

But as a special twist, neither of the extensions just registers the bean. Instead, the extensions want to be notified of the existence of the repository interface via a ProcessAnnotatedType event, or via a @Enhancement callback. I can only guess at the motivation behind this: I speculate that what it's doing is attempting to simulate how the OpenLiberty implementation of Jakarta Data works.

But this is a maximally-overcomplicated way to use CDI to perform the required task; as Scott says, the same total effect may be obtained by sticking a bean-defining annotation on DirectoryRepository and AddressRepository. No CDI extension is necessary.

So, anyway, I spent time trying to figure out why this is not working for us when we run these tests. I see that the extensions (both) get created, and called, but they're never notified of the existence of the Directory or AddressBook interfaces, and so they don't register the DirectoryRepository or AddressRepository implementations. So it seems that the ShrinkWrap-created WAR is not being properly scanned.

This is well beyond my knowledge of Arquillian and ShrinkWrap to understand. But whatever is going wrong here has for sure absolutely nothing to do with our Jakarta Data implementation, and it seems to me that the TCK has no good motivation to do things in such a complicated way.

gavinking added a commit to gavinking/data that referenced this issue Apr 18, 2024
These tests are supposed to be validating that the JD implementation is
ignoring the Directory and AddressBook repository interfaces. It does not
need to be testing stuff related to handling of CDI extensions running on
Arquillian/Shrinkwrap, and so none of this complexity is necessary.

see jakartaee#709
@njr-11
Copy link
Contributor

njr-11 commented Apr 18, 2024

I believe these parts of the extension tests were copied from some Open Liberty tests that attempted to simulate some ways we thought a NoSQL provider might try to plug in via CDI (at least at that point in time of the spec), as part of an effort to help ensure there would be no collisions. This approach for testing does seem to be unnecessarily complicated and it should be fine to simplify it to what is suggested here. That should still cover avoiding conflicts.

@gavinking
Copy link
Contributor

Excellent 🙏🙏

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

Successfully merging a pull request may close this issue.

3 participants