Skip to content

[DeltaSpike] Add ObjectFactory for Apache DeltaSpike #1616

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

Merged
merged 4 commits into from
Aug 31, 2019

Conversation

toepi
Copy link
Contributor

@toepi toepi commented Apr 28, 2019

Summary

objectfactory for cdi with deltaspike

Details

Deltaspike abstract underlining cdi-container. so it is possible to change cdi-impl. only by replace the dependencies.

Motivation and Context

No longer to modules for cdi, only one with can replace the old one (weld and openejb) and in the same time support for another cdi-impl (OpenWebBeans).

How Has This Been Tested?

Add Testcases and profiles to change cdi-impl.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue).
  • New feature (non-breaking change which adds functionality).
  • Breaking change (fix or feature that would cause existing functionality to not work as expected).

Checklist:

  • I've added tests for my code.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@coveralls
Copy link

coveralls commented Apr 28, 2019

Coverage Status

Coverage increased (+0.03%) to 87.853% when pulling 0775763 on toepi:objectfactory_deltaspike into 323fe14 on cucumber:master.

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.

Could you add a README.md to explain how this module should be used? It doesn't appear to be trivial.

@toepi toepi force-pushed the objectfactory_deltaspike branch from 849e1bc to e4970d5 Compare April 29, 2019 21:01
@mpkorstanje mpkorstanje changed the title add module for CDI with deltapike [DeltaSpike] Add ObjectFactory for Apache DeltaSpike May 5, 2019
@toepi
Copy link
Contributor Author

toepi commented May 5, 2019

Have found this old pr #971 this should be deprecated with this one?

@mpkorstanje
Copy link
Contributor

I'm happy to take both PR's. While CDI containers are used only by a fraction of the users they're relatively low maintenance and have a relatively high number of contributors.

I do however have a rather limited knowledge of either so I'd like both implementations to be correct, in line with the frameworks expected use (so as to limit future questions/issues) and focused on the basic use case. For advanced use cases I expect that people re-implement the factory. At least until we come up with something better.

Also if you can both review and approve each others PR's then I can have some confidence that both implementations are good.

@mpkorstanje
Copy link
Contributor

I've pushed some fixes so it will work wit the latest 5.0.0-SNAPSHOT. Would you mind adding a short guide on how to use this module in the read me?

@toepi
Copy link
Contributor Author

toepi commented Aug 15, 2019

sry complete forgotten this pr, will try to add requested guide next week. for 5.0.0 I think this objectfactory should replace weld and openejb do you think I should update this pr to do this also?

@mpkorstanje
Copy link
Contributor

Cheers.

Generally speaking its good practice not to remove something withouth a deprecation period. It also doesn't hurt to keep weld around for a bit. So leave it alone for now.

@rmannibucau
Copy link
Contributor

Side note: this does not fully superseed openejb due to DD impl but an EJBContainer based impl could happily do it and enable to support tomee at the same time - can also be done behind DS API if you want to do the pr ;).

@mpkorstanje
Copy link
Contributor

That'd be quite useful. If you're interested in doing this please do go ahead.

@toepi
Copy link
Contributor Author

toepi commented Aug 18, 2019

after reading cucumber/docs#369, do you think it will be better to replace container.boot() and container.shutdown() with somthink to start and stop a given scope (e.g. requestedscope)?

@rmannibucau
Copy link
Contributor

Makes sense if configurable cause any setup can break tests from my experience. Often request is started by default but you can need session or need to not have request commonly too. Depends how you write it and which container you use sometimes.

Can make more sense to decorate step instance with an interceptor starting/stopping scope at request (@scopes on the step ?).

@mpkorstanje
Copy link
Contributor

mpkorstanje commented Aug 18, 2019

Cucumber currently puts step definitions in the container. Because step definitions generally speaking get dirty the sensible default would be to restart the container completely.

If this operation is expensive you may also consider removing only the step definition classes (everything provided by addClass) and keeping the container alive. However step definitions should be recreated for each scenario.

@rmannibucau
Copy link
Contributor

Operation is cheap but it is a state app code can rely on, this is why it must be configurable IMO. That said, can be a "given" statement with implicit close.

@mpkorstanje
Copy link
Contributor

mpkorstanje commented Aug 18, 2019

I'm not following. Each scenario should run in an clean context. The following feature should always pass no matter what FOO might be.

Feature: Don't share state between scenarios
  Scenario: I dirty the context
      Given I set FOO from BAR to BAZ

  Scenario: The dirty context should gone
       Then FOO should be set to BAR again

Do you agree?

@rmannibucau
Copy link
Contributor

Depending how you run or impl each step context can be different. Typically a client step will have a request context per http request whereas a server step can share it state within the scenario. This is why it must be configurable.
Also keep in mind some lib test if the context is active or not - jta for instance - so starting it by default can make the test silently broken.

@mpkorstanje
Copy link
Contributor

Typically a client step will have a request context per http request whereas a server step can share it state within the scenario.

What are sever steps and client steps? Could you write some scenarios to show the difference.

Also keep in mind some lib test if the context is active or not - jta for instance - so starting it by default can make the test silently broken.

I don't see how this is relevant. Could you write some scenarios to show when it matters?

@rmannibucau
Copy link
Contributor

  1. Selenium vs service testing (famous belly one ;))
  2. If your logic activates stuff based on the scope activation or not then the app behavuor differs. Typically if you start request scope when it shouldnt, transactional scope lifecycle will differ. It is true for several libs so each scenario should be able to define in which context it runs.
    In tomee embedded we prestarted request scope and it broke some users tests so we end up making it fully configurable.
    I dont have a computer handy but can try to shape something next week.
    We can also let it like that and enhance it at need - users can have a single usage for now :crossing_fingers:.

@mpkorstanje
Copy link
Contributor

Let's start with no scope activated (basically selenium). As defaults go that is pretty sensible.

Then we can work out how to activate other scopes, but given that object factory needs to start the context before the scenario can start I don't think it will be possible. Unless you can change the scope once the factory has started. In that case you tag a scenario with the required scope and used tagged hooks to setup that scope.

allow to replace cdi-impl without any change at cucumber.
@toepi toepi force-pushed the objectfactory_deltaspike branch from 2e7d8fe to 29e1362 Compare August 20, 2019 11:19
@toepi
Copy link
Contributor Author

toepi commented Aug 25, 2019

rebased to master and add a readme, but somebody with better English should take a look.

@toepi toepi force-pushed the objectfactory_deltaspike branch from 29e1362 to 0775763 Compare August 30, 2019 12:35
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.

Looks good to me! Anything you'd like to add before I merge this?

@toepi
Copy link
Contributor Author

toepi commented Aug 30, 2019

Currently I do not see anything missing, please merge it and hope we find some user which get some input what we have forgotten.

@mpkorstanje mpkorstanje merged commit f9f4d61 into cucumber:master Aug 31, 2019
@mpkorstanje
Copy link
Contributor

All merged. Thanks

@toepi toepi deleted the objectfactory_deltaspike branch August 31, 2019 08:09
@mpkorstanje mpkorstanje mentioned this pull request Nov 27, 2022
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 this pull request may close these issues.

4 participants