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

[Jonathan Gibbons]

Stuart,

While I would agree on the use of one type, not two, for file paths, I 
would question the choice to use String instead of Path.  If something 
is a file path, use the type system to say so, and use a Path.

-- Jon

On 05/20/2016 04:52 PM, Stuart Marks wrote:
> 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