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

enable to customize cdi2 container initializer #2257

Conversation

rmannibucau
Copy link
Contributor

Is your pull request related to a problem? Please describe.

Enable to use the CDI container initializer configuration using system properties.

Describe the solution you have implemented
Read cucumber.cdi2.* properties and wire them + a SPI to make it code first (easier in some cases, in particular for libraries).

@codecov
Copy link

codecov bot commented Mar 7, 2021

Codecov Report

Merging #2257 (bce1a22) into main (dde1d4c) will decrease coverage by 0.62%.
The diff coverage is 17.72%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #2257      +/-   ##
============================================
- Coverage     83.01%   82.38%   -0.63%     
- Complexity     2340     2342       +2     
============================================
  Files           307      308       +1     
  Lines          8324     8401      +77     
  Branches        768      771       +3     
============================================
+ Hits           6910     6921      +11     
- Misses         1110     1175      +65     
- Partials        304      305       +1     
Impacted Files Coverage Δ Complexity Δ
...ucumber/cdi2/internal/CustomizableInitializer.java 15.78% <15.78%> (ø) 1.00 <1.00> (?)
...i2/src/main/java/io/cucumber/cdi2/Cdi2Factory.java 43.95% <18.33%> (-53.02%) 11.00 <3.00> (+1.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...2d90865. Read the comment docs.

Copy link
Contributor

@mpkorstanje mpkorstanje left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So a few problems:

  1. System properties aren't quite universal. We have a number of different ways to pass properties, this would be yet another.
  2. I'm not happy with all the logic involved in findPackage and toClasses. I would rather see that we burden the user of Cucumber with these problems.

I would suggest we do something like the InjectorSource in cucumber-guice. And I would suggest doing something like we do with cucumber-spring where the SpringBackend tries to find a specific class and the SpringFactory looks out for that class.

So as a quick sketch:

package com.example.stepdefinitions;


class MyCucumberSeContainerCustomizer implements CucumberSeContainerCustomizer {

    public void customize(SeContainerInitializer sci) {
       // do customizations here    
    }

}

@rmannibucau
Copy link
Contributor Author

@mpkorstanje

  1. I'm fine reading properties from anywhere, not sure where you prefer here. overall rational is that configuring system properties to refine a container (and not always configure "first") is very neat since it can be done in surefire or any test launcher just by configuration and build launcher overrides (maven for ex) whereas other solutions are more intrusive AFAIK.
  2. about toClasses I'm not sure, it is quite common and direct. About findPackage I can agree but it is also what CDI impls do generally so don't think it is a big deal and it enables the feature which is quite common. Worse case we can document it better I guess? What should absolutely avoided is a 3rd scanning IMHO (cucumber scans, cdi will scan, we don't want to scan another time).
  3. the SPI option was nice cause it enables to compose them instead of just having a single class - which means coding again to import multiple customizers. That said as a first step a class can be ok-ish, will just enforce code for each setup and do a custom composition instead of a built-in one but it solves the "setup" issue this PR was coming from.

@mpkorstanje
Copy link
Contributor

I'm not suggesting SPI, I'm suggesting the customizer is part of the glue. This means there can be multiple customizers and depending on how Cucumber is configured different once are used.

@rmannibucau
Copy link
Contributor Author

@mpkorstanje I see, I'm fine with that for a first version. Do you want to take over the PR to do it?

@mpkorstanje
Copy link
Contributor

I wasn't intending to.

Would you be interested in adding a cdi2 backend that looks for implementations of io.cucumber.cdi2.SeContainerInitializer and adds them to the object factory?

@rmannibucau
Copy link
Contributor Author

@mpkorstanje I played a bit more with such pattern and came back to an old issue (fixed now): ObjectFactory = Container + Lookup. So overall this PR is useless if we get a CDILookup and CustomCDIContainer (which is way more easier to manage than a SPI whereever it is) or just a PassthroughContainer assuming the container is started with another extension before cucumber runs (which is also generally easy to do with junit like runners).
With that element I totally forgot when doing this PR, I tend to think we should package better CDI to enable this split and get rid of the customizers, wdyt?

@aurelien-reeves aurelien-reeves added the ⚡ enhancement Request for new functionality label Apr 26, 2021
@iliadelo
Copy link

iliadelo commented Oct 8, 2021

Hi everyone , as i can see cdi2 uses a weld container.

It would amazing if we could use WeldInitiator (Weld junit) as a CDI container for our cucumber scenarios because its fully customizable.
Also reads from test beans xml where you can customize everything. Works amazing with Junit4&5.

WeldInitiator weld = WeldInitiator.from(WeldInitiator.createWeld().enableDiscovery().addExtensions(ConfigExtension.class)
                                                                       .property("org.jboss.weld.se.archive.isolation", "false")
    ).activate(RequestScoped.class, ApplicationScoped.class)
                                                    .inject(this)
                                                    .setPersistenceContextFactory(SomePersistence)
                                                    .setEjbFactory(someFactory)
                                                    .setPersistenceUnitFactory(someUnit)
                                                    .build();

@rmannibucau
@mpkorstanje

@mpkorstanje
Copy link
Contributor

@iliadelo if you have a feature request you may want to start a new issue.

@mpkorstanje
Copy link
Contributor

@rmannibucau

So overall this PR is useless if we get a CDILookup and CustomCDIContainer (which is way more easier to manage than a SPI whereever it is) or just a PassthroughContainer assuming the container is started with another extension before cucumber runs (which is also generally easy to do with junit like runners).
With that element I totally forgot when doing this PR, I tend to think we should package better CDI to enable this split and get rid of the customizers, wdyt?

Don't think it will work because there is nothing outside of Cucumber to start the container. I suppose you could use the before-all/after-all hooks to start a single static instance. But at that point you don't need CDI for DI any more.

@mpkorstanje
Copy link
Contributor

I think this got stuck. So I've written #2439. Let's see if any one wants to contribute a solution like that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚡ enhancement Request for new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants