code review for attach on demand (AOD) test fix (7035555)

[Daniel D. Daugherty]

Thanks! And thanks for the quick review!

Dan


On 4/12/2011 9:53 AM, Dmitry Samersoff wrote:
> Dan,
>
> Thumb up!
> -Dmitry
>
> On 2011-04-12 19:47, Daniel D. Daugherty wrote:
>> On 4/12/2011 9:33 AM, Dmitry Samersoff wrote:
>>> Dan,
>>>
>>> test/com/sun/tools/attach/ApplicationSetup.sh
>>>
>>> I don't know whether you use the same shell script for UNIX and Cygwin
>>
>> Yes, these scripts are intended to run on all supported
>> platforms.
>>
>>
>>> but I think
>>>
>>> "${TESTCLASSES}/Application.jar"
>>>
>>> is safer than
>>>
>>> "${TESTCLASSES}"/Application.jar
>>
>> Not sure what you mean by "safer". These should
>> parse equivalently (and that wasn't something
>> that I changed).
>>
>>
>>> test/com/sun/tools/attach/BasicTests.java
>>>
>>> Nit:
>>>
>>> + System.out.println("INFO: SilverBullet.jar not being found and an");
>>> + System.out.println("INFO: agent failing to start.");
>>>
>>> It might be better to keep article with a word. i.e
>>>
>>> + System.out.println("INFO: SilverBullet.jar not being found and ");
>>> + System.out.println("INFO: an agent failing to start.");
>>
>> Agreed. I'll change that (except for that trailing space).
>>
>>
>>>
>>> The same for
>>>
>>> +System.out.println("INFO: appear in the application log about a");
>>> +System.out.println("INFO: BadAgent including a RuntimeException and");
>>
>> I'll fix this one also.
>>
>>
>>>
>>>
>>> test/com/sun/tools/attach/BasicTests.sh
>>>
>>> if [ echo ${osrev} | grep -q 'CYGWIN[^ ]*-5\.0' ] ; then
>>>
>>> IMHO, is better way to use grep but probably you don't need to 
>>> change it.
>>
>> I'm not quite sure why Kelly wrote these if-statements this
>> way, but if I fix the Cygwin one, then I'd feel obligated
>> to fix the MKS one also. I don't really want to got there (yet).
>>
>> So is this a thumbs up review or not? Just trying to be clear.
>>
>> Dan
>>>
>>> -Dmitry
>>>
>>>
>>>
>>> On 2011-04-12 19:12, Daniel D. Daugherty wrote:
>>>> Greetings,
>>>>
>>>> I have minor fixes to an Attach On Demand (AOD) test that I'd
>>>> like to get into T&L snapshot for OpenJDK7-B140 (next week).
>>>> Here is my proposed changeset comment:
>>>>
>>>> 7035555: 4/4 attach/BasicTests.sh needs another tweak for Cygwin
>>>> Summary: Test needs to properly detect missing
>>>> AgentInitializationException. Clarify when exceptions
>>>> are expected. Another Cygwin tweak.
>>>>
>>>> Yes, this bug started out as a tweak for Cygwin and then I
>>>> discovered and fixed the other small issues.
>>>>
>>>> Here is the URL to the webrev:
>>>>
>>>> http://cr.openjdk.java.net/~dcubed/7035555-webrev/0/
>>>>
>>>> I've run the fix through JPRT testing and I ran into an existing
>>>> intermittent failure:
>>>>
>>>> 6461635 4/3 BasicTests.sh test fails intermittently.
>>>>
>>>> I've checked nightly testing and 6461635 makes a periodic
>>>> appearance across all platforms.
>>>>
>>>> Thanks, in advance, for any comments.
>>>>
>>>> Dan
>>>>
>>>
>>>
>
>