RFR 8066085: Need a sanity test for rmic -iiop

[FELIX YANG]

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
>
>