Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Cdi2,Jakarta Cdi] Add step definitions as beans when not discovered #2248
[Cdi2,Jakarta Cdi] Add step definitions as beans when not discovered #2248
Changes from 1 commit
377f828
179c3fd
f2fa54a
6263146
3c792fb
61a2df4
0754285
80ea1fb
785ec1f
31395a7
2c6fc42
f6ed48b
79efff1
8892693
949c39a
40ef3f0
bbaa990
6f3b33e
da701f8
cac3566
d11bf3e
3e9b424
c0b2c48
d6e4840
9605331
e0b7e7d
f9ea8e2
d5266e7
dfbe663
5aed18f
f6f4c7d
01e200a
cc676c4
c040705
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
dropping this line can break some step definition (anyone used as injection in other steps or using cdi some extensions for their injections) so not sure it is desired
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've added a test for this. I could only break it when I don't include
beans.xml
. Do you know other ways to break it?cucumber-jvm/cdi2/src/test/java/io/cucumber/cdi2/Cdi2FactoryTest.java
Lines 103 to 111 in 377f828
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.
A way to ensure it is as expected is to write a CDI extension which veto all undesired managed types and add the desired ones (bm.addAnnotatedType(bm.createAnnotatedType(MyType.class))), this way whatever the scanning is, the test is "as expected".
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 steps inject the local test bean Belly so a beans.xml will always be required.
To properly test it we would need a project that injects beans from the main code or from libraries only.
Here is a quick test project without a test beans.xml: https://github.com/dcendents/cucumber-cdi2-test
It works fine, getInstance creates UnmanagedInstances, beans are injected.
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.
Forget it, this is more complex than that, looking at how openwebbeans find beans, we need to scan the classpath for all beans.xml files, then resolve the jar (or directory) for each and load classes inside that specific jar.
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.
But it is required by the CDI spec: http://www.cdi-spec.org/faq/
Look at: What is beans.xml and why do I need it?
Also: https://docs.jboss.org/cdi/spec/2.0/cdi-spec.html
What you call
@Inject
spec does not exist, it might be called that by some but CDI is a Java spec.@Inject
is just a java annotation that CDI recognises, and other frameworks can use the same annotation but that doesn't make them a CDI implementation.https://weld.cdi-spec.org/
"Weld is the reference implementation of CDI: Contexts and Dependency Injection for the Java EE Platform - a JCP standard for dependency injection and contextual lifecycle management and one of the most important and popular parts of the Java EE"
https://openwebbeans.apache.org/
"Apache OpenWebBeans delivers an implementation of the Contexts and Dependency injection for Java EE (CDI) 2.0 Specification (JSR-365)."
As a user, if I want to use CDI, at a minimum I need:
beans.xml
file in every lib part of the CDI@Inject
on beans I want injectedIf I deploy code in a web or application container that might be it. Nothing else is required.
e.g.: Deploy JAX-WS service on Wildfly and inject beans in it.
If I run in a plain JavaSE environment then I also need to:
So as a user, there is no way around having a
beans.xml
file if I want to use CDI.At this point I hope we can agree CDI is a spec and
beans.xml
is required.Now let's talk about the tests:
I've used cdi-unit in the past and it is true I did not need a
beans.xml
file in my test resources. Why? Because with plain junit tests I only need to inject production code into my class. I also could easily declare mocks as alternative beans and get them injected into the code to test. All in all, everything about my tests was contained in a single class as is usual with junit tests.With cucumber it makes sense to split step classes around features and break the 1 test class for 1 production class paradigm. Then we have to inject a shared state object between those steps.
Imagine the following very simple test structure:
With the current cucumber-cdi2 version 6.10.0, is it possible currently to run those tests without having a
beans.xml
file: NO it is not possible.StepClass1
andStepClass2
are added as synthetic beans, but notPojoSharedState
, cucumber does not know anything about that class, it does not have cucumber annotations in it.So great if cucumber-cdi2 says, "You can start using CDI in your tests and we'll do some automatic wiring for you and it will work even if you don't follow the spec. As long as you don't inject test POJO though."
But where I don't agree (and is why I opened #2241), is that if you want to use Weld (the reference CDI implementation) it will not work because our automatic wiring broke it. You will have to start editing that
beans.xml
file, figure out what cucumber-cdi2 does under the hood and explicitely ignore these classes from the discovery process. And everytime you add a step class don't forget to go and ignore it inbeans.xml
.Now to recap different
@Inject
ion scenarios:beans.xml
file be required insrc/test/resources
?@Inject
classes from other libs (and transitive injection):src/main/resources
and every jar part of the injection)src/main/resources
and every jar part of the injection)@Inject
local pojo test classes (and transitive injection):@Inject
other step classes (and transitive injection):I've asked the question a few times before: Why would I ever need to inject a cucumber step class into another cucumber step class? In my opinion this is a very bad design and should be refactored, test state should not be saved in step classes.
Now you both @mpkorstanje and @rmannibucau disagree, I'm very new to cucumber so I don't know how this should be resolved.
My vote is to keep the code lean and stop messing with synthetic beans.
It is already great that step classes are loaded as
UnmanagedInstance
s and allow injection of classes outside of the local test scope.Is it reasonable to spend so much time and effort to allow injection of step classes into other step classes (bad design), decide to use CDI (and not PicoContainer or other alternatives) but don't want to add the required
beans.xml
as per the CDI spec?P.S.: Should the discussion moved to Slack?
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.
@Inject is NOT CDI but CDI uses @Inject, don't reverse or merge it. This is also why some steps are written portably and tested against CDI, Spring, Guice and a few other IoC. You have CDI so you have @Inject but you can miss all other parts of CDI like its activation (beans.xml).
It does, it is the JSR330: https://javax-inject.github.io/javax-inject/.
and
No again, CDI 2 enables to no more write/create beans.xml and you don't have to use @Inject as this cucumber integration shows - steps are looked up without @Inject generally.
AFAIK with the addClass the case I described works as of today and is broken by removing addClass.
This is a fair question and it can happen to reuse some steps (aliases/rephrasing for a better human writing experience) but you also have the case where you have a transversal bean checking data or resetting in steps with a scope (or not) which is purely technical but requires the step to be made a bean.
Agree, this is why I proposed to enable archives with "just" replacing the addClass by a addPackage(true, xxx) so adding a config to the integration instead of trying to be clever.
This is true but on the other side, if you do what you describe, ie make them actual bean, it is not what you want, you want to lookup the bean properly, this is why I proposed to use a try to lookup and if not possible use unmanaged instance.
Well it is not only about steps into steps - even if this case makes sense from time to time I could live without it - but also being able to use step as beans which is what the addClass enables (but not yet the lookup which I lacked in a few projects already).
And once again CDI does NOT need a beans.xml as per spec but has some funny scanning rules. That said the point is to be able to use steps "as this", don't assume you own all the code you run, it only happens when you are lucky but it seems I'm not as lucky as you on this point ;).
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.
then my bad to think that cucumber-cdi2 was a CDI 2.0 injection framework for cucumber.
I suggest the module be renamed to cucumber-javax-inject. ;-)
Seriously I'm not an expert on the matter, but javax.inject is JSR-330, CDI 2.0 is JSR-365 which includes (and respects) JSR-330: https://javaee.github.io/tutorial/cdi-basic002.html
Now unless I'm mistaken
javax.enterprise.inject.se.SeContainerInitializer.SeContainerInitializer.newInstance()
is part of the CDI and not the javax.inject spec.javax.enterprise.inject.se.SeContainerInitializer
is from cdi-api-2.0.SP1.jar@javax.inject.Inject
comes from javax.inject-1.jarSo by using
SeContainerInitializer
this is a CDI 2.0 module and therefore must respect the spec, at least not break it (again issue #2241)I think part of the problem is that openwebbeans seems very lax about the CDI spec compared to Weld. Weld is the reference implementation and is used in most application containers so it makes sense that we test properly against it. If enabling a feature breaks Weld then in my opinion this is not acceptable.
Weld 1.0 was following javax.inject spec (I never used Weld 1 so wasn't even aware of that spec)
Weld 2.0 is CDI 1.1
Weld 3.0 is CDI 2.0
Weld 4.0 is CDI 3.0
cucumber-cdi2 project builds with Weld 3.X, jakarta-cdi (CDI 3.0) builds with Weld 4.X
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 can't understand what you mean there. cucumber-cdi2 is about running in a CDI container with the CDI 2 SE API configuration support. This means it must also support @Inject spec since CDI includes it (and once again not the opposite).
I see where you come from but to be a CDI container you must pass CDI validations (TCK) but also @Inject validation. This means you can reuse @Inject modules in CDI features without having to have these steps CDI steps. Hope it is clearer written this way.
Hmm, it is not, both pass the same test kit but weld tend to be broken in several environments and more restrictive (but it is not in the spec). I agree we must run with weld - and we were at some point, didn't check recently. We can refine the error you get if you want but in current form it should be portable.
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.
@rmannibucau well I suppose we disagree on what cucumber-cdi2 should do because I have no idea what is the contract between these extensions and cucumber-core. I'm very new to the project.
But I think I nailed the solution this time. Not only does my latest commit fix #2241, it doesn't break any functionality of 6.1.0, it also makes it possible to inject most POJO objects without a
beans.xml
file.I register a CDI Extension and test if all the step classes can be resolved, if not I add them as a bean on the BeanManager. So this doesn't break Weld and it means
addClasses
does what it should. This is also a supported way to work with CDI so no voodoo magic here.Now the cherry on top: for each class added manually, I introspect the injection points and test them as well, so now we can
@Inject
POJO classes without abeans.xml
file, as long as the injected types are valid.i.e.: if it works with a
beans.xml
file, it should also work without. if it is broken with abeans.xml
file, then it will be broken without it, I'm not doing any magic.