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

Add test-container for Eclipse Sisu to Pax-Exam [PAXEXAM-743] #840

Closed
ops4j-issues opened this issue Oct 20, 2015 · 2 comments
Closed

Add test-container for Eclipse Sisu to Pax-Exam [PAXEXAM-743] #840

ops4j-issues opened this issue Oct 20, 2015 · 2 comments
Milestone

Comments

@ops4j-issues
Copy link

Roland Hauser created PAXEXAM-743

You can find the according pull-request here: #41


Affects: 4.6.0
Fixed in: 4.x
Votes: 0, Watches: 2

@ops4j-issues
Copy link
Author

Harald Wellmann commented

First of all, thanks for this PR, and sorry for taking so long to review this contribution.

All in all, I do like the idea of including a Sisu container in Pax Exam, it's just a natural extension of the existing concepts.

The only major issue I see with the current implementation is the (ab)use of the CDI APIs. Using pax.exam.system = cdi and pax-exam-cdi dependency just doesn't fit. Sisu is not a CDI implementation.

So while this approach was probably the quickest solution, I don't think it's very clean, and it would be more fitting to introduce a JSR330 mode (for any DI containers that implement JSR330 but not CDI).

In addition, before integrating this new container, I'd like to see some integration tests along the lines of what you see in itest/cdi. Your https://github.com/SourcePond/paxexam-sisu-demo.git repo looks like a good starting point.

And finally, there's the usual trivia like formatting (there's an Eclipse formatter in https://github.com/ops4j/org.ops4j.pax.exam2/blob/master/assets/EclipseJavaFormatter.xml), Checkstyle and SonarQube warnings.

@ops4j-issues
Copy link
Author

Roland Hauser commented

Thank you for the review. I agree, it's indeed not a clean solution. I will dive a little more into the Pax-Exam code this weekend to implement a "jsr330" mode . Additionally, I will extend the integration-tests. Sorry for my late response.

@ops4j-issues ops4j-issues added this to the 4.x milestone Feb 23, 2021
@oliverlietz oliverlietz closed this as not planned Won't fix, can't repro, duplicate, stale Jun 2, 2024
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

2 participants