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

CDI integration #46

Closed
minisu opened this issue Jan 15, 2014 · 31 comments
Closed

CDI integration #46

minisu opened this issue Jan 15, 2014 · 31 comments

Comments

@minisu
Copy link
Member

minisu commented Jan 15, 2014

Previous discussion (from other issue):

@svenruppert:

Hi, as Henrik wrote.. I am working on an CDI bootstrap for TestFX. The first tests are good. So I would like to create three maven modules. The first one would be the parent with the dep management. the second the TestFX without CDI and the third one based on the second one with CDI support.
For the combination I would use an aproch like I described in my blog: http://www.rapidpm.org/2014/01/fxcontroller-with-cdi-managed.html. Same for the CDI bootstrap. let me know if this would be ok for you all. If yes, I would create the three modules first, so that we can merge this soon to master branch.

@Philipp91:

@svenruppert Do you have something like a per-window Context/Scope for your CDI integration?
I have my own CDI bootstrap for JavaFX. Integrating this with TestFX was as easy as replacing the internally used javafx.application.Application-implementation by my own one. That's why I asked about this feature above. Maybe you could use that, too?
I don't understand what the second maven module (TestFX without CDI) is about. Wouldn't it be enough to have TestFX, CDI4FX (or whatever your CDI-JavaFX integration is called) - both of which already exist - and only one more module that contains a class like CDIGuiTest, which extends GuiTest?

@svenruppert:

@Philipp91 Well the CDI module would have more deps.. additional classes on cdi side and changes inside the non-CDI module, because I would bootstrap the tests itself too. But in a way you don´t need Arquillian. The FX stuff is mostly Java SE and if you don´t want to use CDI you don´t want to have this deps/classes inside your project.
I would say, three modules would be the clean way to do it..
If possible I would like to see your CDI bootstrap, and I think to integrate a additionaly scope would be possible.

@hastebrot:

@svenruppert I really like your concept with the three maven modules.

@Philipp91:

Okay, so your TestFX integration allows to use injection inside the test classes? That sounds great. I currently have to use methods like "getBean(SomeClass.class)" etc. ...

You can see my implementation, it is currently integrated with a project and therefore in aprivate BitBucket repository. I can add you there or send you an email. However, I wouldn't consider it stable. It needs to intercept all (JavaFX) events, which does not work in all cases. Mostly PopupWindows (used by Menu, ChoiceBox, etc.) cause problems. It works fine for this specific application, but currently, we have to use special "AttachedMenuItem"s etc. instead of the normal JavaFX classes.
Refer to https://community.oracle.com/thread/2592504 for details and to https://javafx-jira.kenai.com/browse/RT-33543 for a possible solution (scheduled for Java 9 ...). But maybe there is a different, better way to do this.
The general problem that needs to be solved is that the same thread (JavaFX main UI thread) is responsible for all windows, so at some place someone needs to tell which window the thread is currently working on.

@svenruppert:

@Philipp91 Maybe I have a basic step for a solution.. ContextResolver are usefull in my projects .. but this for later.. And yes, you can use CDI inside the tests (and the TestFX Framework itself) too.. and no Arquillian needed.. The first tests are all green.

What are the final steps?
First finishing the refactorings and tests..
After this creating the three modules ( I could do it if it is ok)
After this I would branch again for the CDI support.

@svenruppert
Copy link
Member

ok, switched to this issues now ;-)

@svenruppert
Copy link
Member

How we will decide what are the next steps ?

What are the final steps?
First finishing the refactorings and tests..
After this creating the three modules ( I could do it if it is ok)
After this I would branch again for the CDI support.

@minisu
Copy link
Member Author

minisu commented Jan 16, 2014

@svenruppert: Sounds good to me. The easiest way is if we somehow eventually end up with a pull request to master.

@Philipp91 @hastebrot: Does this sound good? Does it make sense to merge this before or after the refactoring that @hastebrot is doing?

@hastebrot
Copy link
Member

I'm not quite sure.

Here are my changes (hastebrot/TestFX@3405406) that interfere with Svens (@a91b00f).

There is a StageApplication and a StageRetriever now. A StageRetrieverImpl calls static methods on StageApplication and GuiTest instantiates the StageRetrieverImpl. The static methods are a bit nasty.

@svenruppert
Copy link
Member

@hastebrot Well, finish your refactorings, let us merge all to master.. I will integrate CDI after this.
I think, this would be the best and fastest way.
After Your refactorings I will create the three modules before I will start with CDI incl backmerge to master.

Would this be OK for you ?

@hastebrot
Copy link
Member

Ok.

@svenruppert
Copy link
Member

Please let me know, after you finished and I could start...

@svenruppert
Copy link
Member

Dear All.. any time plans so that I could plan a little bit ?

@hastebrot
Copy link
Member

@svenruppert Refactoring: Only some tasks in the pull request description are left (extract class FxRobotImpl). After that I need to finish the unit tests, ensure compatibility to the existing GuiTest API and merge the changesets from the last two weeks from master. There are also three issues (in offset(), DragDropContext and StageRetriever) I'd like to address.

Till the end of the month it could be ready, I guess.

@svenruppert
Copy link
Member

@hastebrot Hi, how the work is going on? Could you give me an estimation, please? I would like to plan some more articles about TestFX, so I need a timerange that I could give to JAXenter.de

@svenruppert svenruppert self-assigned this Feb 5, 2014
@hastebrot
Copy link
Member

@svenruppert Sorry for the delay. I reserved this weekend to finish it and plan to merge the changesets from master in the beginning of the next week.

@minisu @Philipp91 What do you think is the best way to merge your changes? git fetch origin and then git merge origin/master and resolve the conflicts manually. Or just put my branch into master and merge your changes afterwards manually.

@minisu
Copy link
Member Author

minisu commented Feb 10, 2014

@hastebrot: Personally, I think that the second alternative would be easier. I'll be away for a one week vacation, but I've made sure that you all have push access so you won't be blocked.

@aalmiray
Copy link
Contributor

May I suggest to keep any CDI specific classes out of the defualt impl and choose JSR-330 compliance instead? This way framework implementors (like Griffon and Jaxcp) can hook-in their own DI solutions.

@hastebrot
Copy link
Member

@svenruppert #39 is in master now. Have a look at GuiTest, AppRobotTestBase and DefaultAppSetupFactory for your changes.

@minisu Thanks. Have a nice vacation.

@aalmiray +1

@svenruppert
Copy link
Member

Sorry for the delay on my side.. I would start now with the modules..

@aalmiray
Copy link
Contributor

Just curious, is the CDI integration intended to be an optional module?

@svenruppert
Copy link
Member

well, a parent module for the deps, one for the se without cdi and one for the cdi integration. So the user could use TestFX with or without cdi.

@svenruppert
Copy link
Member

branch feature/46 is pushed. would be nice to get first feedback if the structure with the modules is ok for all. No other changes are done, only moving classes

@aalmiray
Copy link
Contributor

All static imports were converted to their non-static form. Was this an intended change?

@svenruppert
Copy link
Member

oh no.. intellij .. sorry , main quest will be if the structure is ok. so that we can merge it soon to master..

@aalmiray
Copy link
Contributor

Code formatting and standards, the one true Aquilles heel of FLOSS projects 😉

@svenruppert
Copy link
Member

one main quest will be.. switching to jdk8 ? ;-) any plans for this ?

@aalmiray
Copy link
Contributor

Considering that JFX8 is a much better version than JFX 2.2 I'd say yes; but proceed with caution as only a few adopters jump to newer versions of the JDK in the first months.

@hastebrot
Copy link
Member

@svenruppert @aalmiray Hi. I'd like to experiment with Gradle subprojects a bit, maybe we could switch to Gradle and use the conventions from Griffon.

@aalmiray
Copy link
Contributor

aalmiray commented Mar 1, 2014

A cursory review if the current pom lets me think that converting the project to a gradle build is a straight forward task. From then on making it a multi-project build is simple. I can cook up an alternate build.gradle file for review to get us started 😄

@hastebrot
Copy link
Member

That would be great! I'm also +1 for a drop of JavaFX 2.x support for future releases in favor of JDK 8.

@aalmiray
Copy link
Contributor

aalmiray commented Mar 1, 2014

Just sent a PR for it (#71) Watch out as all sources were moved to a subdirectory, preparing the way for multiple modules to be added later. There are a few wrinkles that need ironing out (as noted in the TODO section), but it's a start. You'll note that the original pom.xml is still there (tucked into the testfx-core subdir)

@svenruppert
Copy link
Member

good morning ... I have a question... How we could move on?
All ist still on hold. What could I do to help?

@hastebrot
Copy link
Member

Hi Sven!

I think PR #71 needs some testing. You could work through a small check list if everything works fine with Gradle. I added a comment to the PR.

@svenruppert
Copy link
Member

Good Morning,
the main question is the order we want to do the work and who is responsible for example the merge requests.. If I understud it right, we first switch to gradle and after this i can start with cdi?

@hastebrot hastebrot added this to the 3.2 milestone Apr 29, 2014
@hastebrot hastebrot modified the milestones: 4.0, 3.2 Jun 8, 2014
@brcolow
Copy link
Collaborator

brcolow commented Mar 5, 2018

Support for dependency injection frameworks is not currently planned, but a fully tested PR would be welcome.

@brcolow brcolow closed this as completed Mar 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants