Upgrade project to Java EE 7 (+ TomEE 7 webprofile) #14

mojavelinux · 2016-12-25T10:20:19Z

upgrade to TomEE 7.0.2 webprofile
switch to the Java EE 7 API (TomEE provided)
add Arquillian BOM
upgrade to Arquillian 1.1.11.Final
set Java source/target compatibility to 1.8
use JAX-RS 2 client API in tests
add JAX-RS 2 client API and providers to test runtime
annotate Arquillian test with @RunAsClient
add JAX-RS application initializer (required for portability)
upgrade Gradle wrapper to 3.2.1
fix Gradle build, add Arquillian dependencies
rename ColorService to ColorResource
use consistent project name
exclude version from name of war file
update instructions in README
fix AsciiDoc syntax in README
stub in TomEE remote adapter and configuration for tests

mojavelinux · 2016-12-25T10:23:34Z

@dblevins Here's your xmas gift from me and the Arquillian family! 👽 🎄 🎁

You may want to create a javaee-7 branch upstream so master doesn't shadow the setup for Java EE 6 (and break the reference in the blog post). You could also move master to a javaee-6 branch and make this version the master. I'm fine with either approach, as long as this version is accessible.

rmannibucau · 2016-12-25T10:45:26Z

src/test/java/org/superbiz/ColorResourceTest.java

    /**
     * Reusable utility method
     * Move to a shared class or replace with equivalent
     */
-    public static String slurp(final InputStream in) throws IOException {
+    private String slurp(final InputStream in) throws IOException {


can be removed using response.readEntity(String.class), no?

Ah, of course! Thanks for the tip.

rmannibucau · 2016-12-25T10:45:37Z

src/test/java/org/superbiz/ColorResourceTest.java

+            client.register(Class.forName(JSON_PROVIDER_CLASS));
+        }
+        catch (ClassNotFoundException e) {}
+        return client.build().target(webappUrl.toURI()).path(getApplicationPath());


don't forget to close the client ;)

rmannibucau · 2016-12-25T10:46:28Z

src/test/java/org/superbiz/ColorResourceTest.java

+        final ClientBuilder client = ClientBuilder.newBuilder();
+        try {
+            // client side
+            client.register(Class.forName(JSON_PROVIDER_CLASS));


why not using "new" since the class is hardcoded? In embedded mode we should also inherit from the provider by default IIRC (not in remote indeed but you used embedded adapter there)

I stubbed in support in the pom for switching to the remote adapter, so I made the test portable. I didn't reference the JohnzonProvider directly so that the class would compile without any adapters or extra jars on the classpath.

rmannibucau · 2016-12-25T10:47:15Z

src/test/java/org/superbiz/ColorResourceTest.java

 */
 @RunWith(Arquillian.class)
-public class ColorServiceTest extends Assert {
+@RunAsClient


why not testable=false which avoids to pollute the war and run as client implicitely (or both if you want it to be obvious)?

That's exactly what @RunAsClient does. If it's set on the class, it automatically implies testable=false on all deployments. You only need the testable flag if you're trying to mix client and server test cases in the same test class.

Hmm. It turns out that despite the fact that Aslak and I discussed this, it doesn't seem to have ever been implemented in core. So the testable=false is still required to keep the test class from being deployed. But we will want @RunAsClient so that the tests are executed from outside the server.

yes cause with some component like warp you need to enrich the archive but run on client side. Issue is only the naming of "testable" which should have been "enrichable" or something like that.

Exactly. The annotation needs work. It's too ambiguous at the moment. We'll take it upstream.

rmannibucau · 2016-12-25T10:48:24Z

pom.xml

-      <groupId>org.apache.openejb</groupId>
-      <artifactId>arquillian-tomee-embedded</artifactId>
-      <version>${tomee.version}</version>
+      <groupId>org.apache.geronimo.specs</groupId>


shouldnt be needed in embedded mode, which dependency do we miss to require it?

This dependency, as well as cxf-rt-rs-client and johnzon-jaxrs, are required for running the test with TomEE remote. I've moved these dependencies to a comment block and documented when they need to be enable. You are correct that they are not required to run the test on embedded.

rmannibucau · 2016-12-25T10:49:11Z

pom.xml

-      <version>${tomee.version}</version>
+      <groupId>org.apache.cxf</groupId>
+      <artifactId>cxf-rt-rs-client</artifactId>
+      <version>${cxf.version}</version>


same, should be in tomee jaxrs transitively fetched by arquillian adapter

rmannibucau · 2016-12-25T10:49:14Z

pom.xml

+    <!-- NOTE JAX-RS JSON integration must be provided on client side -->
+    <dependency>
+      <groupId>org.apache.johnzon</groupId>
+      <artifactId>johnzon-jaxrs</artifactId>


same, should be in tomee jaxrs transitively fetched by arquillian adapter

Again, only needed for the remote adapter. The reason for the very specific dependencies for that scenario was to limit number of items on the classpath. I figured out exactly which jars were needed.

mojavelinux · 2016-12-25T21:09:36Z

Thanks for the feedback! I'll make the necessary updates.

mojavelinux · 2016-12-26T04:17:52Z

I've updated the project.

I kept the configuration for the TomEE remote adapter in place, but put it in comments so it's easy to swap in. Personally, I prefer to test on a server that's isolated from the test so the dependencies and classpath of the server don't leak into my test. For me, it's more clear what's going on this way.

(Btw, the TomEE remote adapter should really be named TomEE managed. As far as I'm concerned, TomEE doesn't have a remote adapter).

- upgrade to TomEE 7.0.2 webprofile - switch to the Java EE 7 API (TomEE provided) - add Arquillian BOM - upgrade to Arquillian 1.1.11.Final - set Java source/target compatibility to 1.8 - use JAX-RS 2 client API in tests - add JAX-RS 2 client API and providers to test runtime - annotate Arquillian test with @RunAsClient - add JAX-RS application initializer (required for portability) - upgrade Gradle wrapper to 3.2.1 - fix Gradle build, add Arquillian dependencies - rename ColorService to ColorResource - use consistent project name - exclude version from name of war file generated by Maven - update instructions in README - fix AsciiDoc syntax in README - stub in TomEE remote adapter and configuration for tests

rmannibucau · 2016-12-26T08:12:32Z

Looks good to me now (will not discuss more the provider point even if portability doesn't exist in this area until we have jsonb by default).

@dblevins how do you want to manage the branches? Personally I'm fine getting this PR on master and updating the post with links using the commit sha but 2 branches are ok too.

mojavelinux force-pushed the javaee-7 branch from af71a83 to 94d1970 Compare December 25, 2016 10:20

rmannibucau reviewed Dec 25, 2016

View reviewed changes

mojavelinux force-pushed the javaee-7 branch from 94d1970 to 831596b Compare December 26, 2016 04:14

mojavelinux force-pushed the javaee-7 branch from 831596b to ef92fd2 Compare December 26, 2016 04:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade project to Java EE 7 (+ TomEE 7 webprofile) #14

Upgrade project to Java EE 7 (+ TomEE 7 webprofile) #14

mojavelinux commented Dec 25, 2016

mojavelinux commented Dec 25, 2016

rmannibucau Dec 25, 2016

mojavelinux Dec 26, 2016

rmannibucau Dec 25, 2016

mojavelinux Dec 26, 2016

rmannibucau Dec 25, 2016

mojavelinux Dec 26, 2016

rmannibucau Dec 25, 2016

mojavelinux Dec 26, 2016

mojavelinux Dec 26, 2016

rmannibucau Dec 26, 2016

mojavelinux Dec 26, 2016

rmannibucau Dec 25, 2016

mojavelinux Dec 26, 2016

rmannibucau Dec 25, 2016

rmannibucau Dec 25, 2016

mojavelinux Dec 26, 2016

mojavelinux commented Dec 25, 2016

mojavelinux commented Dec 26, 2016

rmannibucau commented Dec 26, 2016

Upgrade project to Java EE 7 (+ TomEE 7 webprofile) #14

Are you sure you want to change the base?

Upgrade project to Java EE 7 (+ TomEE 7 webprofile) #14

Conversation

mojavelinux commented Dec 25, 2016

mojavelinux commented Dec 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mojavelinux commented Dec 25, 2016

mojavelinux commented Dec 26, 2016

rmannibucau commented Dec 26, 2016