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
