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

[Felix Yang]

Hi Stuart,
 any comment on the new tests and issue?

Thanks,
Felix
On 2016/4/8 15:52, Felix Yang wrote:

Hi Stuart,
 thanks for the comments. I adjusted the test.  The test failed 
with NoClassDefFoundErr if client/server are in named module but dummy 
app in unnamed module. See 
https://bugs.openjdk.java.net/browse/JDK-8153833


New webrev: http://cr.openjdk.java.net/~xiaofeya/8078812/webrev.01/ 



About simplifying the scenario, I replaced inheritance by moving 
classes in runtime.


Thanks,
Felix

On 2016/3/2 10:28, Stuart Marks wrote:

Hi Felix, sorry for the delay.

Several comments below.

--

 119 // wait for rmiregistry to be fully ready
 120 Thread.sleep(3000);

There are some RMI test utilities that will handle starting up and 
waiting for the registry to be ready. Unfortunately they're in the 
unnamed package (still haven't cleaned that up) so you can't use them 
from this test.


For now I'd recommend scaling the timeout by the test.timeout.factor, 
so that this won't fail on slow systems. There's a test utility for 
that; see Utils.adjustTimeout().


--

The directory "unamed" is misspelled; it has classes in the unnamed 
module. This misspelling is reflected in variable names and values, 
e.g.,


  59 private static final Path UNAMED_SRC_DIR = 
Paths.get(TEST_SRC, "unamed");

  60 private static final Path MODS_OUTPUT_DIR = Paths.get("mods");
  61 private static final Path UNAMED_OUTPUT_DIR = 
Paths.get("unamed");


While spelling errors aren't that big a deal, since this involves 
file and directory names, I'd recommend fixing this now. It'll just 
be harder to fix it later.


Also, "SeperateModuleClient" => "SeparateModuleClient"

--

Overall the structure of the test seems reasonable to test various 
clients and servers in combinations of the same, different, and 
unnamed modules.


I'm not entirely sure what p2.SeperateModuleClient is testing. It 
extends p1.Client and its constructor and testStub() method simply 
call the corresponding methods on super, so it doesn't seem to be 
testing anything different from p1.Client running against p3.Server.


Also, p4.UnamedModuleClient seems to want to run the client from the 
unnamed module, but it too extends p1.Client and simply defers all of 
its execution to its superclass. So I don't think this actually 
testing an RMI client call originating from an unnamed module.


There is an uncomfortable amount of duplication between 
mtest/test/DummyApp.java and p4/UnamedModuleDummyApp. I see what this 
is trying to do, which is to test a RMI server from an unnamed 
module. It instantiates p3.Server, which resides in a named module, 
but exports it from an unnamed module.


So I think there's some tension here. There's subclassing among the 
clients that attempts to avoid duplication, which is good, but it 
doesn't truly seem to be testing clients in different modules and in 
an unnamed module. The server, on the other hand, does seem to be 
testing things properly in different modules, at the cost of 
duplicating all the code into another class that resides in a 
different (unnamed) module.


I'm not entirely sure what the best approach is here. Ideally you'd 
want to have a single implementation of client, server + remote 
interface, and application, and compile each of them once. Then since 
you're invoking a new JVM for each test, invoke it with different 
options to put the client, or the server, or the app, into modules, 
or onto the classpath (to get into an unnamed module). You might have 
to copy compiled class files to different locations so that different 
classes can be either on the classpath or in a named module without 
causing any conflicts.


I'm willing to entertain some simplifications here as well. It might 
be sufficient to deal with just clients and servers in 
different/unnamed modules, and not worry about putting the 
application into different modules. That should reduce the number of 
cases to cover.


s'marks

On 2/29/16 10:05 PM, Felix Yang wrote:

Ping...

-Felix
On 2016/2/24 14:06, Felix Yang wrote:

Hi all,
  please review the new tests to use RMI in module world.

Webrev:
http://cr.openjdk.java.net/~xiaofeya/8078812/webrev.00/
Bug:
https://bugs.openjdk.java.net/browse/JDK-8078812

Thanks,
Felix

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

[Stuart Marks]

Hi Felix,

I finally got a chance to take a good look at this. Again, sorry for the delays. 
I was able to reproduce the failure and find a fix for it. There are also some 
structural issues with the test that will require some improvements.


* * *

First is the way the test fixture is set up. The test now has a single copy of 
the source code for the client, server, and app, and they're compiled once. This 
is good. Unfortunately, the tests modify the filesystem in order to set up 
different cases for the classes' modular structure.


The problem is that makes the tests dependent on each other. The Test-NG 
directives enforce this execution order, but it makes them really hard to work 
with. For example, if I were to disable test #2, this would cause test #3 to 
fail since #3 depends on side effects from test #2. Also, if test #2 were to 
fail, it's hard to debug, since test #3 modifies the fixture after test #2 runs, 
potentially obscuring any problems that #2 might have run into.


Ideally the test should set up a single test fixture and not modify it, allowing 
the different cases to be run independently. Offhand I'm not sure of the best 
way to do this.


One approach that might be worth exploring is to create several different jar 
files from the class files; the jar files would have the classes in different 
modular configurations. Then, each test would invoke a JVM with arguments to put 
the different jar files on the classpath or the module path. I haven't 
investigated this approach, though, so I don't know if it's practical.


Other approaches may be viable as well. It's probably worth discussing and 
prototyping an approach first before you spend too much time implementing any 
particular scheme.


* * *

Moving on to the actual test failure, the message from testUnnamedApp() is as 
follows:


==
Command line: 
[/Users/src/jdk-dev/jdk9-dev/build/macosx-x86_64-normal-server-fastdebug/jdk/bin/java 
-ea -esa -mp mods 
-Djava.rmi.server.codebase=file:/Users/src/jdk-dev/jdk9-dev/jdk/testoutput/JTwork/java/rmi/module/ModuleTest/./mods/mserver/ 
-cp classes testpkg.DummyApp 65071 ]


Error: A JNI error has occurred, please check your installation and try again
Exception in thread "main" java.lang.NoClassDefFoundError: serverpkg/Hello
at java.lang.Class.getDeclaredMethods0(java.base/Native Method)
at java.lang.Class.privateGetDeclaredMethods(java.base/Class.java:2937)
at java.lang.Class.privateGetMethodRecursive(java.base/Class.java:3282)
at java.lang.Class.getMethod0(java.base/Class.java:3252)
at java.lang.Class.getMethod(java.base/Class.java:1961)
at 
sun.launcher.LauncherHelper.validateMainClass(java.base/LauncherHelper.java:648)
at 
sun.launcher.LauncherHelper.checkAndLoadMain(java.base/LauncherHelper.java:499)
Caused by: java.lang.ClassNotFoundException: serverpkg.Hello
	at 
jdk.internal.loader.BuiltinClassLoader.loadClass(java.base/BuiltinClassLoader.java:366)
	at 
jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(java.base/ClassLoaders.java:184)

at java.lang.ClassLoader.loadClass(java.base/ClassLoader.java:419)
... 7 more

test ModuleTest.testUnnamedApp(): failure
java.lang.AssertionError: expected [0] but found [1]
[remainder of stack trace elided]
==

The main class is testpkg.DummyApp, but the launcher fails with 
NoClassDefFoundError on serverpkg/Hello. This was a bit of a puzzle, but I 
finally figured out that this is occurring during the verification of the 
DummyApp class. DummyApp depends on Hello, which can't be found, so it fails 
verification before DummyApp.main() gets called.


It's also strange that it says "A JNI error has occurred" but that might be a 
red herring.


In any case the real issue is that serverpkg.Hello can't be found. The reason it 
can't be found is that it's in a module. That module is on the module path, but 
the module isn't resolved by default. Thus its classes aren't visible to classes 
in the unnamed module.


To add modules to the set of resolved modules, use the -addmods option. In this 
case, testUnnamedApp() has the client and server classes in modules, so it needs 
to have the arguments


-addmods mclient,mserver

added to its command line. Since the mclient module explicitly depends on the 
mserver module, this could instead just e


-addmods mclient

The testUnnamedAppandClient() method has only the server in a named module, so 
it needs to have


-addmods mserver

on its command line.

With these changes, all the tests pass.

* * *

There are a couple other things that could be improved as well. I'll just 
mention them here (so I don't forget) but we should work on these later:


1) The registry and codebase stuff is probably unnecessary and can be removed. 
These might be interesting test cases for the future. It's not clear to me that 
we need to suppo

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

[Felix Yang]

Hi Stuart,

thanks a lot for the comments!

-Felix
On 2016/4/28 9:06, Stuart Marks wrote:

Hi Felix,

I finally got a chance to take a good look at this. Again, sorry for 
the delays. I was able to reproduce the failure and find a fix for it. 
There are also some structural issues with the test that will require 
some improvements.


* * *

First is the way the test fixture is set up. The test now has a single 
copy of the source code for the client, server, and app, and they're 
compiled once. This is good. Unfortunately, the tests modify the 
filesystem in order to set up different cases for the classes' modular 
structure.


The problem is that makes the tests dependent on each other. The 
Test-NG directives enforce this execution order, but it makes them 
really hard to work with. For example, if I were to disable test #2, 
this would cause test #3 to fail since #3 depends on side effects from 
test #2. Also, if test #2 were to fail, it's hard to debug, since test 
#3 modifies the fixture after test #2 runs, potentially obscuring any 
problems that #2 might have run into.


Ideally the test should set up a single test fixture and not modify 
it, allowing the different cases to be run independently. Offhand I'm 
not sure of the best way to do this.


One approach that might be worth exploring is to create several 
different jar files from the class files; the jar files would have the 
classes in different modular configurations. Then, each test would 
invoke a JVM with arguments to put the different jar files on the 
classpath or the module path. I haven't investigated this approach, 
though, so I don't know if it's practical.


Other approaches may be viable as well. It's probably worth discussing 
and prototyping an approach first before you spend too much time 
implementing any particular scheme.
It was chosen to compile once  considering efficiency. If we prefer each 
test can be executed separately,  it looks enough  to just change 
"@BeforeTest" to "@BeforeMethod" and update tests slightly.
With jar files, may I understand it is a different scenario with 
automatic modules. I suggest to add a new test in future enhancement.


See http://cr.openjdk.java.net/~xiaofeya/8078812/webrev.02

-Felix


* * *

Moving on to the actual test failure, the message from 
testUnnamedApp() is as follows:


==
Command line: 
[/Users/src/jdk-dev/jdk9-dev/build/macosx-x86_64-normal-server-fastdebug/jdk/bin/java 
-ea -esa -mp mods 
-Djava.rmi.server.codebase=file:/Users/src/jdk-dev/jdk9-dev/jdk/testoutput/JTwork/java/rmi/module/ModuleTest/./mods/mserver/ 
-cp classes testpkg.DummyApp 65071 ]


Error: A JNI error has occurred, please check your installation and 
try again
Exception in thread "main" java.lang.NoClassDefFoundError: 
serverpkg/Hello

at java.lang.Class.getDeclaredMethods0(java.base/Native Method)
at 
java.lang.Class.privateGetDeclaredMethods(java.base/Class.java:2937)
at 
java.lang.Class.privateGetMethodRecursive(java.base/Class.java:3282)

at java.lang.Class.getMethod0(java.base/Class.java:3252)
at java.lang.Class.getMethod(java.base/Class.java:1961)
at 
sun.launcher.LauncherHelper.validateMainClass(java.base/LauncherHelper.java:648)
at 
sun.launcher.LauncherHelper.checkAndLoadMain(java.base/LauncherHelper.java:499)

Caused by: java.lang.ClassNotFoundException: serverpkg.Hello
at 
jdk.internal.loader.BuiltinClassLoader.loadClass(java.base/BuiltinClassLoader.java:366)
at 
jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(java.base/ClassLoaders.java:184)

at java.lang.ClassLoader.loadClass(java.base/ClassLoader.java:419)
... 7 more

test ModuleTest.testUnnamedApp(): failure
java.lang.AssertionError: expected [0] but found [1]
[remainder of stack trace elided]
==

The main class is testpkg.DummyApp, but the launcher fails with 
NoClassDefFoundError on serverpkg/Hello. This was a bit of a puzzle, 
but I finally figured out that this is occurring during the 
verification of the DummyApp class. DummyApp depends on Hello, which 
can't be found, so it fails verification before DummyApp.main() gets 
called.


It's also strange that it says "A JNI error has occurred" but that 
might be a red herring.


In any case the real issue is that serverpkg.Hello can't be found. The 
reason it can't be found is that it's in a module. That module is on 
the module path, but the module isn't resolved by default. Thus its 
classes aren't visible to classes in the unnamed module.


To add modules to the set of resolved modules, use the -addmods 
option. In this case, testUnnamedApp() has the client and server 
classes in modules, so it needs to have the arguments


-addmods mclient,mserver

added to its command line. Since the mclient module explicitly depends 
on the mserver module, this could instead just e


-addmods mclient

The testUnnamedAppandClient() metho

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

[Felix Yang]

Hi Stuart,

   please review the updated webrev, which remove not-suggested 
filesystem modification and codebase stuff:


http://cr.openjdk.java.net/~xiaofeya/8078812/webrev.02/

Thanks,

Felix

Re: 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

Re: 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

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

[Stuart Marks]

On 5/20/16 4:59 PM, Jonathan Gibbons wrote:

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.


Sure, either way will work. I had started with String because the data 
originates as strings, either from system properties or from string literals.


I switched things around to see what things looked like, starting with Path and 
converting to String as necessary. Basically I declared the constants using 
Paths.get(). This meant fileJoin() could disappear, since Paths.get() can 
concatenate multiple elements.


Paths.get() disappeared from several places inline, and was replaced with a 
chain of calls to resolve() in a couple others. I had to add toString() in a few 
places as well.


The pathJoin() helper had to change a bit though. Formerly it concatenated 
strings with File.pathSeparator; now it has to convert Path objects to strings 
before doing so. The easiest way to do this was to use a short stream:


static String pathJoin(Path... paths) {
return Arrays.stream(paths)
 .map(Path::toString)
 .collect(Collectors.joining(File.pathSeparator));
}

Overall I'd say six of one, half a dozen of the other.

s'marks

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

[Jonathan Gibbons]

On 05/23/2016 02:31 PM, Stuart Marks wrote:

On 5/20/16 4:59 PM, Jonathan Gibbons wrote:
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.


Sure, either way will work. I had started with String because the data 
originates as strings, either from system properties or from string 
literals.


I switched things around to see what things looked like, starting with 
Path and converting to String as necessary. Basically I declared the 
constants using Paths.get(). This meant fileJoin() could disappear, 
since Paths.get() can concatenate multiple elements.


Paths.get() disappeared from several places inline, and was replaced 
with a chain of calls to resolve() in a couple others. I had to add 
toString() in a few places as well.


The pathJoin() helper had to change a bit though. Formerly it 
concatenated strings with File.pathSeparator; now it has to convert 
Path objects to strings before doing so. The easiest way to do this 
was to use a short stream:


static String pathJoin(Path... paths) {
return Arrays.stream(paths)
 .map(Path::toString)
.collect(Collectors.joining(File.pathSeparator));
}

Overall I'd say six of one, half a dozen of the other.

s'marks



Yes, and the example is small enough that maybe six of one really does 
balance half a dozen of the other.  But as a general matter of style, I 
think it is preferable to use Path when you really want paths.


That being said, it would be nice to have a method on the Paths API to 
create a search path from a series of Path objects. (your pathJoin method)


-- Jon

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

[Felix Yang]

Hi Stuart,

thanks for the comments.

On 2016/5/21 7:52, 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.

...

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.
I accept your changes. 'pathJoin' looks cool. Though, I personally 
prefer to work with Path rather than strings (fileJoin). Any way, both 
ways are OK for me.


Thanks,
Felix


* * *

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.
OK, I updated the test to a pure automatic modules version. Following 
subset is selected. Please suggest:

1. all in automatic modules
2. only dummy app as automatic module, and others are in classpath
3. client/server as automatic module, and dummy app is in classpath
4. server/app as automatic module, and client is in classpath

Updated webrev:
 http://cr.openjdk.java.net/~xiaofeya/8078812/webrev.03/

Thanks,
Felix


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.

How about try this in later expansion?
All declared as named modules
Compile to exploded dir as usual
Enhance the JarUtil to accept a filter to exclude any file during 
creating jars (in this case, module-info.class)
Then expand the test by specifying mp/cp with automatic modules, 
explored named modules


-Felix


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

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

[Stuart Marks]

Hi Felix,

This is looking quite nice.

One small cleanup. Client.java has a constructor Client(int port), and an int 
port field. Both are unused and can be removed.



I accept your changes. 'pathJoin' looks cool. Though, I personally prefer to
work with Path rather than strings (fileJoin). Any way, both ways are OK for me.


If you'd like to switch to using Path, that's fine with me. Jon G has also 
expressed such a preference. It should be a fairly straightforward change. The 
fileJoin() utility can be removed, and pathJoin() can be replaced with the 
alternative version from my previous mail. Otherwise it's mostly a matter of 
removing calls to Paths.get() and adding calls to toString() in the right places.



OK, I updated the test to a pure automatic modules version. Following subset is
selected. Please suggest:
1. all in automatic modules
2. only dummy app as automatic module, and others are in classpath
3. client/server as automatic module, and dummy app is in classpath
4. server/app as automatic module, and client is in classpath

Updated webrev:
 http://cr.openjdk.java.net/~xiaofeya/8078812/webrev.03/


I think this is a fine subset. The different test cases are all quite simple and 
they're all straightforward variations of each other, and they all use the same 
test fixture!



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.

How about try this in later expansion?
All declared as named modules
Compile to exploded dir as usual
Enhance the JarUtil to accept a filter to exclude any file during creating jars
(in this case, module-info.class)
Then expand the test by specifying mp/cp with automatic modules, explored named
modules


Yes, this would be fine for future work. The exclusion of module-info.class from 
jar file creation seems like a reasonable approach.


If you eventually end up adding more test cases, it will probably result in a 
lot of redundancy in the way the sub-JVM is invoked. Also, the way to specify 
whether a particular component is in an unnamed module, an automatic module, or 
a named module will be somewhat cumbersome, so you'll probably want helper 
methods for that. And maybe you'd end up with a data provider to drive the test 
cases, instead of writing out individual test methods.


But all of this can be future work.

* * *

In summary, I think there is only the Client.java cleanup, and (optionally) the 
String=>Path conversion.


Did you need me to push this for you? This will go into jdk9/dev, right? I don't 
think it needs to go into jake.


s'marks

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

[Felix Yang]

HI Stuart,


On 2016/5/27 5:57, Stuart Marks wrote:

* * *

In summary, I think there is only the Client.java cleanup, and 
(optionally) the String=>Path conversion.


Did you need me to push this for you? This will go into jdk9/dev, 
right? I don't think it needs to go into jake. 

Please sponsor this change. I didn't change other parts.
New webrev: http://cr.openjdk.java.net/~xiaofeya/8078812/webrev.03/

Thank you very much,
Felix