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

Update beans.xml files to version 3.0 #222

Closed
brideck opened this issue Mar 3, 2021 · 10 comments · Fixed by #224
Closed

Update beans.xml files to version 3.0 #222

brideck opened this issue Mar 3, 2021 · 10 comments · Fixed by #224

Comments

@brideck
Copy link
Contributor

brideck commented Mar 3, 2021

The CDI TCK currently uses the Shrinkwrap API to construct its test applications, including dynamic generation of metadata files like beans.xml. This results in all of the beans.xml files in the TCK being stuck at version 1.1:

<beans xmlns="http://xmlns.jcp.org/xml/ns/javaee" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" bean-discovery-mode="all" version="1.1" xsi:schemaLocation="http://xmlns.jcp.org/xml/ns/javaee http://xmlns.jcp.org/xml/ns/javaee/beans_1_1.xsd"/>

With the release of Jakarta EE 9 and the switch to the jakarta namespace, these should all be updated to version 3.0.

@tevans78
Copy link

tevans78 commented Mar 4, 2021

Should there be tests for all previous versions as well as the latest one? They should all work still.

@Emily-Jiang
Copy link
Contributor

Agreed with @brideck for update the beans.xml to version 3.0. @tevans78 I don't think the tcks should cover the mix and match scenario though.

@tevans78
Copy link

tevans78 commented Mar 4, 2021

Section 12.1 of the spec (Bean Archives) says

An explicit bean archive is an archive which contains a beans.xml file:
•with a version number of 1.1 (or later), with the bean-discovery-mode of all, or.....

Therefore, in order to be spec compliant, a beans.xml with version 1.1. or 2.0 should also work. It is implied that a beans.xml with version 1.0 should also work in general.

@manovotn
Copy link
Contributor

manovotn commented Mar 4, 2021

Yes, it should work with older versions as well (which is basically why all of TCKs pass with current implementations in the first place).
TCKs, as they are now, should switch to new version. Whether we want to add another tests for older versions is up to us - we can but don't need to.

@tevans78
Copy link

tevans78 commented Mar 4, 2021

@manovotn are you aware of any plans to update ShrinkWrap Descriptors to support EE9?

@manovotn
Copy link
Contributor

manovotn commented Mar 4, 2021

@tevans78 this one https://github.com/shrinkwrap/descriptors?

No, I am not. And looking at the commits, it's been dead for some time now. That being said, if there were contributions we could revive it (I suppose I could get to someone who has commit rights). But I have no idea how much work would it be to update it; it is a lot of descriptors and CDI really only cares about a fraction and I have no idea if there are any other parties that would be interested in having this updated.

Alternatively we could either rewrite TCK tests to use a pre-created beans.xml file - many tests actually do that anyway.
Lastly we could look if we can replicate part of the descriptors project as a cdi-tck module that would allow us to define beans.xml (which is probably the only bit we are using).

@Emily-Jiang
Copy link
Contributor

Emily-Jiang commented Mar 4, 2021

I am a bit confused on which version of beans.xml the cdi tck should use. Should we use the latest beans.xml and then which tests should use old version of beans.xml? Personally, I think the spec should say beans.xml version 3.0 instead of saying 1.1 or later as that one still refer to javaee etc. I don't get why it should work with beans.xml version 1.0 either.

@tevans78
Copy link

tevans78 commented Mar 5, 2021

Most tests should use the latest version but there should be some coverage for older versions as well. Otherwise, the wording of the spec should be changed/clarified.

@manovotn
Copy link
Contributor

manovotn commented Mar 9, 2021

After some more probing I found out that the shrinkwrap descriptors project would probably be a lot of work given what we need and that we don't really have any other interested party.

OTOH, I found that Weld in some older version already implemented a beans.xml builder - https://github.com/weld/core/blob/master/tests-common/src/main/java/org/jboss/shrinkwrap/impl/BeansXml.java
We could most likely take this and paste into TCKs (with some minor alterations OFC) as it implements Shrinkwrap Asset. This would cover the cases where we need to define beans.xml, the rest we'd still need to manually declare but that should be a fraction of them.

I'll check if this would be a viable solution tomorrow.

@manovotn
Copy link
Contributor

I have a WIP branch here - https://github.com/manovotn/cdi-tck/commits/issue222
It's not complete (missing tests for the new API itself and change of existing beans.xml files to new schema) but it shows that porting the simple API for beans.xml creation seems like a way to go.

However, there will still be 30+ occurrences of Shrinkwrap descriptors for other than beans.xml files (manifest, web app desc., persistance.xml,...). Some good soul can contribute a PR that will shift away from shrinkwrap and add those files manually.

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 a pull request may close this issue.

4 participants