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

[Dmitry Samersoff]

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


-- 
Dmitry Samersoff
Java Hotspot development team, SPB04
* There will come soft rains ...