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
