cucumber-cdi2 does not work with weld

[dcendents]

Describe the bug
cucumber-cdi2 does not work with weld.

To Reproduce
Steps to reproduce the behavior:

1. Edit the file cucumber-jvm/cdi2/pom.xml
2. Remove the 2 dependencies for OpenWebBeans
3. Add a dependency to Weld 3.1.6.Final
4. Build the project
5. Get the error

WELD-001335: Ambiguous dependencies for type CdiBellyStepDefinitions with qualifiers 
 Possible dependencies: 
  - Managed Bean [class io.cucumber.cdi2.CdiBellyStepDefinitions] with qualifiers [@Any @Default],
  - Managed Bean [class io.cucumber.cdi2.CdiBellyStepDefinitions] with qualifiers [@Any @Default]

Expected behavior
cucumber-cdi2 unit tests to pass when using weld.

Context & Motivation

I chose to use weld and couldn't make it work until I decided to debug the provided unit tests to understand what I was doing wrong, it seems the problem is global to cucumber-cdi2.

Your Environment

• Versions used: 6.10.0 (but also main git branch)

Additional context
It looks like cucumber-cdi2 is adding an explicit bean for every glue class, but then the default behavior for weld is to scan for (all or annotated classes) and will complain about ambiguous dependencies when both are the same class.

I tried to exclude the class from the scan but then I get a ClassCastException on the second scenario (will open another issue for that problem).

[dcendents]

Looking at the code I believe this problem is because glue classes are explicitely added:

    @Override
    public boolean addClass(final Class<?> clazz) {
        getInitializer().addBeanClasses(clazz);
        return true;
    }

While in the deltaspike implementation it is not:

    @Override
    public boolean addClass(final Class<?> clazz) {
        return true;
    }

I think if we want to use CDI, it is up to the user to configure the project to make sure step classes will be injected by the CDI provider, however we decide to do it (annotations, include all classes, etc).

[rmannibucau]

@dcendents it shouldn't since the impl is then able to unify both after scanning. deltaspike just misses this programmatic feature so it was not possible to do. Looks like a weld bug if they duplicate the beans..A maybe quick/temporary workaround can be to check if there is a beans.xml in the module of the class and if so not add the class but we must do it when the module is not a cdi module (which means testing there is no beans.xml or it contains annotated-mode=none and the bean does not have any defining annotation which sounds way too much compared to what the API is expected to do).

[dcendents]

@rmannibucau maybe I don't get the full logic but after stepping through it in the debugger it seems a bit broken to me.

I'm using junit5 and have the following cucumber dependencies:

• cucumber-java
• cucumber-junit-platform-engine
• cucumber-cdi2

I have a single RunCucumberTest class with the @cucumber annotation.
Multiple feature files with multiple scenarios and mutiple Step classes.

Currently the addClass method is called multiple times (for each step it seems) at the start of the first scenario.
Then the CDI container is stopped and a new one is created for the second scenario (test isolation, that's great), but addClass is not called so from the second scenario forward if the CDI container does not know about the Step class then it is manually loaded as an UnmanagedInstance (inside the getInstance method).

So the statement "but we must do it when the module is not a cdi module" is false because getInstance will resolve it anyway. Also to inject a shared state object in the step classes, we need a test resources beans.xml file, I don't see any way around that (addClass will not be called for state POJO objects to be injected).

Furthermore, it means the behavior is completely different for the first scenario and the rest, which with hindsight explains why only the first scenario reported the Ambiguous dependencies problem and all the 50 others succeeded.

So either we stop manually adding classes as it does not seems necessary (my preference as it means I can simply drop a default beans.xml) or we cache the classes set to add them to every new CDI container to get the same behavior for all the scenarios.

[rmannibucau]

@dcendents code was done for junit4 and never had been tested with junit5, I suspect the lifecycle is not yet fully correct.

That said:

So the statement "but we must do it when the module is not a cdi module" is false because getInstance will resolve it anyway.

Yes, the steps must be resolved even when not a CDI bean and it must be resolved with injections, this is the current impl.

Also to inject a shared state object in the step classes, we need a test resources beans.xml file, I don't see any way around that (addClass will not be called for state POJO objects to be injected).

No, using an unmanaged class you don't need, you basically inject in an instance managed by yourself (cucumber-cdi2 here).

So either we stop manually adding classes as it does not seems necessary

As explained ^^ it is necessary, typical case is src/main/resources/META-INF/beans.xml and a step "/no annotation/ public clsas MyStep { @Inject MyService service ....}". This is valid and does not work in your described proposal but works in current cucumber-cdi2 impl.

we cache the classes set to add them to every new CDI container to get the same behavior for all the scenarios

This can work but the description of the behavior with junit platform engine sounds like it is buggy so I would rather investigate this behavior difference rather than using a workaround in cdi2 module for now.

[dcendents]

@rmannibucau I just tested with junit 4 and it is the exact same behavior, addClass is called at the beginning and is set on the first container only, so in my opinion that is very bad to have different CDI environments between the first scenario and the rest.

I still don't see why we need to add classes manually at the beginning though.

Here are the different configurations we want to support:

• The tests are CDI discoverable (beans.xml)
  • Required if we want to inject a ShareState test object.
  • The step instances will be resolved from the CDI container with all injected beans in it (from test classes, main classes and libraries).
• The tests are not CDI discoverable
  1. Current behavior: explicit addClass on CDI container for all step classes before it is started
  • The step instances will be resolved from the CDI container with all injected beans in it (from main classes and libraries only).
  2. Proposed behavior: without explicit addClass on CDI container for all step classes
  • The step instances will be loaded as an UnmanagedInstance, the CDI container will inject managed beans in it (from main classes and libraries only).

Unless I'm missing something, the 2 options when the tests are not CDI discoverable have the same result.

Am I missing something though?

[mpkorstanje]

addClass is not called so from the second scenario forward if the CDI container does not know about the Step class then it is manually loaded as an UnmanagedInstance (inside the getInstance method).

There is an assumption that the factory keeps a list of classes and add them to the context as necessary.

https://github.com/cucumber/cucumber-jvm/blob/main/picocontainer/src/main/java/io/cucumber/picocontainer/PicoFactory.java

So essentially:

public final class PicoFactory implements ObjectFactory {

    private final Set<Class<?>> classes = new HashSet<>();
    private MutablePicoContainer pico;



    public void start() {
        pico = new PicoBuilder().....build();
        
        for (Class<?> clazz : classes) {
            pico.addComponent(clazz);
        }
        pico.start();
    }

    public void stop() {
        pico.stop();
        pico.dispose();
    }

    public boolean addClass(Class<?> clazz) {
        if (isInstantiable(clazz) && classes.add(clazz)) {
            addConstructorDependencies(clazz);
        }
        return true;
    }
    
   ....
}

[mpkorstanje]

Currently the addClass method is called multiple times (for each step it seems) at the start of the first scenario.

That shouldn't happen but happens anyway because the streaming api makes the code look pretty rather then correct.

    @Override
    public void loadGlue(Glue glue, List<URI> gluePaths) {
        GlueAdaptor glueAdaptor = new GlueAdaptor(lookup, glue);

        gluePaths.stream()
                .filter(gluePath -> CLASSPATH_SCHEME.equals(gluePath.getScheme()))
                .map(ClasspathSupport::packageName)
                .map(classFinder::scanForClassesInPackage)
                .flatMap(Collection::stream)
                .forEach(aGlueClass -> scan(aGlueClass, (method, annotation) -> {
                    container.addClass(method.getDeclaringClass());
                    glueAdaptor.addDefinition(method, annotation);
                }));
    }

[dcendents]

The problem is not that it is called multiple times, the Weld implementation is a HashSet and I expect the same for openwebbeans

There are 2 problems.

First, they are not cached like in PicoContainer and it means the first and the other scenarios run with different configurations.

Second, this is a problem with Weld, adding classes explicitely while also scanning for CDI beans, Weld complains about ambiguous classes although it is the same class.

There are 2 fixes I can see:

1 - Cache the classes and add them on every new container.
The problem when using Weld is we need to manually exlcude the step classes in beans.xml, so this will impact potentially everyone as soon as they add the beans.xml file in the test resources folder.

2- Stop adding classes manually to the CDI container.
I have demonstrated that this is not necessary since the getInstance method already deals with the situation and creates an UnmanagedInstance and the injection points in the UnmanagedInstance are still resolved correctly.

I'm trying to push for fix # 2 as it was a pain to understand the problem and I want to prevent that for other users, but obviously I don't want to break any feature so I'm questioning you to tell me if I'm missing something that justifies why addClasses is still needed and does not simply return true like on the deltaspike module.

The only thing that it would/could break is a step class injecting another step class, but I don't see why anyone would need that. And even then by adding a beans.xml file they would become managed and it would be possible.

[mpkorstanje]

@rmannibucau how common is it to test a CDI application without a beans.xml file?

@dcendents Injecting step definitions into other step definitions is something that should be expected to work. While not pretty, or a good practice, it is a feature that makes it possible to refactor step definitions in small increments. So I'd rather not break it on a minor release. However we can schedule this as a breaking change for v7.0.0 and update the documentation to require a beans.xml file.

[dcendents]

@mpkorstanje I see you updated weld to version 4 in the readme but it is probably safer to stick with 3.1.6.Final.

As per their release notes: "Functionally, Weld 3 and 4 remain identical and both are getting the same treatment and doses of bugfixes. The main difference is that Weld 4 operates with EE 9, meaning it expects you to use it with CDI 3.0 along with the new jakarta package names."

I must say I've been out of touch with the java world lately, I was still using CDI 1.1 on widfly 10 I think, so this is the first time I use CDI 2.0 and it is strictly for my unit tests. All I can say is my project works fine with 3.1.6.Final, if I switch to 4.0.0.Final all the tests fail with "No valid CDI implementation found"

I suppose we'll need a new cucumber-cdi3 package soon ;-)

[rmannibucau]

FYI I pushed this PR #2257 to handle more cleanly the "lib" case we talked about, this can enable to make the step handling simpler and assume all are beans at some point skipping the initializer.addBeanClasses.