[Cdi2,Jakarta Cdi] Add step definitions as beans when not discovered

[mpkorstanje]

Depending on the CDI implementation used adding bean classes through
getInitializer().addBeanClasses(clazz); while also using a beans.xml file
to mark all classes in the module as beans results in duplicate bean
definitions. By defining step definitions only as beans when not already defined
we avoid this problem.

Fixes: #2241

[codecov]

Codecov Report

Merging #2248 (c040705) into main (dde1d4c) will increase coverage by 0.09%.
The diff coverage is 96.96%.

@@             Coverage Diff              @@
##               main    #2248      +/-   ##
============================================
+ Coverage     83.01%   83.10%   +0.09%     
- Complexity     2340     2352      +12     
============================================
  Files           307      307              
  Lines          8324     8370      +46     
  Branches        768      770       +2     
============================================
+ Hits           6910     6956      +46     
  Misses         1110     1110              
  Partials        304      304              

Impacted Files	Coverage Δ	Complexity Δ
...i2/src/main/java/io/cucumber/cdi2/Cdi2Factory.java	98.21% <96.96%> (+1.24%)	16.00 <13.00> (+6.00)
...ava/io/cucumber/jakarta/cdi/CdiJakartaFactory.java	98.21% <96.96%> (+1.24%)	16.00 <13.00> (+6.00)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dde1d4c...c040705. Read the comment docs.

[mpkorstanje]

@dcendents could you update the cdi2/README.md? I'm not a frequent user of CDI so I'm not sure what the best way to explain the need for a beans.xml would be.

[rmannibucau]

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

[mpkorstanje]

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

	@Test
	void shouldInjectStepDefinitions() {
	factory.addClass(OtherStepDefinitions.class);
	factory.addClass(StepDefinitions.class);
	factory.start();
	StepDefinitions stepDefinitions = factory.getInstance(StepDefinitions.class);
	assertThat(stepDefinitions.injected, is(notNullValue()));
	factory.stop();
	}

[rmannibucau]

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".

[dcendents]

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.

[dcendents]

I just thought of something, what about calling clazz.getClassLoader().getResourceAsStream("META-INF/beans.xml"); and add the class only if the file is not there?

I think this way we have the best of both worlds. (And the classes need to be cached and added to every cdi container).

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.

[dcendents]

No, it is part of @Inject spec which is used by CDI but also most of other IoC and this is how some compatibilty test suites are written.

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
• write @Inject on beans I want injected

If 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:

• include a CDI implementation on my classpath
• Initialize the container and get beans from it

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:

• io.cucumber.StepClass1
• io.cucumber.StepClass2
• io.cucumber.PojoSharedState (injected into the 2 teps classes)

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 and StepClass2 are added as synthetic beans, but not PojoSharedState, 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 in beans.xml.

Now to recap different @Injection scenarios:

• Should a beans.xml file be required in src/test/resources?
  • Step classes that @Inject classes from other libs (and transitive injection):
    • currently (6.10.0): NO (but it is required in src/main/resources and every jar part of the injection)
    • proposed (7.0.0): NO (but it is required in src/main/resources and every jar part of the injection)
  • Step classes that @Inject local pojo test classes (and transitive injection):
    • currently (6.10.0): YES
    • proposed (7.0.0): YES
  • Step classes that @Inject other step classes (and transitive injection):
    • currently (6.10.0): NO
    • proposed (7.0.0): ? This is what all this is all about

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 UnmanagedInstances 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?

[rmannibucau]

@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).

What you call @Inject spec does not exist, it might be called that by some but CDI is a Java spec.

It does, it is the JSR330: https://javax-inject.github.io/javax-inject/.

As a user, if I want to use CDI, at a minimum I need:

beans.xml file in every lib part of the CDI
write @Inject on beans I want injected

and

So as a user, there is no way around having a beans.xml file if I want to use CDI.

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.

Why would I ever need to inject a cucumber step class into another cucumber step class?

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.

My vote is to keep the code lean and stop messing with synthetic beans.

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.

It is already great that step classes are loaded as UnmanagedInstances and allow injection of classes outside of the local test scope.

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.

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?

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 ;).

[dcendents]

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.jar

So 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

[rmannibucau]

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. ;-)

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).

So 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 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.

I think part of the problem is that openwebbeans seems very lax about the CDI spec compared to Weld.

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.

[dcendents]

@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 a beans.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 a beans.xml file, then it will be broken without it, I'm not doing any magic.