RFR 8066085: Need a sanity test for rmic -iiop

[Stuart Marks]

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