[Fwd: SDK Test Fixes Batch for 2010.07 (6941287, 6962804, 6964018)]

[David Holmes]

Dan,

You can add me to the first two if you like. The third was over my head :)

As Alan said these are test changes so not so critical ... and if 
there's a problem with them you get to fix it again :)

Cheers,
David

Daniel D. Daugherty said the following on 07/22/10 06:12:
> Alan,
> 
> I'm adding the OpenJDK aliases back into this e-mail thread for
> this reply. Just trying to keep folks in the loop...
> 
> Thanks for squeezing in time for a review of 6964018.
> 
> 
> On 7/21/2010 1:43 PM, Alan Bateman wrote:
>> Daniel D. Daugherty wrote:
>>> On 7/21/2010 4:42 AM, Alan Bateman wrote:
>>>> Daniel D. Daugherty wrote:
>>>>> Alan,
>>>>>
>>>>> A quick look from you at the third webrev would be much appreciated...
>>>>>
>>>>> Dan
>>>> Sorry Dan, I just don't have the cycles today due to a couple of 
>>>> high priority issues.  I might get cycles on Friday if we really 
>>>> want me to review.
>>>
>>> I may just go with only Kelly's review. :-|
>> I just spent 15-20m going through 
>> http://cr.openjdk.java.net/~dcubed/6964018-webrev/0/ in more detail 
>> and it looks fine to me. Thanks for explaining about the refactoring 
>> as it looks more than it really is on first glance.  I'm happy with 
>> the updates to the existing tests and happy to see the new 
>> test/java/util/logging tests are more reliable. Given that this is 
>> tests only change then one reviewer should be fine.
> 
> I'll put both you and Kelly down for 6964018. I'll put just Kelly
> down for the first two. Of course, that could change if I get more
> reviewer comments.
> 
> 
>> Minor nits in ShutdownSimpleApplication.java is that System.exit is 
>> probably not needed.
> 
> Because I added the 'set -e' option, I think it is needed. Otherwise
> I get a random process exit status.
> 
> 
>> Also the pre-existing empty comment lines L25-26 can probably be removed.
> 
> I wondered why those were there so I left them in. I had meant to
> ping you about them, but I forgot... I'll remove them.
> 
> 
>> Same comments for SimpleApplication.
> 
> Hopefully, the same resolutions will be OK with you.
> 
> 
>> BTW: In test/java/util/logging/LoggerWeakRefLeak.sh you can listed 
>> issues with jmap - are they still valid with the updated test and the 
>> recent fix to the attach API?
> 
> I suspect that your recent attach API fix will resolve those
> issues, but I plan to specifically test for that after your
> fix gets promoted.
> 
> 
>> The only outstanding issue that I can think of is where you try to 
>> attach just as the process is about to exit (but with the updated test 
>> this doesn't happen due to the "handshake"/socket connection).
> 
> Agreed and thank you for pointing me in that direction.
> 
> Dan
>