search

ops4j / org.ops4j.pax.exam2

Pax Exam is a testing framework for OSGi
https://ops4j1.jira.com/wiki/spaces/PAXEXAM4/
Apache License 2.0
84 stars 100 forks source link

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

Closed ops4j-issues closed 5 months ago

ops4j-issues commented 9 years ago

Roland Hauser created PAXEXAM-743

You can find the according pull-request here: https://github.com/ops4j/org.ops4j.pax.exam2/pull/41

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

ops4j-issues commented 9 years ago

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 commented 9 years ago

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.