RFR 8078812, Test RMI with client and servers as modules

[Stuart Marks]

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
>