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

[Daniel D. Daugherty]

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