RFR 8066085: Need a sanity test for rmic -iiop

[Stuart Marks]

Hi Felix,

Thanks for the updates. I've pushed the changeset. [1] Sorry for the delay; 
holidays, you know.

s'marks

[1] http://hg.openjdk.java.net/jdk9/dev/jdk/rev/af229cf4a61a

On 12/24/14 7:27 AM, FELIX YANG wrote:
> 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
>>>>
>>>>
>>>
>