Hi Stuart,
this is the updated webrev:
http://cr.openjdk.java.net/~xiaofeya/8066085/webrev.02/. Please sponsor
this changeset for me.
Thank you very much,
-Felix
On 12/24/2014 1:01 PM, Stuart Marks wrote:
Hi Felix,
Good improvements. I think this is pretty close; I have just a few
minor comments.
* The test is named IIOPSanityTest but it's in a directory named
iiopCompilation. I don't think we need different names. There are many
test classes named "Foo" that reside in a directory named "foo"; I
think we should do the same here. I think keeping the directory name
"iiopCompilation" and renaming the class to "IIOPCompilation" makes
things a bit more descriptive.
* Declaring string constants like RMIC_PROGRAM, IIOP_OPTION, and
CP_OPTION doesn't really help things, since they're each used exactly
once, the string values are unlikely to change, and the symbolic names
are just derived from the string vaues. You might as well use string
literals directly in each place they're needed.
* Printing the command that's about to be executed is a good idea, but
the printf statement that prints the command has to be kept in synch
with the array containing the command and args. If the command were to
change, it'd be easy to forget to update the printf statement or to
get it wrong.
* I'll also observe that there is an overload of the ProcessBuilder
constructor that takes a List<String> instead of a String[]. Lists are
often easier to work with than arrays, and they have a toString()
method that can be used implicitly by print statements. This suggests
an alternative for constructing the rmic command line, something like
the following:
List<String> command = Arrays.asList(
rmicProgramStr, "-iiop", "-classpath", testClasses, classname);
System.out.println("Running command: " + command);
After you fix these up I think this will be ready to go in.
I can sponsor this changeset for you if you need a sponsor.
Thanks,
s'marks
On 12/19/14 1:56 AM, FELIX YANG wrote:
Hi Stuart,
please review the updated webrev:
http://cr.openjdk.java.net/~xiaofeya/8066085/webrev.01/
Thanks,
-Felix
On 12/12/2014 4:08 PM, Stuart Marks wrote:
On 12/11/14 8:41 PM, FELIX YANG wrote:
Hi all,
please review the fix to add a sanity checking for rmic -iiop
compiling.
Bug:
https://bugs.openjdk.java.net/browse/JDK-8066085
Webrev:
http://cr.openjdk.java.net/~xiaofeya/8066085/webrev.00/
Hi Felix,
Thanks for picking up the writing of this test. I have several
comments.
1. Usually we try to avoid naming tests after bug IDs. There are
such tests in
the suite but most tests have more descriptive names. I'd suggest
something
like "iiopSanityTest".
2. There are other rmic tests in jdk/test/sun/rmi/rmic; the test
should be
moved there (and the path to the TestLibrary updated correspondingly).
3. The test summary should be a bit more explicit. Instead of "a
sanity test
for rmic -iiop compiling" I'd suggest something like "Compiles a
PortableRemoteObject with rmic -iiop and ensures that stub and tie
classes are
generated."
4. When invoking rmic, you need to make sure to pass the -iiop option.
5. Add check for the _Tie class as well as the _Stub class. Not that
it really
matters, since we know these classes are in the unnamed package, but
the stub
and tie class files would end up in directories corresponding to
their package
names -- if they were in a named package.
6. In doTest(), the try/catch of InterruptedException and
IOException and call
to TestLibrary.bomb() is mostly unnecessary. (Yes, other RMI tests
do this,
and I've been removing it where appropriate. In the RMI test
library, the
bomb() call merely wraps and rethrows the exception, and doesn't
really add
any value. It's like calling fail() in a test-ng test.) In general
an uncaught
exception will cause a jtreg test to fail, so you might as well just
declare
the method with "throws Exception" and let them propagate. This
helps reduce
clutter.
7. Similarly I'm not sure it's necessary to check the classname
argument for
null; the failure should be apparent enough if classname is null.
But if you
think it's worth it, a concise way to null-check an argument is to use
Objects.requireNonNull().
8. The "rmiComplie" method is misnamed (or typo'd); I'd suggest
something like
"runRmic".
9. Changing the directory of the rmic subprocess is confusing,
because it
makes different parts of the program deal with different paths to
the same
files. In addition, switching directories to the test.classes
directory will
leave the stub/tie classes there, which could possibly confuse
future test
runs, since that directory isn't necessarily cleared between test runs.
The default working directory of jtreg tests is the "scratch"
directory, which
is cleared before each test run. Leaving the current directory as-is
when
running the subprocesses will write the generated stub/tie classes
to the
scratch directory, which is the right thing.
This implies that you'll need to add the -classpath argument and a
path to the
test.classes directory to the rmic command line.
Thanks,
s'marks
