Ping - Re: RFR 8078812, Test RMI with client and servers as modules

[Stuart Marks]

On 5/16/16 1:18 AM, Felix Yang wrote:
>    please review the updated webrev, which remove not-suggested filesystem
> modification and codebase stuff:
>
> http://cr.openjdk.java.net/~xiaofeya/8078812/webrev.02/

Hi Felix,

OK, this is looking much better and simpler. The big improvement is, as we had 
discussed, the creation of a common fixture for the tests. The tests then use 
the same fixture in different ways to exercise the different cases. This makes 
this more easily extensible to additional cases.

First to be done are a few easy cleanups:

* Since we're no longer using a registry, the starting and stopping of the 
registry process in the start- and shutdownRMIRegistry() methods is no longer 
necessary. They can be removed entirely, along with the rmiRegistry field.

* The imports they required, along with some other unused import statements, can 
also be removed.

* The fixture setup method, compileAll(), should be annotated @BeforeTest 
instead of @BeforeMethod. Compiling once at the beginning of the test should be 
sufficient.

With these out of the way, my observation is that it's still really quite 
difficult to follow what the test is doing, particularly in setting up the text 
fixture.

One reason for this is the repeated conversions between Path and String. Some 
places need Paths and some need Strings. Path elements are concatenated in some 
places with path.resolve(String) and string concatenation with File.separator or 
File.pathSeparator in others. In some cases, fields such as

     MODS_DIR = Paths.get("mods")

are defined when in my opinion it'd just be better to use the string literal 
"mods" in a couple places.

In any case I've taken the liberty of posting some cleanups to a branch in the 
sandbox. I've written a couple utilities to concatenate strings using 
File.separator and File.pathSeparator, and I've kept things mostly as strings, 
only converting to Paths when absolutely necessary. I've also renamed the 
directory of compiled classes as the "exploded" directory since that's sort-of 
the descriptive term in use for a hierarchy of individual class files.

The sandbox branch is JDK-8078812-branch and the diff from your webrev can be 
viewed here:

     http://hg.openjdk.java.net/jdk9/sandbox/jdk/rev/befcc172e68e

and the ModuleTest.java file (the only one I modified) can be viewed here:

 
http://hg.openjdk.java.net/jdk9/sandbox/jdk/file/befcc172e68e/test/java/rmi/module/ModuleTest.java

It's up to you whether you want to accept my changes and continue from this 
point, or go in a different direction, but to my eye this is cleaner and easier 
to follow.

* * *

Now, finally, on to more substantive review issues. :-)

One thing that seemed to be missing was that the application itself wasn't 
wrapped up into a jar file. I've added another Jar command that does this.

Now we have the client, server, and app as separate hierarchies under the 
"exploded" directory, and as modules under the "mods" directory.

I think the idea of having the "exploded" class hierarchy as well as jar files 
useful as modules is a good one. This will let you add cases, where different 
components are on the classpath or are loaded as modules, in addition to the two 
already present here.

One issue here is that there's a module-info class for the app. This makes the 
app an actual named module (I think) as opposed to an automatic module like the 
client and server jar files. It seems like it would be preferable to be 
consistent and have them all be automatic modules.

Given this arrangement, it should be pretty easy to have tests for any of the 
combinations we want of classpath vs modules. I guess there are 8 combinations: 
three components, each of which can be on the classpath or as a module. It's not 
clear to me that we need all 8 combinations. It's probably sufficient to have a 
reasonable subset.

An idea for possible future expansion is to mix automatic modules with named 
modules. I'm not entirely sure how to do that. Perhaps one way is to have 
module-info files for all the components, and then create variant jar files with 
the module-info.class omitted. That way we'd have a modular jar, and then a 
"plain" jar (without module-info.class) that'd be suitable for use as an 
automatic module or to be put on the classpath. That'd be 3^3=27 combinations, 
which I certainly think is overkill.

In any case, for this initial changeset, I think it's sufficient to test a few 
combinations of automatic modules vs. classpath. We can extend the cases to 
include named modules later. Please make a recommendation for some set of 
combinations and implement it, and then send it out for a final round of review.

Thanks.

s'marks