alien11689 commented on code in PR #118:
URL: https://github.com/apache/aries-rsa/pull/118#discussion_r3295157068


##########
discovery/zookeeper/pom.xml:
##########
@@ -95,16 +87,51 @@
             <artifactId>mockito-core</artifactId>
             <scope>test</scope>
         </dependency>
-        <dependency>
-            <groupId>org.hamcrest</groupId>
-            <artifactId>hamcrest-all</artifactId>
-            <scope>test</scope>
-        </dependency>
+
         <dependency>
             <groupId>org.apache.felix</groupId>
             <artifactId>org.apache.felix.converter</artifactId>
             <scope>test</scope>
         </dependency>
     </dependencies>
 
+    <build>
+        <plugins>
+            <!--
+                ZooKeeper artifacts have no OSGi support, and the two 
inter-dependent bundles
+                (zookeeper and zookeeper-jute) make them difficult to just 
wrap, so we embed
+                them into our discovery bundle
+            -->
+            <plugin>
+                <groupId>org.apache.felix</groupId>
+                <artifactId>maven-bundle-plugin</artifactId>

Review Comment:
   why do we need bundle plugin when the whole project is using different 
plugins? 
https://github.com/apache/aries-rsa/blob/master/parent/pom.xml#L379-L399



##########
itests/felix/src/test/java/org/apache/aries/rsa/itests/felix/RsaTestBase.java:
##########
@@ -108,17 +109,20 @@ protected static Option echoTcpService() {
         );
     }
 
-    /**
-     * We create our own junit option to also provide hamcrest and Awaitility 
support
-     */
     protected static Option junit() {
-        // based on CoreOptions.junitBundles()
+        // we use a custom configuration and not CoreOptions.junitBundles
+        // because so hamcrest is not too old and not too new for awaitility
+        String junitVersion = MavenUtils.getArtifactVersion("junit", "junit");
+        String hamcrestVersion = MavenUtils.getArtifactVersion("org.hamcrest", 
"hamcrest");
         return composite(
-                systemProperty("pax.exam.invoker").value("junit"),
-                
bundle("link:classpath:META-INF/links/org.ops4j.pax.tipi.junit.link"),
-                
bundle("link:classpath:META-INF/links/org.ops4j.pax.exam.invoker.junit.link"),
-                mvn("org.apache.servicemix.bundles", 
"org.apache.servicemix.bundles.hamcrest"),
-                mvn("org.awaitility", "awaitility"));
+            wrappedBundle(mavenBundle("junit", "junit", 
junitVersion)).instructions(

Review Comment:
   cannot we use `asInProject()` from pax-exam?



##########
itests/felix/src/test/java/org/apache/aries/rsa/itests/felix/RsaTestBase.java:
##########
@@ -108,17 +109,20 @@ protected static Option echoTcpService() {
         );
     }
 
-    /**
-     * We create our own junit option to also provide hamcrest and Awaitility 
support
-     */
     protected static Option junit() {
-        // based on CoreOptions.junitBundles()
+        // we use a custom configuration and not CoreOptions.junitBundles
+        // because so hamcrest is not too old and not too new for awaitility
+        String junitVersion = MavenUtils.getArtifactVersion("junit", "junit");
+        String hamcrestVersion = MavenUtils.getArtifactVersion("org.hamcrest", 
"hamcrest");
         return composite(
-                systemProperty("pax.exam.invoker").value("junit"),
-                
bundle("link:classpath:META-INF/links/org.ops4j.pax.tipi.junit.link"),
-                
bundle("link:classpath:META-INF/links/org.ops4j.pax.exam.invoker.junit.link"),
-                mvn("org.apache.servicemix.bundles", 
"org.apache.servicemix.bundles.hamcrest"),
-                mvn("org.awaitility", "awaitility"));
+            wrappedBundle(mavenBundle("junit", "junit", 
junitVersion)).instructions(
+                "Bundle-SymbolicName=junit",
+                "Export-Package=org.junit.*;version=" + junitVersion + 
",junit.*;version=" + junitVersion,
+                "Import-Package=org.hamcrest;version=\"" + hamcrestVersion
+                    + "\",org.hamcrest.core;version=\"" + hamcrestVersion + 
"\""
+            ),
+            
mavenBundle().groupId("org.hamcrest").artifactId("hamcrest").versionAsInProject(),

Review Comment:
   isn't `mvn(...)` exactly for it?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to