RFR 8066085: Need a sanity test for rmic -iiop

[Stuart Marks]

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