Re: Ping - Re: RFR 8078812, Test RMI with client and servers as modules
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
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
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
to me that
we need to support modules in the RMI codebase, but at least there should be
tests for cases we think are important.
2) In DummyApp, if creating the Client fails (e.g., because of
NoClassDefFoundError), that error will propagate out of the main() method,
bypassing the calls to System.exit(). The Server object has already been
exported, which will cause the JVM to live indefinitely. This causes the test to
hang and eventually time out, and to leave stale JVM processes after the test run.
In any case, I think the next step is to come up with a good design for varying
the modular structure of the classes for the different test cases, without
modifying the filesystem. Let's work on that, and we can work on the other stuff
later.
s'marks
On 4/14/16 1:45 AM, Felix Yang wrote:
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/
<http://cr.openjdk.java.net/%7Exiaofeya/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
Re: RFR 8078812, Test RMI with client and servers as modules
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
