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

[Daniel D. Daugherty]

On 7/21/2010 5:41 PM, David Holmes wrote:
> Dan,
>
> You can add me to the first two if you like.
Thanks!


> The third was over my head :)

I doubt that, but I have two for that one :-)


> As Alan said these are test changes so not so critical ...

Unless you're trying to get an SDK/JDK job through the JPRT queue :-)


> and if there's a problem with them you get to fix it again :)

Yup. And now I have a template/shared code to use to fix other
annoyingly intermittent tests...

Dan


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