RFR 8066085: Need a sanity test for rmic -iiop

[FELIX YANG]

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